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.

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

use the new std::io::{Error, Result} #14

Merged
merged 3 commits into from
Feb 16, 2015
Merged

Conversation

blaenk
Copy link
Contributor

@blaenk blaenk commented Feb 15, 2015

So on line 123, there's no longer an EndOfFile variant of ErrorKind. The calls to read now return a 0 in the case of EOF (see docs). I wasn't sure what to do in the event of 0 in this case. I checked the man page and it said:

The behavior when the buffer given to read(2) is too small to return
       information about the next event depends on the kernel version: in
       kernels before 2.6.21, read(2) returns 0; since kernel 2.6.21,
       read(2) fails with the error EINVAL.  Specifying a buffer of size

           sizeof(struct inotify_event) + NAME_MAX + 1

       will be sufficient to read at least one event.

I'm not sure what an 'end of file' in the context of inotify is. Maybe we'll have to create our own enum variant to represent this. I return an empty slice for now.

@passcod
Copy link
Contributor

passcod commented Feb 15, 2015

Returning an empty slice is not ideal as we probably want to communicate that there's been some kind of error. Looking through the docs, ResourceUnavailable is probably best:

The operation temporarily failed (for example, because a signal was received), and retrying may succeed.

Especially given this passage in the description of read() in the Rust docs:

If n is 0, then it can indicate one of two scenarios:

  • This reader has reached its "end of file" and will likely no longer be able to produce bytes. Note that this does not mean that the reader will always no longer be able to produce bytes.
  • The buffer specified was 0 bytes in length.

(bold mine, emphasis theirs)

@blaenk
Copy link
Contributor Author

blaenk commented Feb 15, 2015

Sure, that was my point, that you guys would probably know best how to handle it. Do whatever you think is best.

@blaenk
Copy link
Contributor Author

blaenk commented Feb 15, 2015

Actually I don't think read will ever return 0 on versions after 2.6.21, based on my cursory research. It only returned 0 in versions prior to that one, and only when the buffer given to read was smaller than needed to write the next event's data. Since this library directly controls the buffer's size, this should never be a problem, as long as it's at least (as the man page says):

sizeof(struct inotify_event) + NAME_MAX + 1

NAME_MAX is 255 and we can get inotify_event's size using ::std::mem::size_of::<ffi::inotify_event>(), although I don't know if that struct should be defined with #[repr(C)] for it to be accurate. However, given that we're currently using a buffer of 1024 bytes and the inotify_event struct is something like 16 bytes + 255 + 1, I'd say we're on the safe side. In fact, inotify will fit as many events as it can in the buffer, which we're correctly handling right now.

In any case, if read returns 0 it's not possible to recover unless the buffer is grown, so it's not a matter of just trying again later by attempting to read again into the same buffer. This isn't the same thing as being interrupted by a signal, which would instead yield EINTR.

The emphasized text in the docs for read is general; they don't take into account the semantics of inotify, or rather, they aren't making any guarantees about anything.

I'm changing the code to instead return an io::Error with an InvalidInput kind and a description of "buffer passed to read() was too small to read event" or something. It should never happen, since the library has control of that, but we might as well yield it I guess.

@hannobraun
Copy link
Owner

Thanks for the contribution! This looks good, I'm going to merge.

Regarding #[repr(C)]: I think that it's currently missing should be considered a bug. I'm going to fix this in a moment.

I'm changing the code to instead return an io::Error with an InvalidInput kind and a description of "buffer passed to read() was too small to read event" or something. It should never happen, since the library has control of that, but we might as well yield it I guess.

I'm not too sure about that detail. If it can never happen (except due to a bug in inotify-rs, which the user has no control over), then I think a panic! with a good description would be preferable.

I'm going to open an issue about that for now. I'm going to look into it when I have a bit more time. Or, if you're confident that the behaviour is indeed as you described, feel free to open a new pull request for that, if you want.

hannobraun added a commit that referenced this pull request Feb 16, 2015
use the new std::io::{Error, Result}
@hannobraun hannobraun merged commit 5fb0bf0 into hannobraun:master Feb 16, 2015
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.

3 participants