> -----Original Message----- > From: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> > Sent: Monday, October 10, 2022 6:04 AM > To: Eric DeCosta via GitGitGadget <gitgitgadget@xxxxxxxxx> > Cc: git@xxxxxxxxxxxxxxx; Eric DeCosta <edecosta@xxxxxxxxxxxxx> > Subject: Re: [PATCH 08/12] fsmonitor: determine if filesystem is local or > remote > > > On Sun, Oct 09 2022, Eric DeCosta via GitGitGadget wrote: > > > From: Eric DeCosta <edecosta@xxxxxxxxxxxxx> > > > > Compare the given path to the mounted filesystems. Find the mount that > > is the longest prefix of the path (if any) and determine if that mount > > is on a local or remote filesystem. > > > > Signed-off-by: Eric DeCosta <edecosta@xxxxxxxxxxxxx> > > --- > > compat/fsmonitor/fsm-path-utils-linux.c | 163 > ++++++++++++++++++++++++ > > 1 file changed, 163 insertions(+) > > create mode 100644 compat/fsmonitor/fsm-path-utils-linux.c > > > > diff --git a/compat/fsmonitor/fsm-path-utils-linux.c > > b/compat/fsmonitor/fsm-path-utils-linux.c > > new file mode 100644 > > index 00000000000..369692a788f > > --- /dev/null > > +++ b/compat/fsmonitor/fsm-path-utils-linux.c > > @@ -0,0 +1,163 @@ > > +#include "fsmonitor.h" > > +#include "fsmonitor-path-utils.h" > > +#include <errno.h> > > +#include <mntent.h> > > +#include <sys/mount.h> > > +#include <sys/statvfs.h> > > + > > +/* > > + * https://github.com/coreutils/gnulib/blob/master/lib/mountlist.c > > +<https://protect- > us.mimecast.com/s/wKJsC9rj7lc3WKmYUOq9E9?domain=gith > > +ub.com> > > + */ > > +#ifndef ME_REMOTE > > +/* A file system is "remote" if its Fs_name contains a ':' > > + or if (it is of type (smbfs or cifs) and its Fs_name starts with > > +'//') or if it is of any other of the listed types or Fs_name is > > +equal to "-hosts" (used by autofs to mount remote fs). > > + "VM" file systems like prl_fs or vboxsf are not considered remote > > +here. */ # define ME_REMOTE(Fs_name, Fs_type) \ (strchr (Fs_name, > > +':') != NULL \ > > + || ((Fs_name)[0] == '/' \ > > + && (Fs_name)[1] == '/' \ > > + && (strcmp (Fs_type, "smbfs") == 0 \ > > + || strcmp (Fs_type, "smb3") == 0 \ > > + || strcmp (Fs_type, "cifs") == 0)) \ strcmp (Fs_type, "acfs") == 0 \ > > + || strcmp (Fs_type, "afs") == 0 \ strcmp (Fs_type, "coda") == 0 \ > > + || strcmp (Fs_type, "auristorfs") == 0 \ strcmp (Fs_type, "fhgfs") > > + || == 0 \ strcmp (Fs_type, "gpfs") == 0 \ strcmp (Fs_type, "ibrix") > > + || == 0 \ strcmp (Fs_type, "ocfs2") == 0 \ strcmp (Fs_type, "vxfs") > > + || == 0 \ strcmp ("-hosts", Fs_name) == 0) > > +#endif > > So, this is just copy/pasted GPLv3 code into our GPLv2-only codebase?: > https://github.com/coreutils/gnulib/blob/cd1fdabe8b66c102124b6a5b0c97d > ded20246b46/lib/mountlist.c#L230-L247 <https://protect- > us.mimecast.com/s/zxPbC0RMy1hoXW2rCOSXJD?domain=github.com> > Yes. I was hoping for some guidance as to what to do with ME_REMOTE. I also found it, verbatim, here in midnight commander: https://github.com/MidnightCommander/mc/blob/e48cd98ac13a9b4366bd88287f632413766b967f/src/filemanager/mountlist.c#L258-L281 But that's just another GPLv3 code base. > > +static struct mntent *find_mount(const char *path, const struct > > +statvfs *fs) { const char *const mounts = "/proc/mounts"; const > > +char *rp = real_pathdup(path, 1); struct mntent *ment = NULL; > > +struct mntent *found = NULL; struct statvfs mntfs; FILE *fp; int > > +dlen, plen, flen = 0; > > + > > + fp = setmntent(mounts, "r"); > > + if (!fp) { > > + error_errno(_("setmntent('%s') failed"), mounts); return NULL; > > This code would probably be nicer if you returned int, and passed a pointer > to a struct to populate as a paremeter. Then you could just "return error..." > for this and the below cases. > > > + } > > + > > + plen = strlen(rp); > > + > > + /* read all the mount information and compare to path */ while > > + ((ment = getmntent(fp)) != NULL) { if (statvfs(ment->mnt_dir, > > + &mntfs)) { switch (errno) { case EPERM: > > + case ESRCH: > > + case EACCES: > > + continue; > > + default: > > + error_errno(_("statvfs('%s') failed"), ment->mnt_dir); > > + endmntent(fp);is return NULL; } } > > + > > + /* is mount on the same filesystem and is a prefix of the path */ if > > + ((fs->f_fsid == mntfs.f_fsid) && !strncmp(ment->mnt_dir, rp, > > + strlen(ment->mnt_dir))) { dlen = strlen(ment->mnt_dir); if (dlen > > > + plen) continue; > > + /* > > + * root is always a potential match; otherwise look for > > + * directory prefix > > + */ > > + if ((dlen == 1 && ment->mnt_dir[0] == '/') || (dlen > flen && > > + (!rp[dlen] || rp[dlen] == '/'))) { flen = dlen; > > + /* > > + * https://man7.org/linux/man-pages/man3/getmntent.3.html > > + <https://protect- > us.mimecast.com/s/aOmSCgJyVrT01WlYc76tSR?domain=man > > + 7.org> > > + * > > + * The pointer points to a static area of memory which is > > + * overwritten by subsequent calls to getmntent(). > > + */ > > + if (!found) > > + CALLOC_ARRAY(found, 1); > > It seems we never populate >1 of these, so don't you just want xmalloc(). Or > actually... > > > + free(found->mnt_dir); > > + free(found->mnt_type); > > + free(found->mnt_fsname); > > + found->mnt_dir = xmemdupz(ment->mnt_dir, strlen(ment->mnt_dir)); > > + found->mnt_type = xmemdupz(ment->mnt_type, strlen(ment- > >mnt_type)); > > + found->mnt_fsname = xmemdupz(ment->mnt_fsname, > > + found->strlen(ment->mnt_fsname)); > > Don't mix mem*() and str*(). In this case we need the string to be '\0' > delimited, so use xstrndup(). > > And once we do that, we might wonder why we're explicitly finding the '\0', > just to pass it to the xstrn*() function, when we can just do: > > found->mnt_dir = xstrdup(ment->mnt_dir); > ... > > Which would AFAICT be equivalent to what you're doing here. > > > + } > > + } > > + } > > + endmntent(fp); > > + > > + return found; > > +} > > + > > +int fsmonitor__get_fs_info(const char *path, struct fs_info *fs_info) > > +{ struct mntent *ment; > > ...make this (or above) a "struct mntent ment", then pass &ment down, so > we can fill that struct? Dunno... > > > + struct statvfs fs; > > + > > + if (statvfs(path, &fs)) > > + return error_errno(_("statvfs('%s') failed"), path); > > + > > + ment = find_mount(path, &fs); > > + if (!ment) > > + return -1; > > + > > + trace_printf_key(&trace_fsmonitor, > > + "statvfs('%s') [flags 0x%08lx] '%s' '%s'", path, fs.f_flag, > > + ment->mnt_type, ment->mnt_fsname); > > + > > + if (ME_REMOTE(ment->mnt_fsname, ment->mnt_type)) fs_info- > >is_remote > > + = 1; else fs_info->is_remote = 0; > > Shorter: > > fs_info->is_remote = ME_REMOTE(ment->mnt_fsname, ment->mnt_type);
