Unify inotify public interface across backends and accept impl AsFd#1105
Unify inotify public interface across backends and accept impl AsFd#1105sunfishcode merged 2 commits intobytecodealliance:mainfrom
impl AsFd#1105Conversation
SUPERCILEX
changed the title
impl AsFd
| #[inline] | ||
| pub fn inotify_add_watch<P: crate::path::Arg>( | ||
| inot: BorrowedFd<'_>, | ||
| pub fn inotify_add_watch<Fd: AsFd, P: crate::path::Arg>( |
There was a problem hiding this comment.
Actually I think adding a generic isn't backwards compatible if someone explicitly specified P. Then again, I don't think anybody uses this code? https://github.com/search?q=inotify_add_watch+language%3ARust+rustix+-is%3Afork&type=code At least not publicly.
There was a problem hiding this comment.
Would it by chance work to use <P: crate::path::Arg, Fd: AsFd>?
There was a problem hiding this comment.
I think it would complain with a missing generic and you'd have to put an underscore. Maybe we could do impl AsFd and put a todo to make it a generic later?
There was a problem hiding this comment.
Making it impl AsFd and adding a TODO sounds reasonable to me.
There was a problem hiding this comment.
Sounds good, done!
sunfishcode
left a comment
sunfishcode
left a comment
There was a problem hiding this comment.
Looks good, just a few minor details:
| /// application should keep track of this externally to avoid logic errors. | ||
| #[inline] | ||
| pub fn inotify_add_watch<P: crate::path::Arg>( | ||
| inot: impl AsFd, |
There was a problem hiding this comment.
Could you add the TODO comments about changing this impl AsFd to a generic argument in the future?
There was a problem hiding this comment.
See #753 (comment). I feel like it'll be more effective to run a global pass on the whole lib rather than trying to remember the spots impls need to be fixed. If you disagree, I'll be happy to add the TODOs here.
| /// descriptor from being implicitly passed across `exec` boundaries. | ||
| #[doc(alias = "inotify_init1")] | ||
| pub fn inotify_init(flags: CreateFlags) -> io::Result<OwnedFd> { | ||
| // SAFETY: `inotify_init1` has no safety preconditions. |
There was a problem hiding this comment.
Could you restore these three SAFETY comments? I know they're not super interesting, but they're there for the benefit of people auditing the code. They record what we believed the relevant safety considerations are, so that if it ever turns out that we missed something, it's easier to understand the intent of the code.
There was a problem hiding this comment.
Makes sense, restored.
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
|
This is released in rustix 0.38.35. |
2 participants
It currently only accepts a
BorrowedFdwhich is annoying. Pretty sure this is a backwards compatible change.