Skip to content
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

Support chown in eio_posix and eio_linux #781

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

patricoferris
Copy link
Collaborator

This PR adds support to Eio for changing the ownership of a path using fchownat.

Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Rebasing on main should fix the OpenBSD CI error.

lib_eio/path.ml Outdated Show resolved Hide resolved
lib_eio/unix/eio_unix.mli Show resolved Hide resolved
lib_eio/unix/stubs.c Outdated Show resolved Hide resolved
lib_eio_linux/low_level.ml Outdated Show resolved Hide resolved
let chown ?dirfd ~follow:_ ~uid ~gid path =
in_worker_thread @@ fun () ->
match dirfd with
| None -> failwith "Chown is unsupported on Windows"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could just use Unix.chown here.

Copy link
Collaborator Author

@patricoferris patricoferris Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I'm wondering whether chown should be in the cross-platform API at all.

Unix.chown isn't implemented on Windows:
https://github.com/ocaml/ocaml/blob/c484c6932fa2eae03ba0f5a7dbdb26e3eee65eb0/otherlibs/unix/unix_win32.ml#L450 -- it's not implemented in python's os.chown either?

Do we have a rule for what makes it into the cross-platform API and what doesn't ? I mean we already have File.Unix_perm.t.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's OK to keep it. We might want a proper operation-not-supported error at some point.

Co-authored-by: Thomas Leonard <[email protected]>
Don't call Int64_val inside a blocking section.
@talex5
Copy link
Collaborator

talex5 commented Nov 22, 2024

BTW, I was planning to make a new Eio release tomorrow. Do you want me to delay that to get this in, or should I go ahead and merge this later?

@patricoferris
Copy link
Collaborator Author

Oops sorry -- cut the release, no rush on this :))

@talex5 talex5 mentioned this pull request Nov 23, 2024
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants