RE: [PATCH 08/12] fsmonitor: determine if filesystem is local or remote

> -----Original Message-----
> From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Sent: Monday, October 10, 2022 6:04 AM
> To: Eric DeCosta via GitGitGadget <gitgitgadget@gmail.com>
> Cc: git@vger.kernel.org; Eric DeCosta <edecosta@mathworks.com>
> 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@mathworks.com>
> >
> > 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@mathworks.com>
> > ---
> > 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>
> 

> > +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);