Initial statx support #95
Conversation
patricoferris
force-pushed
the
stat
branch
4 times, most recently
from
6669953 to
6669639
Compare
talex5
left a comment
talex5
left a comment
There was a problem hiding this comment.
Thanks - this is probably the most complex of the missing operations!
I think we're not allowed to do any "OCaml work" in the C stubs for submitting, right now I've used (abused) the Open_how struct to hold onto the path and the flags for the statx call, I should probably create another C allocated thing for that? Or are we allowed to have allocating submissions (by this I mean a C stub that can call CAMLParam and friends?).
It's best not to have the submit functions raise exceptions because we're already allocated the heap slot by then (and looks like we don't free it if they raise).
You can use Sketch.alloc here. Open_how pre-dates that and should probably be removed. The sketch buffer works for things that only need to stay around until the submit (so you can't use it for the results structure).
Would be good to allow setting and getting the mask. This is the main benefit of statx. However, we could do that later.
talex5
left a comment
talex5
left a comment
There was a problem hiding this comment.
I pushed some commits to your branch with some suggested changes. If you're happy with them, this is OK to squash and merge.
| ~mask:Uring.Statx.Mask.basic_stats | ||
| "test-openat" | ||
| statx | ||
| Uring.Statx.Flags.empty_path |
There was a problem hiding this comment.
Why empty_path here? The path isn't empty.
There was a problem hiding this comment.
Yep, I forgot to add the empty flag 👍
talex5
force-pushed
the
stat
branch
2 times, most recently
from
8167f3c to
afe108c
Compare
| "STATX_SIZE", Int; | ||
| "STATX_BLOCKS", Int; | ||
| "STATX_BASIC_STATS", Int; | ||
| "STATX_BTIME", Int; |
There was a problem hiding this comment.
-
"STATX_MNT_ID", Int; -
"STATX_DIOALIGN", Int;
I also have as newer possible options here.
| val create : unit -> internal | ||
| (** Use [create] to make an {! internal} statx struct to pass to {! statx}. *) | ||
|
|
||
| val internal_to_t : internal -> t |
There was a problem hiding this comment.
One of the key benefits to statx is that you can retrieve only the values you need, and not have everything else present. So instead of the big type t, we could just have noalloc accessors that convert the relevant internal struct field into an OCaml value. The internal block can be reused across sequential statx calls, and pools of them used for parallel invocations. This has the benefit that a lot of statx calls would have minimal allocation only for the fields that they need.
|
|
||
| static value get_file_type_variant(struct statx *sb) { | ||
| int filetype = sb->stx_mode & S_IFMT; | ||
| if (filetype == S_IFREG) { |
There was a problem hiding this comment.
This probably should be a switch statement for efficiency.
|
I'd like to revise the interface to not allocate as much, before it goes into a release... |
CHANGES: - Add `statx` support (@patricoferris, @talex5, @avsm ocaml-multicore/ocaml-uring#95 ocaml-multicore/ocaml-uring#97). - Add type annotations to tests (@patricoferris ocaml-multicore/ocaml-uring#96, reviewed by @talex5). Fixes MDX tests on OCaml 5.1. - Update liburing to 2.4 (@anmonteiro ocaml-multicore/ocaml-uring#93, reviewed by @talex5). - Fix accidental shadowing in `ocaml_uring_get_msghdr_fds` (@talex5 ocaml-multicore/ocaml-uring#91, reviewed by @avsm).
CHANGES: - Add `statx` support (@patricoferris, @talex5, @avsm ocaml-multicore/ocaml-uring#95 ocaml-multicore/ocaml-uring#97). - Add type annotations to tests (@patricoferris ocaml-multicore/ocaml-uring#96, reviewed by @talex5). Fixes MDX tests on OCaml 5.1. - Update liburing to 2.4 (@anmonteiro ocaml-multicore/ocaml-uring#93, reviewed by @talex5). - Fix accidental shadowing in `ocaml_uring_get_msghdr_fds` (@talex5 ocaml-multicore/ocaml-uring#91, reviewed by @avsm).
3 participants
Fixes #83.
I think we're not allowed to do any "OCaml work" in the C stubs for submitting, right now I've used (abused) the
Open_howstruct to hold onto the path and the flags for the statx call, I should probably create another C allocated thing for that? Or are we allowed to have allocating submissions (by this I mean a C stub that can callCAMLParamand friends?).The stat record is mostly the same as
Eio.File.statbut with a few extra fields for statx.Quite a few stat fields are int32s -- should we be using
Optint.tfor that (I'm not sure what the 32-bit C code would look like for that presumably justcaml_copy_int32) ? Maybe it isn't worth it.