Implementing the `chmod` support #772

webbunivAdmin · 2024-10-24T12:23:36Z

Add `chmod` Support to Eio

This PR implements support for changing file permissions using the chmod operation in the Eio library.
#764

Key Changes:

Added chmod function that allows setting file permissions on a target resource:
- Parameters:
  - ~follow: Determines whether to follow symlinks.
  - ~perm: Specifies the permission bits to set.
  - t: The target file or directory.
Exception Handling:
- Exceptions are handled to provide robust error reporting.
- Context-aware re-raising of exceptions in case of failure for better debugging.
Seamless Integration with Eio’s resource abstraction, supporting various permission modes.

Tests:

patricoferris

For future reference, it would be good to correctly attribute code to the original author. I believe most of this was from the branch I pointed to. You could have committed using co-authored-by or by cherry-picking from my branch.

I think we're going to need some tests here too, similar to those in the symlink PR

patricoferris

Also seems to be a tmp/test_file that shouldn't be committed.

patricoferris · 2024-10-27T10:24:50Z

examples/chmod/main.ml

+    let permissions = get_permissions (Eio.Path.native_exn file_path) env in
+    Printf.printf "Permissions: %o\n" permissions;
+
+    Eio.Path.chmod ~follow:true ~perm:0o600 file_path;


I think we could remove this example and make the code a test in tests/fs.md. Also it would be good to change the permissions to something other that the same permissions used to create the file, that way we can see chmod working.

patricoferris

Thanks @webbunivAdmin -- getting there :))

patricoferris · 2024-10-28T17:24:34Z

tests/fs.md

@@ -18,7 +18,6 @@ open Eio.Std
 let ( / ) = Path.( / )

 let run ?clear:(paths = []) fn =
-  Eio_main.run @@ fun env ->


Why did you remove this @webbunivAdmin ? It is very important as it is the effect handler for Eio code. Without this none of this tests will work.

patricoferris · 2024-10-28T17:25:06Z

tests/fs.md

@@ -1011,3 +1017,23 @@ Exception: Failure "Simulated error".
 +"/" / "" = "/"
 - : unit = ()
 ```
+
+```ocaml


To run the test make sure you run dune runtest and dune promote the output back to this file.

This still holds true.

webbunivAdmin · 2024-10-29T16:19:47Z

@patricoferris for checkout for the new tests have written

webbunivAdmin · 2024-10-29T16:20:13Z

@patricoferris for checkout for the new tests have written

patricoferris · 2024-11-01T13:33:34Z

Hey @webbunivAdmin,

A few notes from me here. Firstly, in terms of co-authoring, you only need to add this to your commits that are indeed co-authored by me. Everything after the initial commit is your own work and need not have co-authored on them.

Secondly, I do wonder what these pushes of update fs.md are for? Do you not have the code building and running locally to be able to run dune runtest to see if it is working before pushing. It is also possible to undo commits on your branch:

git reset --hard HEAD~n        # Reset head backwards n commits, dropping the commits
edit edit edit
git add foo                    # add new changes
git commit -m "Useful message" # commit them
git push -f                    # force push to your branch to undo the commits

Some of the failures right now are not due to the code (just flakey tests/CI). However, the latest code still has no test of the chmod feature.

patricoferris · 2024-11-02T09:47:28Z

tests/fs.md

@@ -1011,3 +1017,23 @@ Exception: Failure "Simulated error".
 +"/" / "" = "/"
 - : unit = ()
 ```
+
+```ocaml


This still holds true.

patricoferris · 2024-11-02T09:48:40Z

tests/fs.md

+
+  let file_path = cwd / "test-file" in
+  Path.save ~create:(`Exclusive 0o644) file_path "test data";
+  traceln "+create <cwd:test-file> with permissions 0o644 -> ok";


The idea of these tests is that the try_x variant of all of the Path functions does two things. (1) it invokes the function it is named after e.g. chmod and (2) it prints something using traceln that we can then inspect to see what it did.

patricoferris · 2024-11-02T09:49:11Z

tests/fs.md

+
+  let file_path = cwd / "test-file" in
+  Path.save ~create:(`Exclusive 0o644) file_path "test data";
+  traceln "+create <cwd:test-file> with permissions 0o644 -> ok";


So we don't need this.

Suggested change

traceln "+create <cwd:test-file> with permissions 0o644 -> ok";

patricoferris · 2024-11-02T10:06:40Z

tests/fs.md

+  traceln "+<cwd:test-file> initial permissions = %o" initial_perm;
+  assert (initial_perm = 0o644);
+
+  Path.chmod ~follow:true ~perm:0o400 file_path;


I envisage this code to look something like:

