-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add symlink support #715
Add symlink support #715
Conversation
ca9b02e
to
17ab9b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is backwards. The target of a symlink is its content (a string).
symlink() creates a symbolic link named linkpath which contains the string target.
(from symlink(2))
(the analogy is that ln -s src dst
is like cp src dst
)
So the API should be:
val symlink : ?exists_ok:bool -> target:string -> _ t -> unit
In particular, we want to limit where you can create a symlink, not what it can point to.
Ah! Glad I added the caveat to check that -- thanks ^^" ! I've updated everything, I think, to be correct now. I went for val symlink : ?exists_ok:bool -> link_to:string -> _ t -> unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good - thanks!
I pushed some commits to update primitives.h (happens automatically if you have lintcstubs_arity_cmt
in your PATH) and I tried to make the signatures more consistent.
Ready to merge once the exists_ok
thing is clarified/fixed/removed.
lib_eio/path.mli
Outdated
Eio.Path.(symlink ~link_to:"hello.txt" (env#cwd / "world.txt")) | ||
]} | ||
|
||
@param exist_ok If [false] (the default) then we raise {! Fs.Already_exists} if the symlink already exists with that target. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if it exists with a different target, or isn't a symlink? Also exist
vs exists
.
(could just drop the exists_ok
feature and add it later)
Removing let test_statx () =
let module X = Uring.Statx in
Eio_main.run @@ fun env ->
let ( / ) = Eio.Path.( / ) in
let path = env#cwd / "test3.data" in
Eio.Path.with_open_out path ~create:(`Exclusive 0o600) @@ fun file ->
Eio.Flow.copy_string "hello" file;
Eio.Flow.copy_string "+" file strace: https://gist.github.com/patricoferris/ace0a69433f299ee4ac94368028bb96b
|
By uninterruptable, you mean Ctrl-C doesn't kill it? The machine has to be rebooted to remove the stuck process? Does it happen if you just run that single test as a complete program? e.g. this program works for me: let () =
Eio_main.run @@ fun env ->
let ( / ) = Eio.Path.( / ) in
let path = env#cwd / "test3.data" in
Eio.Path.with_open_out path ~create:(`Exclusive 0o600) @@ fun file ->
Eio.Flow.copy_string "hello" file;
Eio.Flow.copy_string "+" file If it still hangs without this PR, we should move it to another issue. @koonwen: can your tools help debug this? |
The tool still needs some work. In the meantime though, I can recommend using
|
@koonwen thanks - that prints a lot of info! I had a go this morning at getting bpftrace to show probes called by uring workers, and this worked a bit for me:
For me, @patricoferris's test case gives:
Some combination of these might help track it down! |
Quick update that @talex5's trace file gives me:
so presumably |
OK, I installed Ubuntu 22.04.4 and set up a zfs partition and got the same problem. The Eio tests pass on ext4, but hang on zfs.
The process itself is just waiting for
The CPU use is coming from a uring worker thread:
The worker is doing various things:
After rebooting the VM, I tried with 14ae3cf (i.e. without the symlink commits) and that hangs too, so I don't think it needs to block this PR. This is a ZFS (or Linux) bug. |
I can reproduce this (only) under a ZFS partition using Linux 6.8 too:
Works fine on a ext4 partition with the same kernel. We should extract a standalone test case here for reporting upstream. |
317120c
to
e38f6a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened ocaml-multicore/ocaml-uring#113 to track the uring bug. I've also removed the exists_ok
option, made the example a bit more meaningful, and squashed and rebased. Ready for merge now, I think.
7c05fbd
to
d3f3069
Compare
CHANGES: New features: - Add `Eio.Path.symlink` (@patricoferris ocaml-multicore/eio#715, reviewed by @talex5). - Add `Eio.Pool.use ~never_block` (@SGrondin ocaml-multicore/eio#657, reviewed by @talex5). - Add `Eio_unix.Net.import_socket_listening` (@alyssais ocaml-multicore/eio#733). - Add `Eio.Time.Timeout.sleep` (@talex5 ocaml-multicore/eio#726). Documentation: - Add `examples/fs` showing how to walk a directory tree (@talex5 ocaml-multicore/eio#730). - README: explain that `read_all` reads until shutdown (@talex5 ocaml-multicore/eio#717, reported by @Wenke-D). - Use long dash in README title (@lucperkins ocaml-multicore/eio#718). Linux backend: - Require Linux >= 5.15 (@talex5 ocaml-multicore/eio#720, reviewed by @SGrondin and @avsm). Removes a work-around that required checking whether every flow was a tty. - Don't call submit immediately before wait (@talex5 ocaml-multicore/eio#728). This is slightly faster and makes the traces clearer. - Don't record submit events when there's nothing to submit (@talex5 ocaml-multicore/eio#729). Makes the traces a bit clearer. - Split flow into its own file (@talex5 ocaml-multicore/eio#727). - Add work-around for signals race (@talex5 ocaml-multicore/eio#734). POSIX backend: - Add `_BSD_SOURCE` flag to fix build on OpenBSD (@prgbln ocaml-multicore/eio#722). - Fix sandboxed path resolution on OpenBSD (@jebrosen ocaml-multicore/eio#723, reviewed by @talex5). OpenBSD uses `ELOOP` when opening a symlink with `O_NOFOLLOW`. Build and test: - Benchmarks: record uname, Eio backend, and number of cores (@talex5 ocaml-multicore/eio#719). - Update to MDX 2.4.1 for OCaml 5.2 (@talex5 ocaml-multicore/eio#712).
CHANGES: New features: - Add `Eio.Path.symlink` (@patricoferris ocaml-multicore/eio#715, reviewed by @talex5). - Add `Eio.Pool.use ~never_block` (@SGrondin ocaml-multicore/eio#657, reviewed by @talex5). - Add `Eio_unix.Net.import_socket_listening` (@alyssais ocaml-multicore/eio#733, reviewed by @talex5). - Add `Eio.Time.Timeout.sleep` (@talex5 ocaml-multicore/eio#726). Documentation: - Add `examples/fs` showing how to walk a directory tree (@talex5 ocaml-multicore/eio#730). - README: explain that `read_all` reads until shutdown (@talex5 ocaml-multicore/eio#717, reported by @Wenke-D). - Use long dash in README title (@lucperkins ocaml-multicore/eio#718). Linux backend: - Require Linux >= 5.15 (@talex5 ocaml-multicore/eio#720, reviewed by @SGrondin and @avsm). Removes a work-around that required checking whether every flow was a tty. - Don't call submit immediately before wait (@talex5 ocaml-multicore/eio#728). This is slightly faster and makes the traces clearer. - Don't record submit events when there's nothing to submit (@talex5 ocaml-multicore/eio#729). Makes the traces a bit clearer. - Split flow into its own file (@talex5 ocaml-multicore/eio#727). - Add work-around for signals race (@talex5 ocaml-multicore/eio#734). POSIX backend: - Add `_BSD_SOURCE` flag to fix build on OpenBSD (@prgbln ocaml-multicore/eio#722). - Fix sandboxed path resolution on OpenBSD (@jebrosen ocaml-multicore/eio#723, reviewed by @talex5). OpenBSD uses `ELOOP` when opening a symlink with `O_NOFOLLOW`. Build and test: - Benchmarks: record uname, Eio backend, and number of cores (@talex5 ocaml-multicore/eio#719). - Update to MDX 2.4.1 for OCaml 5.2 (@talex5 ocaml-multicore/eio#712).
CHANGES: New features: - Add `Eio.Path.symlink` (@patricoferris ocaml-multicore/eio#715, reviewed by @talex5). - Add `Eio.Pool.use ~never_block` (@SGrondin ocaml-multicore/eio#657, reviewed by @talex5). - Add `Eio_unix.Net.import_socket_listening` (@alyssais ocaml-multicore/eio#733, reviewed by @talex5). - Add `Eio.Time.Timeout.sleep` (@talex5 ocaml-multicore/eio#726). Documentation: - Add `examples/fs` showing how to walk a directory tree (@talex5 ocaml-multicore/eio#730). - README: explain that `read_all` reads until shutdown (@talex5 ocaml-multicore/eio#717, reported by @Wenke-D). - Use long dash in README title (@lucperkins ocaml-multicore/eio#718). Linux backend: - Require Linux >= 5.15 (@talex5 ocaml-multicore/eio#720, reviewed by @SGrondin and @avsm). Removes a work-around that required checking whether every flow was a tty. - Don't call submit immediately before wait (@talex5 ocaml-multicore/eio#728). This is slightly faster and makes the traces clearer. - Don't record submit events when there's nothing to submit (@talex5 ocaml-multicore/eio#729). Makes the traces a bit clearer. - Split flow into its own file (@talex5 ocaml-multicore/eio#727). - Add work-around for signals race (@talex5 ocaml-multicore/eio#734). POSIX backend: - Add `_BSD_SOURCE` flag to fix build on OpenBSD (@prgbln ocaml-multicore/eio#722). - Fix sandboxed path resolution on OpenBSD (@jebrosen ocaml-multicore/eio#723, reviewed by @talex5). OpenBSD uses `ELOOP` when opening a symlink with `O_NOFOLLOW`. Build and test: - Benchmarks: record uname, Eio backend, and number of cores (@talex5 ocaml-multicore/eio#719). - Update to MDX 2.4.1 for OCaml 5.2 (@talex5 ocaml-multicore/eio#712).
As part of #510 this PR adds symlink support to
Eio_posix
andEio_linux
with a stubbed out implementation on Windows. I updated all the uses ofUnix.symlink
in the FS tests to useEio.Path.symlink
as a test -- definitely worth checking I didn't mess up the arguments I alwaysln
incorrectly!