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.

Sign up for GitHub

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

Jump to bottom

mirage-kv 2.0.0 #65

Merged
merged 4 commits into from
Apr 8, 2019
Merged

mirage-kv 2.0.0 #65

merged 4 commits into from
Apr 8, 2019

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Mar 2, 2019

this upgrades tar-mirage to the mirage-kv 2.0.0 API :D

@hannesm
Copy link
Member Author

hannesm commented Mar 2, 2019 •
edited
Loading

while doing this port, I noticed that this is unlikely to work on Solo5, where block_read expects at most one sector to be read -- while in tar-mirage, BLOCK.read gets a single huge buffer passed. do other block backends (unix/xen) have the same semantics as solo5-block? .oO(feels like a revision of mirage-block needs to be done /cc @g2p @samoht @mato)

I also noticed that while sector_size is available, there are various instances of 4095 and 4096 hardcoded in tar-mirage, also 512. in addition, the code looks a bit suspicious in terms of integer overflows (+ and add are used without checking, lots ot Int64.to_int without checks)

@hannesm hannesm mentioned this pull request Mar 2, 2019
Closed
13 tasks
@hannesm hannesm requested review from avsm and djs55 March 2, 2019 16:09
jberdine pushed a commit to jberdine/opam-repository that referenced this pull request Mar 2, 2019
@avsm
[new release] tar-mirage, tar and tar-unix (1.0.0)
f31e2ab 
CHANGES:

- port build to dune from builder (@avsm)
- upgrade opam metadata to 2.0 (@avsm)
- remove topkg in favour of dune-release (@avsm)
- use modern `ppx_cstruct` instead of `cstruct.ppx` (mirage/ocaml-tar#65 @avsm @djs55)
- test with OCaml 4.07 as well (@avsm)
avsm
avsm reviewed Mar 3, 2019
View reviewed changes
mirage/tar_mirage.ml
| Ok data -> Ok (Digest.string data)

(* Compare filenames without a leading / or ./ *)
let trim_slash x =
Copy link
Member

Choose a reason for hiding this comment

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

Is this function something that can be replaced with Fpath.normalize use?

Copy link
Member Author

Choose a reason for hiding this comment

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

We're using Mirage_kv.Key, where I can't find such a function. But maybe worth an addition!

@mato
Copy link

mato commented Mar 4, 2019

feels like a revision of mirage-block needs to be done /cc @g2p @samoht @mato

See Solo5/solo5#325 (comment) for what needs to be done in Solo5 to support block operations in multiples of the block size.

hannesm referenced this pull request in mirage/docs Mar 20, 2019
@avsm
remove tar-mirage as it conflicts with new kv-lwt
34c4317
@hannesm
Copy link
Member Author

hannesm commented Mar 20, 2019

ok, thanks. the block read/write did not change in this PR, and should be addressed in other PRs. IMHO this is ready to be merged + released to unbreak tar-mirage with mirage 3.5.0 (which uses mirage-kv 2.0.0). any concerns about merging this PR?

@samoht
Copy link
Member

samoht commented Mar 20, 2019

LGTM

@hannesm
Copy link
Member Author

hannesm commented Mar 29, 2019

how can we move forward here? tar-mirage is atm not compatible with mirage 3.5.0 due to the changes in mirage-kv. IMHO it'd be great to have this merged and released //cc @djs55

@samoht samoht merged commit 8d76b56 into mirage:master Apr 8, 2019
@djs55
Copy link
Member

djs55 commented Apr 9, 2019

@samoht thanks for taking care of this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@avsm avsm avsm left review comments

@djs55 djs55 Awaiting requested review from djs55

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hannesm @mato @samoht @djs55 @avsm