`Eq` implementation of `WatchDescriptor` is wrong

[hannobraun]

`Eq` implementation of `WatchDescriptor` is wrong

[arsalan86]

I have tested this by creating multiple Inotify instances and matching the WatchDescriptors they generate.

assert_eq!(wd_from_inotify1, wd_from_inotify2); passes when the contained c_int is the same for both WatchDescriptorss. This should not be the case.

While the Eq trait does seem useful within the scope of an Inotify instance, the potential to match across instances can break things.

I don't think removing the Eq trait is the right way to address this issue. Perhaps it makes more sense for the Inotify struct to generate an identifier unique to itself. This can be passed to generated WatchDescriptors as a field. This would retain the usability of the Eq trait within the scope of the parent Inotify, and protect against matching WatchDescriptors between instances of Inotify

[hannobraun]

I think the most straight-forward way to address this would be to add the inotify instance's file descriptor to WatchDescriptor. That would do nothing to solve #61, but at least the Eq implementation would be working correctly.

[arsalan86]

Yes, that does seem to be the most practical way of fixing the Eq implementation. I've implemented a field of type RawFd in the WatchDescriptor struct in my working tree.

I'll do a PR once I've figured out what can be done about #61. My tree doesn't build at the moment.

[hannobraun]

If you make a pull request for adding the RawFd, I'd definitely accept it. Yes, eventually we want a complete solution that also solves #61, but I suspect that will require more thinking and therefore more time. Until then, fixing the Eq issue in a simple way will be very welcome.

[arsalan86]

Adding the RawFd field to WatchDescriptor introduces another issue: Inotify[1] itself doesn't associate watch descriptors and events with a particular Inotify instance.

From the Inotify source (accessed from here):

pad_name_len = round_event_name_len(fsn_event);
inotify_event.len = pad_name_len;
inotify_event.mask = inotify_mask_to_arg(fsn_event->mask);
inotify_event.wd = event->wd;
inotify_event.cookie = event->sync_cookie;

Implementing a mechanism to prevent WatchDescriptors from being shared between Inotify[2] instances (by adding a RawFd field to the WatchDescriptor struct) is possible, but it requires changes to inotify-rs such that the Inotify and Event structs will need to handle checking WatchDescriptor instances for RawFd. For instance, the add_watch method would then need to return a WatchDescriptor with a RawFd field assigned self.0,

match wd {
    -1 => Err(io::Error::last_os_error()),
    _  => Ok(WatchDescriptor(wd, self.0)),
}

and the rm_watch method would need to check this against the RawFd field of the parent Inotify instance. This adds extraneous functionality to the library over and above the scope of the original Inotify library, since Inotify doesn't include information about the originating Inotify instance with watch descriptors.

This is a major change that affects how the library handles events and watch descriptors, and would also be a breaking change.

[1]Inotify denotes the inotify.c library
[2]Inotify denotes the Inotify struct in inotify-rs and is formatted as inline code to make it distinct from Inotify above

[hannobraun]

This is a major change that affects how the library handles events and watch descriptors, and would also be a breaking change.

I think the additional overhead on top of inotify is warranted in this case. I believe it is reasonable to expect an idiomatic Rust wrapper to prevent misuse of the underlying API, as far as that is practical.

Breaking changes are acceptable at this stage. That's why the library is still at an 0.x version.

[arsalan86]

If there’s going to be breaking changes, why not use the opportunity to provide an easier and more manageable interface to the user? I think making use of a hashmap for indexing watch descriptors is a good way forward: The `add_watch()` method creates a HashMap<key, watchdescriptor> and returns the key. The `rm_watch()` method takes this key and looks it up in the HashMap. Events are also returned with the same key to link them to HashMaps. The keys could be unique (how?) to prevent contamination between instances of `Inotify`. Alternatively, the HashMap itself could be passed to the user as Inotify.HashMap. I’m just thinking out loud right now. I’m going to try something and push a branch if it seems to work.

[hannobraun]

I'm don't understand what the benefit of this would be. Why give the user a key to refer to the WatchDescriptor instead of just giving them the WatchDescriptor directly? What is the purpose of the HashMap?

[arsalan86]

I'm sorry about that momentary lapse of reason. Turns out I was doing something very wrong.

I'm making a PR for this bugfix.

[arsalan86]

I believe this should be closed.

[hannobraun]

I think this should stay open. While #67 addressed the main defect, I thought of another case where the Eq implementation could be wrong: If the inotify file descriptor is being reused, and an old WatchDescriptor is still around.

Usually, this shouldn't be a problem. You can just create one Inotify instance and use that forever. However, people might use the library differently (see #68, where a user talks about one Inotify instance per watched directory). A long-running process that watches many directories and opens/closes many files might run into this bug sooner or later.

I have an idea on how to address this (warning, pure idea, not tested in any way): Convert the fd in Inotify from a RawFd to a Rc<RawFd>. Convert fd in WatchDescriptor to a Weak<RawFd> that points to the same RawFd. Methods that handle WatchDescriptor would still need to compare the two RawFds, however, by accessing the WatchDescriptors's RawFd through the Weak pointer, they would make sure the original RawFd was never dropped.

I think this would also solve other issues mentioned in #61. However, it would come with additional cost, most notably one heap allocation per Inotify instance. This sounds acceptable to me, but maybe we should ask the notify developers for feedback before merging anything like this. On the other hand, no one ever complained about the heap-allocated buffer in Inotify when that was still a thing.

[hannobraun]

I did a bit of research. It seems that Linux immediately re-assigns the numbers from closed file descriptors. I've tested with this piece of code:

let inotify_1 = Inotify::init().unwrap();
print!("{}\n", inotify_1.as_raw_fd());
inotify_1.close().unwrap();
let inotify_2 = Inotify::init().unwrap();
print!("{}\n", inotify_2.as_raw_fd());

It prints the same number twice.

This makes it somewhat likely to run into this issue, when handling multiple Inotify instances. Knowing that, I think the overhead of the solution I outlined above is clearly worth it (altough I still haven't looked at the solution any closer, so I don't know if it will work out).