Skip to content

Conversation

last-genius
Copy link

Adds the partial directory call to client_unix. Server implementation is available in xapi-project/oxenstored#2, so wasn't attempted here.

Also removes the deprecated Restrict call, and brings some minor improvements elsewhere.

Thanks to @freddy77 for working on this.

Changes were tested with the server implementation above, with the client coping well with large directories without having to utilize larger packets.

@last-genius
Copy link
Author

The CI will fail for OCaml <4.13 because some newer Stdlib functions are used, do we intend to support these?

@edwintorok
Copy link
Collaborator

edwintorok commented Nov 15, 2024

We previously discussed that the minimum OCaml version could be bumped. I'd suggest doing that in a PR of its own and checking the Ci passes there. 4.13 is probably ok mirage/ocaml-vhd#78 (comment)

(* Node's children changed, restart *)
if data <> "" && new_generation <> generation then read_part "" ""
else
let data = data ^ new_data in
Copy link

Choose a reason for hiding this comment

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

For efficiency it would be better to collect all strings in a list and do a String.concat or to use a Buffer value.

Copy link
Author

Choose a reason for hiding this comment

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

Switched to using a Buffer in the fixup commit.

@last-genius last-genius force-pushed the directory_part branch 2 times, most recently from 01b9eb8 to 8412de5 Compare November 18, 2024 12:40
@last-genius
Copy link
Author

Somehow missed that Frediano's tailcall assertions aren't actually tailcalls, so the CI fails:

{11E395A2-0042-446A-8B0F-3F284CDE948D}

Dropping this commit altogether

@last-genius
Copy link
Author

I've addressed the comments, and tested this. If the fixup looks good, I can squash it and we can merge this.

@last-genius
Copy link
Author

@psafont would you mind merging this?

Andrii Sultanov and others added 5 commits December 3, 2024 13:03
Signed-off-by: Andrii Sultanov <andrii.sultanov@cloud.com>
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
Storage should not order the entries, check it adding in not
alphabetical order

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
Since call IDs are encoded in C as enum variants, the removal of
Restrict created a hole that should be treated as an error. Oxenstored
treats it as an Invalid variant, so follow that practice here as well.

Signed-off-by: Andrii Sultanov <andrii.sultanov@cloud.com>
Also add Reset_watches call that has been added upstream - currently
unimplemented. It needs to be present so as not to leave a hole in call IDs.

Server implementation is a noop as well.

Co-authored-by: Andrii Sultanov <andrii.sultanov@cloud.com>
Signed-off-by: Andrii Sultanov <andrii.sultanov@cloud.com>
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@psafont psafont merged commit ba66bbb into mirage:main Dec 3, 2024
1 check passed
psafont added a commit to psafont/opam-repository that referenced this pull request May 28, 2025
CHANGES:

* minor performance fixes (mirage/ocaml-xenstore#53 by freddy77)
* use dune 2.0 (mirage/ocaml-xenstore#54 by lindig)
* require OCaml 4.13 (mirage/ocaml-xenstore#56 by last-genius)
* Add support `XS_DIRECTORY_PART` to the client (mirage/ocaml-xenstore#55 by last-genius)
* Add x-maintenance-intent to opam  metadata (mirage/ocaml-xenstore#57 by hannesm)
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.

5 participants