"Eric DeCosta via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> +static int is_remote_fs(const char* path) {
Style (asterisk sticks to the variable not to the type).
> + switch (fs.f_type) {
> + case 0x61636673: /* ACFS */
> ...
> + case 0xA501FCF5: /* VXFS */
> + return 1;
> + default:
> + break;
> + }
Align case/default to switch by de-denting one level, just like you
did the switch() statement in find_mount().
> +static int find_mount(const char *path, const struct statvfs *fs,
> + struct mntent *ent)
> +{
> + const char *const mounts = "/proc/mounts";
> + const char *rp = real_pathdup(path, 1);
Nobody seems to free() this once we are done with this function.
> + struct mntent *ment = NULL;
> + struct statvfs mntfs;
> + FILE *fp;
> + int found = 0;
> + int dlen, plen, flen = 0;
> +
> + ent->mnt_fsname = NULL;
> + ent->mnt_dir = NULL;
> + ent->mnt_type = NULL;
More on this later.
> + fp = setmntent(mounts, "r");
> + if (!fp) {
> + error_errno(_("setmntent('%s') failed"), mounts);
> + return -1;
> + }
> +
> + 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);
> + return -1;
> + }
> + }
> +
> + /* 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
> + *
> + * The pointer points to a static area of memory which is
> + * overwritten by subsequent calls to getmntent().
> + */
> + found = 1;
> + free(ent->mnt_fsname);
> + free(ent->mnt_dir);
> + free(ent->mnt_type);
> + ent->mnt_fsname = xstrdup(ment->mnt_fsname);
> + ent->mnt_dir = xstrdup(ment->mnt_dir);
> + ent->mnt_type = xstrdup(ment->mnt_type);
> + }
So more than one mount entries could match the given path. This
loop implements "the last one wins", but is that a sensible thing to
do? Shouldn't it be more like "the longest one, being the most
specific, wins"? If /usr mounts on / and /usr/local mounts on /usr,
asking for /usr/local/me would want to discover that it is on
the filesystem mounted at /usr/local regardless of the order in
which getmntent() returns the entries, no?
> + }
> + }
> + endmntent(fp);
> +
> + if (!found)
> + return -1;
> +
> + return 0;
> +}
> +
> +int fsmonitor__get_fs_info(const char *path, struct fs_info *fs_info)
> +{
> + struct mntent ment;
> + struct statvfs fs;
> +
> + if (statvfs(path, &fs))
> + return error_errno(_("statvfs('%s') failed"), path);
> +
> +
> + if (find_mount(path, &fs, &ment) < 0) {
> + free(ment.mnt_fsname);
> + free(ment.mnt_dir);
> + free(ment.mnt_type);
It is a good idea to free, but I _think_ the code is easier to
follow if you _clear_ ment before calling find_mount(), not in
find_mount().
> + return -1;
> + }
> +
> + trace_printf_key(&trace_fsmonitor,
> + "statvfs('%s') [flags 0x%08lx] '%s' '%s'",
> + path, fs.f_flag, ment.mnt_type, ment.mnt_fsname);
> +
> + fs_info->is_remote = is_remote_fs(ment.mnt_dir);
> + fs_info->typename = ment.mnt_fsname;
> + free(ment.mnt_dir);
> + free(ment.mnt_type);
> +
> + if (fs_info->is_remote < 0) {
> + free(ment.mnt_fsname);
> + return -1;
fs_info->typename just started to point into an already freed memory
at this point, which is a very safe thing to do to the caller of
this function.
Rather, perhaps delay the setting of typename after this statement,
which would be more friendly to the caller than telling them that
they are not allowed to touch the member when the function returns
negative.
> + }
> +
> + trace_printf_key(&trace_fsmonitor,
> + "'%s' is_remote: %d",
> + path, fs_info->is_remote);
> +
> + return 0;
> +}
> +
> +int fsmonitor__is_fs_remote(const char *path)
> +{
> + struct fs_info fs;
> +
> + if (fsmonitor__get_fs_info(path, &fs))
> + return -1;
> +
> + free(fs.typename);
> +
> + return fs.is_remote;
> +}
> +
> +/*
> + * No-op for now.
> + */
> +int fsmonitor__get_alias(const char *path, struct alias_info *info)
> +{
> + return 0;
> +}
> +
> +/*
> + * No-op for now.
> + */
> +char *fsmonitor__resolve_alias(const char *path,
> + const struct alias_info *info)
> +{
> + return NULL;
> +}
