Re: [PATCH 15/19] Split the fusermount do_mount function

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Mar 24, 2026 at 10:05:13PM +0100, Bernd Schubert wrote:
> 
> 
> On 3/24/26 01:14, Darrick J. Wong wrote:
> > On Mon, Mar 23, 2026 at 06:45:10PM +0100, Bernd Schubert wrote:
> >> From: Bernd Schubert <bschubert@ddn.com>
> >>
> >> We will need new API and old API and need to pass the options to
> >> two different functions - factor out the option parsing.
> >>
> >> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
> >> ---
> >>  util/fusermount.c | 298 +++++++++++++++++++++++++++++++++++++-----------------
> >>  1 file changed, 205 insertions(+), 93 deletions(-)
> >>
> >> diff --git a/util/fusermount.c b/util/fusermount.c
> >> index f17b44f51142c682b339d0ce2287f7c00d644454..ecf509bb80d5cd129f6e582f1ec666502c55603a 100644
> >> --- a/util/fusermount.c
> >> +++ b/util/fusermount.c
> >> @@ -917,30 +917,132 @@ static int mount_notrunc(const char *source, const char *target,
> >>  	return mount(source, target, filesystemtype, mountflags, data);
> >>  }
> >>  
> >> +struct mount_params {
> >> +	/* Input parameters */
> >> +	int fd;                  /* /dev/fuse file descriptor */
> >> +	mode_t rootmode;         /* Root mode from stat */
> >> +	const char *dev;         /* Device path (/dev/fuse) */
> >>  
> >> -static int do_mount(const char *mnt, const char **typep, mode_t rootmode,
> >> -		    int fd, const char *opts, const char *dev, char **sourcep,
> >> -		    char **mnt_optsp)
> >> +	/* Parsed mount options */
> >> +	unsigned long flags;     /* Mount flags (MS_NOSUID, etc.) */
> >> +	char *optbuf;           /* Kernel mount options buffer */
> >> +	char *fsname;           /* Filesystem name from options */
> >> +	char *subtype;          /* Subtype from options */
> >> +	int blkdev;             /* Block device flag */
> >> +
> >> +	/* Generated mount parameters */
> >> +	char *source;           /* Mount source string */
> >> +	char *type;             /* Filesystem type string */
> >> +	char *mnt_opts;         /* Mount table options */
> >> +
> >> +	/* Pointer for optbuf manipulation */
> >> +	char *optbuf_end;       /* Points to end of optbuf for sprintf */
> >> +};
> >> +
> >> +static void free_mount_params(struct mount_params *mp)
> >> +{
> >> +	free(mp->optbuf);
> >> +	free(mp->fsname);
> >> +	free(mp->subtype);
> >> +	free(mp->source);
> >> +	free(mp->type);
> >> +	free(mp->mnt_opts);
> >> +}
> >> +
> >> +/*
> >> + * Check if option is deprecated large_read.
> >> + *
> >> + * Returns true if the option should be skipped (large_read on kernel > 2.4),
> >> + * false otherwise (all other options or large_read on old kernels).
> >> + */
> >> +static bool check_large_read(const char *opt, unsigned int len)
> >> +{
> >> +	struct utsname utsname;
> >> +	unsigned int kmaj, kmin;
> >> +	int res;
> >> +
> >> +	if (!opt_eq(opt, len, "large_read"))
> >> +		return false;
> >> +
> >> +	res = uname(&utsname);
> >> +	if (res == 0 &&
> >> +	    sscanf(utsname.release, "%u.%u", &kmaj, &kmin) == 2 &&
> >> +	    (kmaj > 2 || (kmaj == 2 && kmin > 4))) {
> >> +		fprintf(stderr,
> >> +			"%s: note: 'large_read' mount option is deprecated for %i.%i kernels\n",
> >> +			progname, kmaj, kmin);
> >> +		return true;
> > 
> > Ugh, you can drop the uname and all that.  Nobody's running 2.4 kernels
> > anymore, right?
> 
> Added a new commit that removes this. If someone complains I can recvert
> that

Thanks!

> > 
> >> +	}
> >> +	return false;
> >> +}
> >> +
> >> +/*
> >> + * Check if user has permission to use allow_other or allow_root options.
> >> + *
> >> + * Returns -1 if permission denied, 0 if allowed or option is not
> >> + * allow_other/allow_root.
> >> + */
> >> +static int check_allow_permission(const char *opt, unsigned int len)
> >> +{
> >> +	if (getuid() != 0 && !user_allow_other &&
> >> +	    (opt_eq(opt, len, "allow_other") || opt_eq(opt, len, "allow_root"))) {
> >> +		fprintf(stderr, "%s: option %.*s only allowed if 'user_allow_other' is set in %s\n",
> >> +			progname, len, opt, FUSE_CONF);
> >> +		return -1;
> >> +	}
> >> +	return 0;
> >> +}
> >> +
> >> +/*
> >> + * Process generic mount option.
> >> + *
> >> + * Handles mount flags (ro, rw, suid, etc.), kernel options
> >> + * (default_permissions, allow_other, max_read, blksize), or exits on
> >> + * unknown options.
> >> + */
> >> +static int process_generic_option(const char *opt, unsigned int len,
> >> +				   unsigned long *flags, char **dest)
> >> +{
> >> +	int on;
> >> +	int flag;
> >> +
> >> +	if (find_mount_flag(opt, len, &on, &flag)) {
> >> +		if (on)
> >> +			*flags |= flag;
> >> +		else
> >> +			*flags &= ~flag;
> >> +		return 0;
> >> +	}
> >> +
> >> +	if (opt_eq(opt, len, "default_permissions") ||
> >> +	    opt_eq(opt, len, "allow_other") ||
> >> +	    begins_with(opt, "max_read=") ||
> >> +	    begins_with(opt, "blksize=")) {
> >> +		memcpy(*dest, opt, len);
> >> +		*dest += len;
> >> +		**dest = ',';
> >> +		(*dest)++;
> >> +		return 0;
> >> +	}
> >> +
> >> +	fprintf(stderr, "%s: unknown option '%.*s'\n", progname, len, opt);
> >> +	exit(1);
> >> +}
> >> +
> >> +static int prepare_mount(const char *opts, struct mount_params *mp)
> >>  {
> >>  	int res;
> >> -	int flags = MS_NOSUID | MS_NODEV;
> >> -	char *optbuf;
> >> -	char *mnt_opts = NULL;
> >>  	const char *s;
> >>  	char *d;
> >> -	char *fsname = NULL;
> >> -	char *subtype = NULL;
> >> -	char *source = NULL;
> >> -	char *type = NULL;
> >> -	int blkdev = 0;
> >>  
> >> -	optbuf = (char *) malloc(strlen(opts) + 128);
> >> -	if (!optbuf) {
> >> +	mp->flags = MS_NOSUID | MS_NODEV;
> >> +	mp->optbuf = (char *) malloc(strlen(opts) + 128);
> >> +	if (!mp->optbuf) {
> >>  		fprintf(stderr, "%s: failed to allocate memory\n", progname);
> >>  		return -1;
> >>  	}
> >>  
> >> -	for (s = opts, d = optbuf; *s;) {
> >> +	for (s = opts, d = mp->optbuf; *s;) {
> >>  		unsigned len;
> >>  		const char *fsname_str = "fsname=";
> >>  		const char *subtype_str = "subtype=";
> >> @@ -953,10 +1055,10 @@ static int do_mount(const char *mnt, const char **typep, mode_t rootmode,
> >>  				break;
> >>  		}
> >>  		if (begins_with(s, fsname_str)) {
> >> -			if (!get_string_opt(s, len, fsname_str, &fsname))
> >> +			if (!get_string_opt(s, len, fsname_str, &mp->fsname))
> >>  				goto err;
> >>  		} else if (begins_with(s, subtype_str)) {
> >> -			if (!get_string_opt(s, len, subtype_str, &subtype))
> >> +			if (!get_string_opt(s, len, subtype_str, &mp->subtype))
> >>  				goto err;
> >>  		} else if (opt_eq(s, len, "blkdev")) {
> >>  			if (getuid() != 0) {
> >> @@ -965,7 +1067,7 @@ static int do_mount(const char *mnt, const char **typep, mode_t rootmode,
> >>  					progname);
> >>  				goto err;
> >>  			}
> >> -			blkdev = 1;
> >> +			mp->blkdev = 1;
> >>  		} else if (opt_eq(s, len, "auto_unmount")) {
> >>  			auto_unmount = 1;
> >>  		} else if (!opt_eq(s, len, "nonempty") &&
> >> @@ -973,122 +1075,132 @@ static int do_mount(const char *mnt, const char **typep, mode_t rootmode,
> >>  			   !begins_with(s, "rootmode=") &&
> >>  			   !begins_with(s, "user_id=") &&
> >>  			   !begins_with(s, "group_id=")) {
> >> -			int on;
> >> -			int flag;
> >> -			int skip_option = 0;
> >> -			if (opt_eq(s, len, "large_read")) {
> >> -				struct utsname utsname;
> >> -				unsigned kmaj, kmin;
> >> -				res = uname(&utsname);
> >> -				if (res == 0 &&
> >> -				    sscanf(utsname.release, "%u.%u",
> >> -					   &kmaj, &kmin) == 2 &&
> >> -				    (kmaj > 2 || (kmaj == 2 && kmin > 4))) {
> >> -					fprintf(stderr, "%s: note: 'large_read' mount option is deprecated for %i.%i kernels\n", progname, kmaj, kmin);
> >> -					skip_option = 1;
> >> -				}
> >> -			}
> >> -			if (getuid() != 0 && !user_allow_other &&
> >> -			    (opt_eq(s, len, "allow_other") ||
> >> -			     opt_eq(s, len, "allow_root"))) {
> >> -				fprintf(stderr, "%s: option %.*s only allowed if 'user_allow_other' is set in %s\n", progname, len, s, FUSE_CONF);
> >> +			bool skip;
> >> +
> >> +			if (check_allow_permission(s, len) == -1)
> >>  				goto err;
> >> -			}
> >> -			if (!skip_option) {
> >> -				if (find_mount_flag(s, len, &on, &flag)) {
> >> -					if (on)
> >> -						flags |= flag;
> >> -					else
> >> -						flags  &= ~flag;
> >> -				} else if (opt_eq(s, len, "default_permissions") ||
> >> -					   opt_eq(s, len, "allow_other") ||
> >> -					   begins_with(s, "max_read=") ||
> >> -					   begins_with(s, "blksize=")) {
> >> -					memcpy(d, s, len);
> >> -					d += len;
> >> -					*d++ = ',';
> >> -				} else {
> >> -					fprintf(stderr, "%s: unknown option '%.*s'\n", progname, len, s);
> >> -					exit(1);
> >> -				}
> >> -			}
> >> +
> >> +			skip = check_large_read(s, len);
> >> +
> >> +			/*
> >> +			 * Skip deprecated large_read to avoid passing it to
> >> +			 * kernel which would reject it as unknown option.
> >> +			 */
> >> +			if (!skip)
> >> +				process_generic_option(s, len, &mp->flags, &d);
> >>  		}
> >>  		s += len;
> >>  		if (*s)
> >>  			s++;
> >>  	}
> >>  	*d = '\0';
> >> -	res = get_mnt_opts(flags, optbuf, &mnt_opts);
> >> +	res = get_mnt_opts(mp->flags, mp->optbuf, &mp->mnt_opts);
> >>  	if (res == -1)
> >>  		goto err;
> >>  
> >> +	mp->optbuf_end = d;
> >> +
> >>  	sprintf(d, "fd=%i,rootmode=%o,user_id=%u,group_id=%u",
> >> -		fd, rootmode, getuid(), getgid());
> >> +		mp->fd, mp->rootmode, getuid(), getgid());
> >>  
> >> -	source = malloc((fsname ? strlen(fsname) : 0) +
> >> -			(subtype ? strlen(subtype) : 0) + strlen(dev) + 32);
> >> +	mp->source = malloc((mp->fsname ? strlen(mp->fsname) : 0) +
> >> +			(mp->subtype ? strlen(mp->subtype) : 0) + strlen(mp->dev) + 32);
> >>  
> >> -	type = malloc((subtype ? strlen(subtype) : 0) + 32);
> >> -	if (!type || !source) {
> >> +	mp->type = malloc((mp->subtype ? strlen(mp->subtype) : 0) + 32);
> >> +	if (!mp->type || !mp->source) {
> >>  		fprintf(stderr, "%s: failed to allocate memory\n", progname);
> >>  		goto err;
> >>  	}
> >>  
> >> -	if (subtype)
> >> -		sprintf(type, "%s.%s", blkdev ? "fuseblk" : "fuse", subtype);
> >> +	if (mp->subtype)
> >> +		sprintf(mp->type, "%s.%s", mp->blkdev ? "fuseblk" : "fuse", mp->subtype);
> >>  	else
> >> -		strcpy(type, blkdev ? "fuseblk" : "fuse");
> >> +		strcpy(mp->type, mp->blkdev ? "fuseblk" : "fuse");
> > 
> > Hrm, maybe the patch adding fuse_mnt_build_source / fuse_mnt_build_type
> > should have done something about this code too.
> > 
> > The straight-up conversion looks ok though.
> 
> Yeah, I need to do that in a later commit that makes these two functions
> more generic.

<nod> Looking forward to it.  I hoep I'm not being too annoying by doing
an xfs-style "and here's other random small cleanups" review :P

--D