try_chmod ~follow:true ~perm:0o400 file_path; try_stat ~info_type:`Perm file_path;

Note:

It uses try_chmod which prints the information about the execution of chmod.

It uses try_stat to print information about the file, I've modified it slightly so that try_stat would look something like

-let try_stat path = +let try_stat ?(info_type=`Kind) path = let stat ~follow = - match Eio.Path.stat ~follow path with - | info -> Fmt.str "@[<h>%a@]" Eio.File.Stat.pp_kind info.kind + match Eio.Path.stat ~follow path, info_type with + | info, `Perm -> Fmt.str "@[<h>%o@]" info.perm + | info, `Kind -> Fmt.str "@[<h>%a@]" Eio.File.Stat.pp_kind info.kind | exception Eio.Io (e, _) -> Fmt.str "@[<h>%a@]" Eio.Exn.pp_err e in

patricoferris · 2024-11-02T10:07:04Z

lib_eio_windows/fs.ml

@@ -176,6 +181,7 @@ end = struct
    with_parent_dir t path @@ fun dirfd path ->
    Err.run (Low_level.symlink ~link_to dirfd) path

+


Make sure your commits don't have unnecessary changes like this newline.

patricoferris · 2024-11-02T10:11:14Z

lib_eio_windows/fs.ml

@@ -89,6 +89,11 @@ end = struct

  let v ~label ~sandbox dir_path = { dir_path; sandbox; label; closed = false }

+  let chmod (_t : t) ~(follow : bool) ~(perm : int) (_path : string) : unit =


Like with the other functions that do not have an implementation, we should actually still create a stub in eio_windows_stubs.c that fails. See, for example, how symlink does it. This reduces the burden for someone to come along and do a chmod implementation for Windows.

webbunivAdmin · 2024-11-06T10:35:15Z

@patricoferris At last i made

patricoferris · 2024-11-15T17:46:07Z

Before reviewing @webbunivAdmin could you tidy up the Git history a little here. 35 of the 49 commits are called Update fs.md ^^" ! Mostly you could just squash the history with a soft reset and a force push to your branch.

webbunivAdmin · 2024-11-17T08:45:43Z

@patricoferris i thing am done

patricoferris · 2024-11-20T21:27:28Z

Hey @webbunivAdmin, sorry if I wasn't clear enough in my previous post. To make reviewing this easier could you tidy up the git history please. For example there is a pretty good stackoverflow post about how you can do that here: https://stackoverflow.com/questions/5189560/how-do-i-squash-my-last-n-commits-together

Does that make sense? If you don't want to squash the history do let me know and I'll take over the PR at some point to get it in a reviewable shape. Thanks :))

It's quicker to do it in a single call.

This is a quick fix for ocaml-multicore#732.

