fs: implement inotify(2) - #1440
Open
gburd wants to merge 2 commits into
Open
Conversation
inotify was stubbed (inotify_init returned EMFILE, add_watch/rm_watch returned EINVAL), so programs that watch the filesystem for changes could not run. Implement it against OSv's VFS. All path mutations route through a handful of central functions in fs/vfs/vfs_syscalls.cc; those now call osv_inotify_notify() with the absolute path affected and the event mask. libc/inotify.cc keeps a registry of inotify instances (each a pollable special_file, modeled on eventfd), matches every event against their watches, and queues a struct inotify_event for the fd, waking any poller. - inotify_init/inotify_init1 create the fd (IN_CLOEXEC/IN_NONBLOCK honored). - inotify_add_watch resolves the path to absolute, adds (or merges, with IN_MASK_ADD) a watch, and returns a watch descriptor. - inotify_rm_watch removes a watch and queues IN_IGNORED. - read() returns as many packed inotify_event structures as fit in the buffer (blocking, or EAGAIN when non-blocking), rejecting a too-small buffer with EINVAL; poll() reports POLLIN when events are queued. A directory watch fires for changes to entries within it (reporting the entry name); a watch on the object itself fires with no name. Events wired: IN_CREATE (mkdir, open O_CREAT), IN_DELETE (rmdir, unlink), IN_MOVED_FROM / IN_MOVED_TO (rename), each with IN_ISDIR when the object is a directory. The rename hook rebuilds the full destination path because sys_rename truncates `dest` to its parent in place before the VOP. Not yet covered (follow-ups): IN_MODIFY/IN_CLOSE_WRITE on writes (needs an fd-to-path lookup), IN_ACCESS/IN_OPEN, and recursive watches. Add tests/tst-inotify.cc covering create/delete/move on files and a subdirectory (names and IN_ISDIR), IN_IGNORED on watch removal, and the EAGAIN/EINVAL paths. Passes on OSv under KVM; tst-remove (unlink/rename/rmdir) continues to pass, so the VFS mutation paths are unaffected.
There was a problem hiding this comment.
🟡 Not ready to approve
There are build/ABI correctness issues (missing headers, move-cookie always 0) and test robustness/coverage gaps that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds a functional inotify(2) implementation to OSv by introducing an inotify-backed pollable fd in libc and wiring VFS mutation syscalls to emit inotify events, along with a new test to validate basic create/delete/move behavior.
Changes:
- Implement inotify fd/event queue + global registry and VFS notification entry point in
libc/inotify.cc. - Emit inotify notifications from core VFS mutation syscalls (create/mkdir/rmdir/unlink/rename) in
fs/vfs/vfs_syscalls.cc. - Add
tst-inotifyand hook it into the test module build.
File summaries
| File | Description |
|---|---|
| tests/tst-inotify.cc | Adds an integration test for inotify events and error paths. |
| modules/tests/Makefile | Adds tst-inotify.so to the test suite build. |
| libc/inotify.cc | Implements inotify fd, watch registry, event queuing, and VFS notification hook. |
| fs/vfs/vfs.h | Declares the osv_inotify_notify() VFS hook. |
| fs/vfs/vfs_syscalls.cc | Calls osv_inotify_notify() on successful path mutations. |
Review details
Suppressed comments (2)
tests/tst-inotify.cc:94
- The directory delete case doesn't assert rmdir() succeeded and doesn't check that the IN_DELETE event reports the entry name (it should for a directory watch). This weakens the coverage for the name-reporting behavior.
rmdir(sub.c_str());
mask = read_event(ifd, name);
assert((mask & IN_DELETE) && (mask & IN_ISDIR));
tests/tst-inotify.cc:99
- The test claims to cover IN_IGNORED on watch removal, but it never reads/asserts the queued IN_IGNORED event after inotify_rm_watch(). Drain and verify it so regressions are caught.
// Removing the watch queues IN_IGNORED and rejects a bogus wd.
assert(inotify_rm_watch(ifd, wd) == 0);
errno = 0;
assert(inotify_rm_watch(ifd, 9999) == -1 && errno == EINVAL);
- Files reviewed: 5/5 changed files
- Comments generated: 7
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
+16
to
+20
| // A watch on a directory fires for changes to entries within it (with the entry | ||
| // name); a watch on a file fires for changes to the file itself. This covers | ||
| // the common events (IN_CREATE, IN_DELETE, IN_MODIFY, IN_MOVED_FROM/TO, | ||
| // IN_CLOSE_WRITE, IN_ATTRIB); the recursive-watch and mount-related events are | ||
| // out of scope. |
Comment on lines
+296
to
+301
| // Strip a trailing slash (except for root). | ||
| size_t n = strlen(abs); | ||
| while (n > 1 && abs[n - 1] == '/') { | ||
| abs[--n] = '\0'; | ||
| } | ||
| return in->add_watch(abs, mask); |
Comment on lines
+22
to
+28
| #include <sys/inotify.h> | ||
| #include <fs/fs.hh> | ||
| #include <libc/libc.hh> | ||
| #include <osv/fcntl.h> | ||
| #include <osv/mutex.h> | ||
| #include <osv/condvar.h> | ||
| #include <osv/poll.h> |
Comment on lines
+121
to
+125
| WITH_LOCK(_mutex) { | ||
| bool queued = false; | ||
| for (auto& w : _watches) { | ||
| if (!(w.mask & (mask & IN_ALL_EVENTS))) { | ||
| continue; |
Comment on lines
+106
to
+111
| auto* ev = reinterpret_cast<inotify_event*>(buf.data()); | ||
| ev->wd = wd; | ||
| ev->mask = mask; | ||
| ev->cookie = 0; | ||
| ev->len = namelen; | ||
| if (!name.empty()) { |
Comment on lines
+285
to
+295
| // Resolve to an absolute path so it matches what the VFS hook reports. | ||
| char abs[PATH_MAX]; | ||
| if (pathname[0] == '/') { | ||
| strlcpy(abs, pathname, sizeof(abs)); | ||
| } else { | ||
| char cwd[PATH_MAX]; | ||
| if (!getcwd(cwd, sizeof(cwd))) { | ||
| return libc_error(errno); | ||
| } | ||
| snprintf(abs, sizeof(abs), "%s/%s", cwd, pathname); | ||
| } |
Comment on lines
+50
to
+58
| // Work in a fresh writable directory under /tmp (ramfs). | ||
| const char* dir = "/tmp/inotify-test"; | ||
| mkdir(dir, 0777); | ||
|
|
||
| int ifd = inotify_init1(IN_NONBLOCK); | ||
| assert(ifd >= 0); | ||
|
|
||
| int wd = inotify_add_watch(ifd, dir, IN_CREATE | IN_DELETE | IN_MOVED_FROM | IN_MOVED_TO); | ||
| assert(wd >= 0); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
inotify was stubbed (
inotify_initreturnedEMFILE,add_watch/rm_watchreturned
EINVAL), so programs that watch the filesystem for changes could notrun.
How
Implemented against OSv's VFS. All path mutations route through a handful of
central functions in
fs/vfs/vfs_syscalls.cc; those now callosv_inotify_notify()with the absolute path affected and the event mask.libc/inotify.cckeeps a registry of inotify instances (each a pollablespecial_file, modeled on eventfd), matches every event against their watches,and queues a
struct inotify_eventfor the fd, waking any poller.inotify_init/inotify_init1create the fd (IN_CLOEXEC/IN_NONBLOCKhonored).
inotify_add_watchresolves the path to absolute, adds (or merges, withIN_MASK_ADD) a watch, and returns a watch descriptor.inotify_rm_watchremoves a watch and queuesIN_IGNORED.read()returns as many packedinotify_eventstructures as fit in thebuffer (blocking, or
EAGAINwhen non-blocking), rejecting a too-small bufferwith
EINVAL;poll()reportsPOLLINwhen events are queued.A directory watch fires for changes to entries within it (reporting the entry
name); a watch on the object itself fires with no name. Events wired:
IN_CREATE(mkdir, open O_CREAT),IN_DELETE(rmdir, unlink),IN_MOVED_FROM/IN_MOVED_TO(rename), each withIN_ISDIRwhen the object is a directory. Therename hook rebuilds the full destination path because
sys_renametruncatesdestto its parent in place before the VOP.Not yet covered (follow-ups):
IN_MODIFY/IN_CLOSE_WRITEon writes (needs anfd-to-path lookup),
IN_ACCESS/IN_OPEN, and recursive watches.Testing
tests/tst-inotify.cccovers create/delete/move on files and a subdirectory(names and
IN_ISDIR),IN_IGNOREDon watch removal, and theEAGAIN/EINVALpaths. Passes on OSv under KVM.
tst-remove(unlink/rename/rmdir) continues topass, so the VFS mutation paths are unaffected.
(Recreated from #1422, which GitHub auto-closed when its branch was rebased onto current master. Same change, rebased and verified on master 3aba46c.)