Re: [PATCH v7 07/10] fsmonitor: implement filesystem change listener for Linux
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Mar 4, 2026, 07:43 UTC
- Message-ID
- <aafii97SoHS96rkA@pks.im>
- In-Reply-To
- <adac7964cce28d237ef09574858be240fe3269fc.1772065643.git.gitgitgadget@gmail.com>
On Thu, Feb 26, 2026 at 12:27:20AM +0000, Paul Tarjan via GitGitGadget wrote:
Show 13 quoted lines
> From: Paul Tarjan <github@paulisageek.com> > > Implement the built-in fsmonitor daemon for Linux using the inotify > API, bringing it to feature parity with the existing Windows and macOS > implementations. > > The implementation uses inotify rather than fanotify because fanotify > requires either CAP_SYS_ADMIN or CAP_PERFMON capabilities, making it > unsuitable for an unprivileged user-space daemon. While inotify has > the limitation of requiring a separate watch on every directory (unlike > macOS's FSEvents, which can monitor an entire directory tree with a > single watch), it operates without elevated privileges and provides > the per-file event granularity needed for fsmonitor.
Thanks for adding this explanation, makes sense.
Show 15 quoted lines
> diff --git a/compat/fsmonitor/fsm-path-utils-linux.c b/compat/fsmonitor/fsm-path-utils-linux.c > new file mode 100644 > index 0000000000..b4c19e0655 > --- /dev/null > +++ b/compat/fsmonitor/fsm-path-utils-linux.c > @@ -0,0 +1,220 @@ > +#include "git-compat-util.h" > +#include "fsmonitor-ll.h" > +#include "fsmonitor-path-utils.h" > +#include "gettext.h" > +#include "trace.h" > + > +#include <sys/statfs.h> > + > +#ifdef HAVE_LINUX_MAGIC_H
I saw that this define is only wired up for CMake. I guess we should also add it to our Makefile (probably via config.mak.uname) and Meson (probably via compiler.has_header()).
[snip]
Show 26 quoted lines
> +/*
> + * Get the filesystem type name for logging purposes.
> + */
> +static const char *get_fs_typename(unsigned long f_type)
> +{
> + switch (f_type) {
> + case CIFS_SUPER_MAGIC:
> + return "cifs";
> + case SMB_SUPER_MAGIC:
> + return "smb";
> + case SMB2_SUPER_MAGIC:
> + return "smb2";
> + case NFS_SUPER_MAGIC:
> + return "nfs";
> + case AFS_SUPER_MAGIC:
> + return "afs";
> + case CODA_SUPER_MAGIC:
> + return "coda";
> + case V9FS_MAGIC:
> + return "9p";
> + case FUSE_SUPER_MAGIC:
> + return "fuse";
> + default:
> + return "unknown";
> + }
> +}This selection looks rather interesting to me. Why wouldn't we include common filesystems like ext4 and the like? Certainly hints that the function needs better documentation, and potentially a better name.
Show 70 quoted lines
> +/*
> + * Find the mount point for a given path by reading /proc/mounts.
> + * Returns the filesystem type for the longest matching mount point.
> + */
> +static char *find_mount(const char *path, struct statfs *fs)
> +{
> + FILE *fp;
> + struct strbuf line = STRBUF_INIT;
> + struct strbuf match = STRBUF_INIT;
> + struct strbuf fstype = STRBUF_INIT;
> + char *result = NULL;
> + struct statfs path_fs;
> +
> + if (statfs(path, &path_fs) < 0)
> + return NULL;
> +
> + fp = fopen("/proc/mounts", "r");
> + if (!fp)
> + return NULL;
> +
> + while (strbuf_getline(&line, fp) != EOF) {
> + char *fields[6];
> + char *p = line.buf;
> + int i;
> +
> + /* Parse mount entry: device mountpoint fstype options dump pass */
> + for (i = 0; i < 6 && p; i++) {
> + fields[i] = p;
> + p = strchr(p, ' ');
> + if (p)
> + *p++ = '\0';
> + }
> +
> + if (i >= 3) {
> + const char *mountpoint = fields[1];
> + const char *type = fields[2];
> + struct statfs mount_fs;
> +
> + /* Check if this mount point is a prefix of our path */
> + if (starts_with(path, mountpoint) &&
> + (path[strlen(mountpoint)] == '/' ||
> + path[strlen(mountpoint)] == '\0')) {
> + /* Check if filesystem ID matches */
> + if (statfs(mountpoint, &mount_fs) == 0 &&
> + !memcmp(&mount_fs.f_fsid, &path_fs.f_fsid,
> + sizeof(mount_fs.f_fsid))) {
> + /* Keep the longest matching mount point */
> + if (strlen(mountpoint) > match.len) {
> + strbuf_reset(&match);
> + strbuf_addstr(&match, mountpoint);
> + strbuf_reset(&fstype);
> + strbuf_addstr(&fstype, type);
> + *fs = mount_fs;
> + }
> + }
> + }
> + }
> + }
> +
> + fclose(fp);
> + strbuf_release(&line);
> + strbuf_release(&match);
> +
> + if (fstype.len)
> + result = strbuf_detach(&fstype, NULL);
> + else
> + strbuf_release(&fstype);
> +
> + return result;
> +}Sorry, but I still don't quite understand what we're doing here. Isn't the longest matching mount point always the one that statfs(3p) gave us? Why do we have to scan "/proc/mounts"?
Patrick