From: Paul Tarjan <paul@paultarjan.com>
Date: Wed, 04 Mar 2026 18:17:50 GMT
Subject: Re: [PATCH v7 07/10] fsmonitor: implement filesystem change listener for Linux
Message-ID: <20260304181750.25730-1-github@paulisageek.com>
In-Reply-To: <aafii97SoHS96rkA@pks.im>

On Tue, Mar 4, 2026, Patrick Steinhardt wrote:
> 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()).

It's already wired up in all three:

  - config.mak.uname line 78: BASIC_CFLAGS += -DHAVE_LINUX_MAGIC_H
  - meson.build: compiler.has_header('linux/magic.h') check
  - CMakeLists.txt: add_compile_definitions(HAVE_LINUX_MAGIC_H)

While looking at CMakeLists.txt I noticed the GIT-BUILD-OPTIONS
string replacements were hardcoded to "win32" for both
FSMONITOR_DAEMON_BACKEND and FSMONITOR_OS_SETTINGS. Fixed that to
use the CMake variables so Linux/macOS builds get the right values.

> 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.

It's only used as a fallback when /proc/mounts isn't available. Since
we only care about naming the remote/special filesystems that
is_remote_fs() flags as incompatible, there's no need for ext4 etc.
Updated the comment in v8 to make this clearer. Also removed
V9FS_MAGIC from this function since 9p is used for local VM/container
host mounts where fsmonitor works fine, and it's not in is_remote_fs().

> 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"?

statfs(2) gives us f_type (magic number) and f_fsid but not the
human-readable filesystem type string. We need the type name (e.g.
"nfs", "cifs") for the fs_info.typename field, which check_uds_volume()
compares against strings like "msdos" and "ntfs". /proc/mounts has that
string, and we match on f_fsid to find the right entry.

Also fixed a redundant statfs() call: find_mount() was calling
statfs(path, ...) again even though the caller already had the result.
Changed it to take a const pointer to the caller's statfs instead.