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
Fanotify Support #542
base: main
Are you sure you want to change the base?
Fanotify Support #542
Conversation
- merge fanotify Listener -> Watcher - Add NewFanotifyWatcher() API - Rename Start() -> start() and invoke it from NewFanotifyWatcher - Make methods of Listener to Watcher - Rename fanotify readEvents to readFanotifyEvents
- Move Stop() functionality to Close() - Make FanotifyEvent is subtype of Event - Convert EventType to Op
- Add AddMount, RemoveMount methods to watch and unwatch entire mount point - Add AddPermissions, Allow, Deny methods to setup Permission requests and responses
I don't think that any fanotify support should add new symbols or features, other than a setting to select that you want to use it. Beyond that, it should just provide the same feature set the existing backends do. Need to see what the best way would be to select the fanotify backend; for now I'd just make a Things like additional event types and such are not cross-platform. New event types like |
backend_inotify.go
Outdated
// fsnotify.PermissionToExecute Permission to open file for execution. (Applicable only to fanotify watcher.) | ||
// | ||
// fsnotify.PermissionToRead Permission to read a file or directory. (Applicable only to fanotify watcher.) | ||
PermissionEvents chan FanotifyEvent |
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.
I don't think we should have any references to fanotify in the backend_inotify.go file.
What I'd do for now is adjust the build tags on backend_inotify.go
so it never compiles and then implement a new Watcher
in backend_fanotify.go
. This way all the tests should run against the fanotify backend.
I need to think a bit on how to best architecture things on this so you can tell you prefer to use fanotify; my idea was that you can select the backend per-watch.
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.
Sounds good. A step closer. Will update back soon.
Thanks for the comments and information regarding the issues. Regarding #521 |
My idea was that you can set the backend per-watch. This will be useful especially with the poll watcher: for example if you want to watch
This isn't really something we can easily support in fsnotify in the first place, since the goal is to make all features work well on all supported platforms; it's a bit of a "lowest common denominator" thing. The downside is that you can't use these kind of (often useful) features, but the upside is you can write your code on your Linux laptop and be reasonably confident it will also work well for people on macOS, OpenBSD, Windows, etc. I'm not against adding support for this kind of thing, somehow, because in a number of use cases you just don't care about cross-platform support and obviously these kind of things are useful. But need to think about how to do this in a way that's 1) not ugly, 2) doesn't lead to people accidentally writing applications that will only work correct on fanotify, and 3) is compatible (so we don't need to make a v2). I'm not yet 100% sure how to best do that. But in general, fsnotify won't be as "fully featured" like the fanotify Go library you wrote because the cross-platform support makes everything a lot harder. |
The path forward would be something like:
Also, worth pointing out I'm working on adding recursive watch support for inotify (and the other backends): using Come to think of it, we can perhaps use "watch entire mount point" as an optimisation if you do |
Sounds great. I'll then proceed with the (2) one for now. I think I'll be able to disable
I can try and explore on an idea to make the Fanotify Watcher a composition of multiple fanotify watcher types one for each (paths, permissions, mount point) that way the composite watcher is the same while the user can still call |
Personally I'd just keep things as simple/minimal as possible for the initial version, and add additional things later on. It doesn't all have to be in one PR, and just getting the initial implementation right will already be plenty of work. I'd be fine just not doing recursion at all for the initial version and adding that later on. But it's your PR so you can decide. |
Yep I agree. I wasn't planning on making those changes here :). As a first step making Fanotify and Inotify functionally equivalent is good. |
- Disable Inotify - Split fanotify
- Add Add(), AddWith() methods - Add Remove() method - Disable backend_inotify_test.go - Tests are still breaking due to - missing fields in Watcher (Errors, watches etc.)
- Add logic to close watcher once only. - Fix tests + add fanotify specific output
@arp242 with the recent commits I've addressed -
NOTE - I had to introduce a temporary flag I am seeing a couple of problems -
|
How do you run the tests? I tried:
I added a bit more context with the below patch, and it seems to fail on the
|
I usually ran with
|
The
|
Are you still interested in working on this? |
BTW what this needs is to be reworked to be much simpler, not adding any new features and things like that. Basically something like the below patch, except, well, working, passing all the tests, etc. – this is just something I cooked up quickly. Adding new event types, mountpoint watching, and things like that are all out of scope here. Also just get it running on the latest Linux; don't worry about supporting older Linux systems (there's quite a lot of code for this).
|
Initial work on adding fanotify support. Much of the code is from https://github.com/opcoder0/fanotify.
These are some of the things that are on my to do list -
unix.FAN_UNLIMITED_QUEUE
unix.FAN_UNLIMITED_MARKS
unix.FAN_REPORT_TID
unix.FAN_ENABLE_AUDIT
getFileHandle
andgetFileHandleWithName
.CAP_SYS_ADMIN
).In addition to any other comments, I would appreciate feedback on the APIs as I am not sure how to maintain the API compatability with inotify and fanotify.
Closes #114