Re: [PATCH v4 2/6] fsmonitor: determine if filesystem is local or remote
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 25, 2022, 07:31 UTC
- Message-ID
- <xmqqedtrcwxy.fsf@gitster.g>
- In-Reply-To
- <e53fc07754094aa5ba8080ec7761869c6429a8af.1669230044.git.gitgitgadget@gmail.com>
"Eric DeCosta via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +static int is_remote_fs(const char* path) {Style (asterisk sticks to the variable not to the type).
Show 8 quoted lines
> + 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().
Show 5 quoted lines
> +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.
Show 9 quoted lines
> + 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;
Show 50 quoted lines
> + 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?
Show 9 quoted lines
> + } > + } > + endmntent(fp); > + > + if (!found) > + return -1; > + > + return 0; > +}
Show 14 quoted lines
> +
> +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().
Show 15 quoted lines
> + 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.
Show 37 quoted lines
> + }
> +
> + 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;
> +}