-
Notifications
You must be signed in to change notification settings - Fork 85
feat(fs): add splice #608
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
base: master
Are you sure you want to change the base?
feat(fs): add splice #608
Conversation
compio-fs/src/pipe.rs
Outdated
|
|
||
| #[cfg(linux_all)] | ||
| impl<TIn: AsFd + 'static, TOut: AsFd + 'static> IntoFuture for SpliceBuilder<TIn, TOut> { | ||
| type IntoFuture = Pin<Box<dyn Future<Output = Self::Output>>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use Box. It's OK to add a new async fn to do so. Actually I guess a simple splice method is OK instead of a builder...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's OK to keep the builder. Just don't Box please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh so I need to add a build() method to create the Splice, do we want to keep that API? or should I make a simple splice method that receives all arguments instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I have exposed the future type for submit in #614. You can rebase and use that directly so no need for the Box.
compio-fs/src/pipe.rs
Outdated
| self.offset_out, | ||
| self.len, | ||
| ) | ||
| .flags(self.flags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another problem is that, it's OK to assume nonblocking everywhere on polling driver. Therefore, we should wait for the readiness of the fds, either in the driver, or here with PollFd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same comment as #609 (comment), right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I made it to check the in fd in the driver, but the out fd has to be polled manually. The flags should also be modified to use the nonblocking IO. Please check the driver type at runtime.
|
Could you rebase this PR? |
|
rebased |
Berrysoft
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the rebase. I think there are some existing issues to be solved.
6c82d3e to
d81b1c1
Compare
|
yeah I only rebased :) |
b785541 to
f18ba9d
Compare
|
I modified the driver to include |
built on #609
Usage (file -> socket example)