path.mli says: > In Eio, the directory separator is always "/", even on Windows. However, `Path.(/)` used `Filename.concat` to create paths, which uses the native separator ("\" on Windows).

This allows e.g. using the result of the `import_socket_stream` as a `stream_socket_ty` without needing a cast.

This can take quite a long time.

Instead of having separate Alloc, Alloc_or_wait and Free effects, the scheduler now provides a single Get effect to return itself, and the actual work is now done in the calling fiber. This is cleaner, and seems to be slightly faster too. Note that `alloc_fixed_or_wait` is currently not cancellable (it wasn't before either, but it's more obvious now). It would be possible to use DLS to store the scheduler rather than using an effect. However, the improvement in speed is minimal and there are some complications with sys-threads, so probably better to wait for OCaml to support thread-local-storage first.

Co-Authored-By: Patrick Ferris <[email protected]>

If the fiber fails, record the backtrace when failing the switch. `fork` itself already did this, but `fork_daemon` and `fork_promise_exn` didn't.

patricoferris · 2024-11-30T12:47:34Z

Closing this in favour of #785

patricoferris requested changes Oct 25, 2024

View reviewed changes

patricoferris requested changes Oct 27, 2024

View reviewed changes

patricoferris requested changes Oct 28, 2024

View reviewed changes

webbunivAdmin requested a review from patricoferris October 29, 2024 14:52

patricoferris reviewed Nov 2, 2024

View reviewed changes

webbunivAdmin requested a review from patricoferris November 6, 2024 10:34

talex5 and others added 17 commits November 21, 2024 13:32

eio_linux: don't call submit immediately before wait

085a0dc

It's quicker to do it in a single call.

eio_linux: don't record submit events when there's nothing to submit

e403d8f

Add examples/fs showing how to walk a directory tree

fffcf23

Prepare release

72bf01b

eio_linux: add work-around for signals race

68e50f8

This is a quick fix for ocaml-multicore#732.

Add Eio_unix.Net.import_socket_listening

877af2e

Prepare release

14fefcb

Add example to Buf_read.seq documentation

a68438f

define struct clone_args for linux-lts versions that don't have it

3de9383

eio_windows: improve openat error handling

60a3c42

eio_windows: run the fs example in CI

e364ed6

Eio.Path: always use "/" as separator

c698586

path.mli says: > In Eio, the directory separator is always "/", even on Windows. However, `Path.(/)` used `Filename.concat` to create paths, which uses the native separator ("\" on Windows).

Eio_unix.Net: make some return types more polymorphic

0769ce3

This allows e.g. using the result of the `import_socket_stream` as a `stream_socket_ty` without needing a cast.

examples/fs: show how to read files while scanning

c92704f

Record trace event when spawning processes

9e00b15

This can take quite a long time.

eio_linux: allow alloc_fixed_or_wait to be cancelled

175ccaf

webbunivAdmin and others added 21 commits November 21, 2024 13:36

trying to remove some bugs from the tests

75732f1

Co-Authored-By: Patrick Ferris <[email protected]>

Update fs.md

02225be

Co-Authored-By: Patrick Ferris <[email protected]>

Update fs.md

9390eb4

Co-Authored-By: Patrick Ferris <[email protected]>

Update fs.md

17a20aa

Co-Authored-By: Patrick Ferris <[email protected]>

Update fs.md

e54ce95

Co-Authored-By: Patrick Ferris <[email protected]>

Update fs.md

84cc303

try_chmod implementation

04843a7

following the comments

34e7ac4

Update fs.md

650088b

Update fs.md

cd4fb2b

Update fs.md

ce4adee

Update fs.md

e9b4c36

Update fs.md

97d9220

Co-Authored-By: Patrick Ferris <[email protected]>

Update fs.ml

e051c6e

Co-Authored-By: Patrick Ferris <[email protected]>

trying to remove some bugs from the tests

a0e5da2

Co-Authored-By: Patrick Ferris <[email protected]>

Update fs.md

e6c0861

Co-Authored-By: Patrick Ferris <[email protected]>

Update fs.md

7fb0485

Co-Authored-By: Patrick Ferris <[email protected]>

Update fs.md

f477b32

Co-Authored-By: Patrick Ferris <[email protected]>

Update fs.md

0393837

Co-Authored-By: Patrick Ferris <[email protected]>

following the comments

55e7f8d

Preserve backtraces in fork_daemon and fork_promise_exn

65689dc

If the fiber fails, record the backtrace when failing the switch. `fork` itself already did this, but `fork_daemon` and `fork_promise_exn` didn't.

webbunivAdmin force-pushed the main branch from bb8c90d to 65689dc Compare November 21, 2024 10:44

webbunivAdmin added 5 commits November 21, 2024 13:49

to resolve the conflicts

0c79cbc

Merge branch 'ocaml-multicore:main' into main

121137d

now they are set

a060da7

Merge branch 'main' of https://github.com/webbunivAdmin/eio

fa65b09

putint the new line

38b1f77

patricoferris mentioned this pull request Nov 29, 2024

Support chmod in Eio_linux and Eio_posix #785

Draft

patricoferris closed this Nov 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing the `chmod` support #772

Implementing the `chmod` support #772

webbunivAdmin commented Oct 24, 2024 •

edited

Loading

patricoferris left a comment

patricoferris left a comment

patricoferris Oct 27, 2024

patricoferris left a comment

patricoferris Oct 28, 2024

patricoferris Oct 28, 2024

patricoferris Nov 2, 2024

webbunivAdmin commented Oct 29, 2024

webbunivAdmin commented Oct 29, 2024

patricoferris commented Nov 1, 2024

patricoferris Nov 2, 2024

patricoferris Nov 2, 2024

patricoferris Nov 2, 2024

patricoferris Nov 2, 2024

patricoferris Nov 2, 2024

patricoferris Nov 2, 2024

webbunivAdmin commented Nov 6, 2024

patricoferris commented Nov 15, 2024

webbunivAdmin commented Nov 17, 2024

patricoferris commented Nov 20, 2024

patricoferris commented Nov 30, 2024

		@@ -176,6 +181,7 @@ end = struct
		with_parent_dir t path @@ fun dirfd path ->
		Err.run (Low_level.symlink ~link_to dirfd) path

		@@ -89,6 +89,11 @@ end = struct

		let v ~label ~sandbox dir_path = { dir_path; sandbox; label; closed = false }

		let chmod (_t : t) ~(follow : bool) ~(perm : int) (_path : string) : unit =

Implementing the chmod support #772

Implementing the chmod support #772

Conversation

webbunivAdmin commented Oct 24, 2024 • edited Loading

Add chmod Support to Eio

Key Changes:

Tests:

patricoferris left a comment

Choose a reason for hiding this comment

patricoferris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricoferris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webbunivAdmin commented Oct 29, 2024

webbunivAdmin commented Oct 29, 2024

patricoferris commented Nov 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webbunivAdmin commented Nov 6, 2024

patricoferris commented Nov 15, 2024

webbunivAdmin commented Nov 17, 2024

patricoferris commented Nov 20, 2024

patricoferris commented Nov 30, 2024

Implementing the `chmod` support #772

Implementing the `chmod` support #772

webbunivAdmin commented Oct 24, 2024 •

edited

Loading

Add `chmod` Support to Eio