search

cberner / fuser

Filesystem in Userspace (FUSE) for Rust
MIT License
836 stars 114 forks source link

Sans-IO Reply type: `ll::Response` #131

Closed wmanley closed 3 years ago

wmanley commented 3 years ago

Raising for early visibility.

Should help reduce duplication between sync and async APIs, e.g. with #130.

TODO

wmanley commented 3 years ago

@cberner: I now consider this ready for review. I've not yet deduplicated async_api/reply.rs as I have a more extensive deduplication planned for a future PR and I don't want to duplicate that work.

cberner commented 3 years ago

Great! I'm in the process of reverting the async changes, since #133 is abandoned, but then will take a look at this

wmanley commented 3 years ago

Started going through the commits one at a time, since there are so many.

Yeah, there's a lot. I tried to break them up to make it easier to review/bisect.

Also, mind rebasing?

Done

cberner commented 3 years ago

Started going through the commits one at a time, since there are so many.

Yeah, there's a lot. I tried to break them up to make it easier to review/bisect.

Yep, definitely appreciated! It's much easier to review this way

cberner commented 3 years ago

@wmanley I think I'm missing some context. There are a bunch of new .reply() methods on the types in ll::request, but they're all marked as dead code. What are they intended for?

wmanley commented 3 years ago

Noticed one typo. Otherwise the commits up through "ReplyRaw: Check for integer overflow" look good.

I've raised #137 with the commits up till there so as to allow merging incrementally. I've fixed the typo there. I'll rebase this PR once that's merged.

wmanley commented 3 years ago

@wmanley I think I'm missing some context. There are a bunch of new .reply() methods on the types in ll::request, but they're all marked as dead code. What are they intended for?

Yes indeed, they are not used yet. With this PR and #119 I've been working towards creating #136 - an enum based API which I believe offers significant advantages as compared to a trait based one. You can get a feel for how the .reply() will be used looking here: https://github.com/cberner/fuser/pull/136/commits/f05d437f5b3c5aa05ccfb821ad7787401549e8a8?diff=unified&w=1 (view hiding whitespace changes to get a better feel for it). Basically: users can't create Responses directly because the Response::new_* methods are pub(crate). They instead create them by calling .reply() on the operation they've been passed. op.reply will always return a Response of the correct type for that Request type.

reply isn't used in this PR, but it seemed natural to include it here as I was working through each reply type.

cberner commented 3 years ago

Ah, I see. Cool, I'll take a look at that. I merged #137 :)

wmanley commented 3 years ago

Rebased since #137 is merged

wmanley commented 3 years ago

I just fixed some clippy lints and have rebased them into the relevant commits. No exciting changes made.

cberner commented 3 years ago

Ok, now that I understand how #136 works I think I see what these are for. I'd like to merge this work in a way that doesn't involve adding dead code though. Is there an obvious way to break it up into medium sized PRs that add end-to-end functionality?

wmanley commented 3 years ago

Ok, now that I understand how #136 works I think I see what these are for. I'd like to merge this work in a way that doesn't involve adding dead code though. Is there an obvious way to break it up into medium sized PRs that add end-to-end functionality?

Unfortunately I don't know how to do this. I'm trying to gradually work towards #136, but until we make these types public there is going to be code in this crate that is unused, because it's not yet callable from outside the crate. I see the #[allow(dead_code)] as an advantage really - if this work does come to naught then it's easy to find the code that can be deleted by grepping for that.

Two alternatives:

I think it's a bit premature for the former, and the latter seems like it would be hard to review.

cberner commented 3 years ago

I see, ok I'll review this as-is then! Should have time this weekend. Maybe sooner.

One thing, I may end up asking for changes to these pieces, when we get to the future PRs and I see how everything fits together. Just fair warning :)

wmanley commented 3 years ago

I see, ok I'll review this as-is then! Should have time this weekend. Maybe sooner.

Thanks

One thing, I may end up asking for changes to these pieces, when we get to the future PRs and I see how everything fits together. Just fair warning :)

Yeah, I'm expecting the API will continue to evolve before it's ready to be made public. Porting simple.rs to this API has been really helpful in identifying areas where the API could be better, and when I port ostree-fuse to this API I'm expecting to learn more about how the API could be improved.

cberner commented 3 years ago

Merged. Thanks!