git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Eric DeCosta via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Eric Sunshine" <sunshine@sunshineco.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Glen Choo" <chooglen@google.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Taylor Blau" <me@ttaylorr.com>,
	"Eric DeCosta" <edecosta@mathworks.com>
Subject: Re: [PATCH v4 2/6] fsmonitor: determine if filesystem is local or remote
Date: Fri, 25 Nov 2022 16:31:05 +0900	[thread overview]
Message-ID: <xmqqedtrcwxy.fsf@gitster.g> (raw)
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).

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

  reply	other threads:[~2022-11-25  7:31 UTC|newest]

Thread overview: 89+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-09 14:37 [PATCH 00/12] fsmonitor: Implement fsmonitor for Linux Eric DeCosta via GitGitGadget
2022-10-09 14:37 ` [PATCH 01/12] fsmonitor: refactor filesystem checks to common interface Eric DeCosta via GitGitGadget
2022-10-18 12:29   ` Ævar Arnfjörð Bjarmason
2022-10-09 14:37 ` [PATCH 02/12] fsmonitor: relocate socket file if .git directory is remote Eric DeCosta via GitGitGadget
2022-10-09 14:37 ` [PATCH 03/12] fsmonitor: avoid socket location check if using hook Eric DeCosta via GitGitGadget
2022-10-09 14:37 ` [PATCH 04/12] fsmonitor: deal with synthetic firmlinks on macOS Eric DeCosta via GitGitGadget
2022-10-09 14:37 ` [PATCH 05/12] fsmonitor: check for compatability before communicating with fsmonitor Eric DeCosta via GitGitGadget
2022-10-09 14:37 ` [PATCH 06/12] fsmonitor: add documentation for allowRemote and socketDir options Eric DeCosta via GitGitGadget
2022-10-09 14:37 ` [PATCH 07/12] fsmonitor: prepare to share code between Mac OS and Linux Eric DeCosta via GitGitGadget
2022-10-09 22:36   ` Junio C Hamano
2022-10-10 16:23   ` Jeff Hostetler
2022-10-09 14:37 ` [PATCH 08/12] fsmonitor: determine if filesystem is local or remote Eric DeCosta via GitGitGadget
2022-10-10 10:04   ` Ævar Arnfjörð Bjarmason
2022-10-14 19:38     ` Eric DeCosta
2022-10-09 14:37 ` [PATCH 09/12] fsmonitor: implement filesystem change listener for Linux Eric DeCosta via GitGitGadget
2022-10-09 14:37 ` [PATCH 10/12] fsmonitor: enable fsmonitor " Eric DeCosta via GitGitGadget
2022-10-09 14:37 ` [PATCH 11/12] fsmonitor: test updates Eric DeCosta via GitGitGadget
2022-10-18 12:42   ` Ævar Arnfjörð Bjarmason
2022-10-09 14:37 ` [PATCH 12/12] fsmonitor: update doc for Linux Eric DeCosta via GitGitGadget
2022-10-18 12:43   ` Ævar Arnfjörð Bjarmason
2022-10-09 22:24 ` [PATCH 00/12] fsmonitor: Implement fsmonitor " Junio C Hamano
2022-10-10  0:19   ` Eric Sunshine
2022-10-10 21:08 ` Junio C Hamano
2022-10-14 21:45 ` [PATCH v2 " Eric DeCosta via GitGitGadget
2022-10-14 21:45   ` [PATCH v2 01/12] fsmonitor: refactor filesystem checks to common interface Eric DeCosta via GitGitGadget
2022-10-14 21:45   ` [PATCH v2 02/12] fsmonitor: relocate socket file if .git directory is remote Eric DeCosta via GitGitGadget
2022-10-18 12:12     ` Ævar Arnfjörð Bjarmason
2022-10-14 21:45   ` [PATCH v2 03/12] fsmonitor: avoid socket location check if using hook Eric DeCosta via GitGitGadget
2022-10-14 21:45   ` [PATCH v2 04/12] fsmonitor: deal with synthetic firmlinks on macOS Eric DeCosta via GitGitGadget
2022-10-14 21:45   ` [PATCH v2 05/12] fsmonitor: check for compatability before communicating with fsmonitor Eric DeCosta via GitGitGadget
2022-10-14 21:45   ` [PATCH v2 06/12] fsmonitor: add documentation for allowRemote and socketDir options Eric DeCosta via GitGitGadget
2022-10-14 21:45   ` [PATCH v2 07/12] fsmonitor: prepare to share code between Mac OS and Linux Eric DeCosta via GitGitGadget
2022-10-14 23:46     ` Junio C Hamano
2022-10-17 21:30       ` Eric DeCosta
2022-10-18  6:31         ` Junio C Hamano
2022-10-14 21:45   ` [PATCH v2 08/12] fsmonitor: determine if filesystem is local or remote Eric DeCosta via GitGitGadget
2022-10-14 21:45   ` [PATCH v2 09/12] fsmonitor: implement filesystem change listener for Linux Eric DeCosta via GitGitGadget
2022-10-18 11:59     ` Ævar Arnfjörð Bjarmason
2022-10-14 21:45   ` [PATCH v2 10/12] fsmonitor: enable fsmonitor " Eric DeCosta via GitGitGadget
2022-10-14 21:45   ` [PATCH v2 11/12] fsmonitor: test updates Eric DeCosta via GitGitGadget
2022-10-14 21:45   ` [PATCH v2 12/12] fsmonitor: update doc for Linux Eric DeCosta via GitGitGadget
2022-10-14 23:32   ` [PATCH v2 00/12] fsmonitor: Implement fsmonitor " Junio C Hamano
2022-10-17 21:32     ` Eric DeCosta
2022-10-17 22:22       ` Junio C Hamano
2022-10-18 14:02       ` Johannes Schindelin
2022-10-17 22:14   ` Glen Choo
2022-10-18  4:17     ` Junio C Hamano
2022-10-18 17:03       ` Glen Choo
2022-10-18 17:11         ` Junio C Hamano
2022-10-19  1:19         ` Ævar Arnfjörð Bjarmason
2022-10-19  2:28           ` Eric Sunshine
2022-10-19 16:58             ` Junio C Hamano
2022-10-19 19:11             ` Ævar Arnfjörð Bjarmason
2022-10-19 20:14               ` Eric Sunshine
2022-10-19  1:04       ` Ævar Arnfjörð Bjarmason
2022-10-19 16:33         ` Junio C Hamano
2022-10-20 16:13       ` Junio C Hamano
2022-10-20 16:37         ` Eric Sunshine
2022-10-20 17:01           ` Junio C Hamano
2022-10-20 17:54             ` Eric Sunshine
2022-10-20 15:43     ` Junio C Hamano
2022-10-20 22:01       ` Junio C Hamano
2022-11-16 23:23   ` [PATCH v3 0/6] " Eric DeCosta via GitGitGadget
2022-11-16 23:23     ` [PATCH v3 1/6] fsmonitor: prepare to share code between Mac OS and Linux Eric DeCosta via GitGitGadget
2022-11-16 23:23     ` [PATCH v3 2/6] fsmonitor: determine if filesystem is local or remote Eric DeCosta via GitGitGadget
2022-11-16 23:23     ` [PATCH v3 3/6] fsmonitor: implement filesystem change listener for Linux Eric DeCosta via GitGitGadget
2022-11-16 23:23     ` [PATCH v3 4/6] fsmonitor: enable fsmonitor " Eric DeCosta via GitGitGadget
2022-11-16 23:23     ` [PATCH v3 5/6] fsmonitor: test updates Eric DeCosta via GitGitGadget
2022-11-16 23:23     ` [PATCH v3 6/6] fsmonitor: update doc for Linux Eric DeCosta via GitGitGadget
2022-11-16 23:27     ` [PATCH v3 0/6] fsmonitor: Implement fsmonitor " Taylor Blau
2022-11-23 19:00     ` [PATCH v4 " Eric DeCosta via GitGitGadget
2022-11-23 19:00       ` [PATCH v4 1/6] fsmonitor: prepare to share code between Mac OS and Linux Eric DeCosta via GitGitGadget
2022-11-23 19:00       ` [PATCH v4 2/6] fsmonitor: determine if filesystem is local or remote Eric DeCosta via GitGitGadget
2022-11-25  7:31         ` Junio C Hamano [this message]
2022-12-12 10:24         ` Ævar Arnfjörð Bjarmason
2022-11-23 19:00       ` [PATCH v4 3/6] fsmonitor: implement filesystem change listener for Linux Eric DeCosta via GitGitGadget
2022-12-12 10:42         ` Ævar Arnfjörð Bjarmason
2022-11-23 19:00       ` [PATCH v4 4/6] fsmonitor: enable fsmonitor " Eric DeCosta via GitGitGadget
2022-11-23 19:00       ` [PATCH v4 5/6] fsmonitor: test updates Eric DeCosta via GitGitGadget
2022-11-23 19:00       ` [PATCH v4 6/6] fsmonitor: update doc for Linux Eric DeCosta via GitGitGadget
2022-12-12 21:57       ` [PATCH v5 0/6] fsmonitor: Implement fsmonitor " Eric DeCosta via GitGitGadget
2022-12-12 21:58         ` [PATCH v5 1/6] fsmonitor: prepare to share code between Mac OS and Linux Eric DeCosta via GitGitGadget
2022-12-12 21:58         ` [PATCH v5 2/6] fsmonitor: determine if filesystem is local or remote Eric DeCosta via GitGitGadget
2022-12-12 21:58         ` [PATCH v5 3/6] fsmonitor: implement filesystem change listener for Linux Eric DeCosta via GitGitGadget
2022-12-12 21:58         ` [PATCH v5 4/6] fsmonitor: enable fsmonitor " Eric DeCosta via GitGitGadget
2022-12-12 21:58         ` [PATCH v5 5/6] fsmonitor: test updates Eric DeCosta via GitGitGadget
2022-12-12 21:58         ` [PATCH v5 6/6] fsmonitor: update doc for Linux Eric DeCosta via GitGitGadget
2023-04-12  5:42         ` [PATCH v5 0/6] fsmonitor: Implement fsmonitor " Junio C Hamano
2022-10-18 12:47 ` [PATCH 00/12] " Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqedtrcwxy.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=chooglen@google.com \
    --cc=edecosta@mathworks.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=me@ttaylorr.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).