Bug 120784

It appears that mount_fs destroys some arguments. This is an improved patch 
which fixes this.

The last uploaded patch is not downloadable. I have changed the encoding 
settings of Thunderbird and hope it will work this time.

This is an alternative patch I have sent to Remko Lodder. It probably should 
be available here, too.

This one also gets rid of the use_mountprog() stuff, since things simply 
work automatically due to the fallback. The previous version of the patch 
was targeted at touching as few lines as possible.

This one is meant to be as clean as I can make it. I prefer it, but both 
versions work and a more conservative mind might prefer the previous patch.



diff -Pur sbin/mount.orig/mount.c sbin/mount/mount.c
--- sbin/mount.orig/mount.c	2008-02-18 19:44:05.000000000 +0100
+++ sbin/mount/mount.c	2008-02-19 19:02:43.000000000 +0100
@@ -126,28 +126,6 @@
  static const char groupquotaeq[] = "groupquota=";

  static int
-use_mountprog(const char *vfstype)
-{
-	/* XXX: We need to get away from implementing external mount
-	 *      programs for every filesystem, and move towards having
-	 *	each filesystem properly implement the nmount() system call.
-	 */
-	unsigned int i;
-	const char *fs[] = {
-	"cd9660", "mfs", "msdosfs", "nfs", "nfs4", "ntfs",
-	"nwfs", "nullfs", "portalfs", "smbfs", "udf", "unionfs",
-	NULL
-	};
-
-	for (i = 0; fs[i] != NULL; ++i) {
-		if (strcmp(vfstype, fs[i]) == 0)
-			return (1);
-	}
-	
-	return (0);
-}
-
-static int
  exec_mountprog(const char *name, const char *execname, char *const argv[])
  {
  	pid_t pid;
@@ -547,20 +525,18 @@
  	argv[argc] = NULL;

  	if (debug) {
-		if (use_mountprog(vfstype))
-			printf("exec: mount_%s", vfstype);
-		else
-			printf("mount -t %s", vfstype);
+		printf("mount -t %s", vfstype);
  		for (i = 1; i < argc; i++)
  			(void)printf(" %s", argv[i]);
  		(void)printf("\n");
  		return (0);
  	}

-	if (use_mountprog(vfstype)) {
+	ret = mount_fs(vfstype, argc, argv);
+	if (ret < 0) {
+		if (verbose)
+			warn("falling back to old style mount");
  		ret = exec_mountprog(name, execname, argv);
-	} else {
-		ret = mount_fs(vfstype, argc, argv);
  	}

  	free(optbuf);
diff -Pur sbin/mount.orig/mount_fs.c sbin/mount/mount_fs.c
--- sbin/mount.orig/mount_fs.c	2008-02-18 19:44:05.000000000 +0100
+++ sbin/mount/mount_fs.c	2008-02-18 19:44:37.000000000 +0100
@@ -107,6 +107,11 @@
  				val = p + 1;
  			}
  			build_iovec(&iov, &iovlen, optarg, val, (size_t)-1);
+			// Repair arguments, in case they are required when
+			// falling back to the old style exec_mountprog.
+			if (p != NULL) {
+				*p = '=';
+			}
  			break;
  		case '?':
  		default:
@@ -131,8 +136,6 @@
  	build_iovec(&iov, &iovlen, "errmsg", errmsg, sizeof(errmsg));
  	
  	ret = nmount(iov, iovlen, mntflags);
-	if (ret < 0)
-		err(1, "%s %s", dev, errmsg);

  	return (ret);
  }

This problem was mentioned last February and long standing bug,
i.e. http://docs.freebsd.org/cgi/getmsg.cgi?fetch=282404+0+archive/2007/freebsd-current/20070204.freebsd-current

In 7-release, mount program is modified to rely on nmount system call
based on the source code change I read.

The implementation to detect whether it is to call nmount or mount_XXX
is determined by a list of KNOWN external mount programs.  THAT is the
problem as I mentioned one year ago.  mount(3) is more tied to nmount(2)
so that it is more reasonable and appropriate to list all nmount2) supported
file systems like my patch.  This is another alternative to this problem.

It appears that mount_fs does not have a return policy when encountering 
unknown options. This makes my patch unusable in some cases. E.g.
# mount -t msdosfs -o -L=$LANG device mountpoint

The patch from Yoshihiro Ota does not raise such problems, so I suggest this 
should be the one with the testing focus. My patches are to be discarded.

Comment 6 Alejandro Pulver 2008-04-21 18:43:24 UTC

I'm the maintainer of the fusefs-ntfs port, and the previous patch is
the one mentioned in the README.FreeBSD (under files/) to solve the
startup mount issue.

So IMO it would be nice if it gets committed.

Best Regards,
Ale

Comment 7 Robert Watson 2008-11-04 15:00:29 UTC

Responsible Changed
From-To: freebsd-bugs->rodrigc

Assign to Craig, who has been working on the mount code.  Craig, could you 
take a look at these patches and see if we can get some appropriate 
solution into SVN?  Thanks!

patch-5.diff leaves out ntfs-3g, so it doesn't fix the 'cannot mount 
ntfs-3g in fstab' problem.

see  
http://stackoverflow.com/questions/137219/automatically-mounting-ntfs-partition-on-freebsd-at-boot-time

On Fri, Nov 07, 2008 at 04:20:07AM +0000, George Wood wrote:
>  patch-5.diff leaves out ntfs-3g, so it doesn't fix the 'cannot mount 
>  ntfs-3g in fstab' problem.

I disagree with your patch too.

I would prefer to see a new option added to mount(8),
-o extmountprog=.....

which would specifically force mount to use an external mount program.

Not ideal, but that way we can keep the existing mount behavior,
and not have to extend the list inside mount(8) for file systems which
need to use an external mount program.

Ideally, the FUSE people should move FUSE to use nmount() properly,
and any functionality which must be done in userspace should migrate
into mount(8) itself.

-- 
Craig Rodrigues
rodrigc@crodrigues.org

George Wood wrote:
> patch-5.diff leaves out ntfs-3g, so it doesn't fix the 'cannot mount 
> ntfs-3g in fstab' problem.
> 
> see  
> http://stackoverflow.com/questions/137219/automatically-mounting-ntfs-partition-on-freebsd-at-boot-time
> 

Read it again.

It does fix the problem.

Regards

On Fri, Nov 07, 2008 at 04:20:07AM +0000, George Wood wrote:
> > patch-5.diff leaves out ntfs-3g, so it doesn't fix the 'cannot mount
> > ntfs-3g in fstab' problem.
>
> I disagree with your patch too.
>
> I would prefer to see a new option added to mount(8),
> -o extmountprog=.....
>
> which would specifically force mount to use an external mount program.
>
> Not ideal, but that way we can keep the existing mount behavior,
> and not have to extend the list inside mount(8) for file systems which
> need to use an external mount program.

Read Yoshihiro Ota's mail. You have failed to understand how the patch
works.

The whole point of patch-5 is that you do not have to keep track of all
the file systems. It's a much cleaner solution than your proposal.

Regards

On Fri, Nov 07, 2008 at 09:10:08AM +0000, Dominic Fandrey wrote:
>  Read Yoshihiro Ota's mail. You have failed to understand how the patch
>  works.

If you mean this e-mail:
http://lists.freebsd.org/pipermail/freebsd-current/2007-February/068980.html

I've seen that patch and don't like it either.  It inverts the logic
use_mountprog(), and doesn't get it right, because it doesn't
list all possible file systems which need to use just nmount().

I still prefer adding a -o extmountprog flag to mount(8).
-- 
Craig Rodrigues
rodrigc@crodrigues.org

Craig Rodrigues wrote:
> On Fri, Nov 07, 2008 at 09:10:08AM +0000, Dominic Fandrey wrote:
>>  Read Yoshihiro Ota's mail. You have failed to understand how the patch
>>  works.
> 
> If you mean this e-mail:
> http://lists.freebsd.org/pipermail/freebsd-current/2007-February/068980.html
> 
> I've seen that patch and don't like it either.  It inverts the logic
> use_mountprog(), and doesn't get it right, because it doesn't
> list all possible file systems which need to use just nmount().
> 
> I still prefer adding a -o extmountprog flag to mount(8).

Wouldn't it be easier to have a list of all nmount using file systems?
We do, hopefully, know all the file systems using nmount (they're all in base
after all). But the number of user space mount programs could be infinite.

On Fri, Nov 07, 2008 at 11:47:04AM +0100, Dominic Fandrey wrote:
> Wouldn't it be easier to have a list of all nmount using file systems?
> We do, hopefully, know all the file systems using nmount (they're all in base
> after all). But the number of user space mount programs could be infinite.

No, it wouldn't.  We can't assume that maintaining a list of file systems
inside mount(8) is a good thing.  The only reason I put the existing
list of external mount programs in mount(8) was a temporary hack
to support existing file systems *in the base system* that had not been
fully converted to nmount(), to support existing fstab setups.  This forces
authors of new file systems to
explicitly support nmount() directly.  Over time, I have been converting
file systems and removing mount programs *in the base system* for file
systems which support nmount().

It is not possible to migrate every file system to just use nmount(),
(NFS still must do some stuff in userspace before calling nmount()).

For newer file systems that still require another mount program, I think
adding a flag to mount(8) is OK. 

-- 
Craig Rodrigues
rodrigc@crodrigues.org

Craig Rodrigues wrote:
> On Fri, Nov 07, 2008 at 11:47:04AM +0100, Dominic Fandrey wrote:
>> Wouldn't it be easier to have a list of all nmount using file systems?
>> We do, hopefully, know all the file systems using nmount (they're all in base
>> after all). But the number of user space mount programs could be infini
> 
> No, it wouldn't.  We can't assume that maintaining a list of file systems
> inside mount(8) is a good thing.  The only reason I put the existing
> list of external mount programs in mount(8) was a temporary hack
> to support existing file systems *in the base system* that had not been
> fully converted to nmount(), to support existing fstab setups.  This forces
> authors of new file systems to
> explicitly support nmount() directly.  Over time, I have been converting
> file systems and removing mount programs *in the base system* for file
> systems which support nmount().
> 
> It is not possible to migrate every file system to just use nmount(),
> (NFS still must do some stuff in userspace before calling nmount()).
> 
> For newer file systems that still require another mount program, I think
> adding a flag to mount(8) is OK. 
> 

As well, by now I'd be glad about any solution. One patch less to apply after
every csup. My favourite would still be a fallback, but there are too many
places where the assumption was made that an error causes a call of exit().

I wish errno had been used instead. Exception handling is one of the few things
I miss in C.

I have finally created a patch that adds an option called
"external" to mount.

diff -Pur src/sbin/mount.orig/mount.8 src/sbin/mount/mount.8
--- src/sbin/mount.orig/mount.8	2009-01-13 10:54:29.000000000 +0100
+++ src/sbin/mount/mount.8	2009-01-13 12:47:57.000000000 +0100
@@ -142,6 +142,11 @@
 .Fl u
 flag, this is the same as specifying the options currently in effect for
 the mounted file system.
+.It Cm external
+This flag forces the use of external mount programs,
+allowing the use of userland mount tools that do not support the
+.Xr nmount 2
+architecture.
 .It Cm force
 The same as
 .Fl f ;
diff -Pur src/sbin/mount.orig/mount.c src/sbin/mount/mount.c
--- src/sbin/mount.orig/mount.c	2009-01-13 10:54:29.000000000 +0100
+++ src/sbin/mount/mount.c	2009-01-13 12:55:16.000000000 +0100
@@ -503,9 +503,11 @@
 {
 	char *argv[100];
 	struct statfs sf;
-	int argc, i, ret;
+	int argc, i, ret, external;
 	char *optbuf, execname[PATH_MAX], mntpath[PATH_MAX];
 
+	external = 0;
+
 	/* resolve the mountpoint with realpath(3) */
 	(void)checkpath(name, mntpath);
 	name = mntpath;
@@ -514,6 +516,9 @@
 		mntopts = "";
 	optbuf = catopt(strdup(mntopts), options);
 
+	if (hasopt(optbuf, "external"))
+		external = 1;
+
 	if (strcmp(name, "/") == 0)
 		flags |= MNT_UPDATE;
 	if (flags & MNT_FORCE)
@@ -547,7 +552,7 @@
 	argv[argc] = NULL;
 
 	if (debug) {
-		if (use_mountprog(vfstype))
+		if (external || use_mountprog(vfstype))
 			printf("exec: mount_%s", vfstype);
 		else
 			printf("mount -t %s", vfstype);
@@ -557,7 +562,7 @@
 		return (0);
 	}
 
-	if (use_mountprog(vfstype)) {
+	if (external || use_mountprog(vfstype)) {
 		ret = exec_mountprog(name, execname, argv);
 	} else {
 		ret = mount_fs(vfstype, argc, argv); 
@@ -689,6 +694,12 @@
 				 * before mountd starts.
 				 */
 				continue;
+			} else if (strcmp(p, "external") == 0) {
+				/*
+				 * "external" is used to force the use of
+				 * userland mount programs.
+				 */
+				continue;
 			} else if (strcmp(p, "userquota") == 0) {
 				continue;
 			} else if (strncmp(p, userquotaeq,

Comment 17 Sam Leffler 2009-02-03 16:51:45 UTC

Responsible Changed
From-To: rodrigc->sam

I'll try to deal with this

On Tue, Feb 03, 2009 at 04:51:58PM +0000, sam@FreeBSD.org wrote:
> Synopsis: [patch] mount(8): allow mount from fstab with 3rd party tools like ntfs-3g
> 
> Responsible-Changed-From-To: rodrigc->sam
> Responsible-Changed-By: sam
> Responsible-Changed-When: Tue Feb 3 16:51:45 UTC 2009
> Responsible-Changed-Why: 
> I'll try to deal with this
> 
> http://www.freebsd.org/cgi/query-pr.cgi?pr=120784

This patch looks OK, but I would prefer the option
be changed from -o external to -o mountprog
to make it clear that you are specifying a mount program.

-- 
Craig Rodrigues
rodrigc@crodrigues.org

How about that one?



diff -Pur src/sbin/mount.orig/mount.8 src/sbin/mount/mount.8
--- src/sbin/mount.orig/mount.8	2009-02-05 17:35:47.000000000 +0100
+++ src/sbin/mount/mount.8	2009-02-05 17:37:52.000000000 +0100
@@ -163,6 +163,11 @@
 flag but without the
 .Fl l
 flag.
+.It Cm mountprog
+This flag forces the use of external mount programs,
+allowing the use of userland mount tools that do not support the
+.Xr nmount 2
+architecture.
 .It Cm multilabel
 Enable multi-label Mandatory Access Control, or MAC, on the specified file
 system.
diff -Pur src/sbin/mount.orig/mount.c src/sbin/mount/mount.c
--- src/sbin/mount.orig/mount.c	2009-02-05 17:35:47.000000000 +0100
+++ src/sbin/mount/mount.c	2009-02-05 17:36:08.000000000 +0100
@@ -503,9 +503,11 @@
 {
 	char *argv[100];
 	struct statfs sf;
-	int argc, i, ret;
+	int argc, i, ret, mountprog;
 	char *optbuf, execname[PATH_MAX], mntpath[PATH_MAX];
 
+	mountprog = 0;
+
 	/* resolve the mountpoint with realpath(3) */
 	(void)checkpath(name, mntpath);
 	name = mntpath;
@@ -514,6 +516,9 @@
 		mntopts = "";
 	optbuf = catopt(strdup(mntopts), options);
 
+	if (hasopt(optbuf, "mountprog"))
+		mountprog = 1;
+
 	if (strcmp(name, "/") == 0)
 		flags |= MNT_UPDATE;
 	if (flags & MNT_FORCE)
@@ -547,7 +552,7 @@
 	argv[argc] = NULL;
 
 	if (debug) {
-		if (use_mountprog(vfstype))
+		if (mountprog || use_mountprog(vfstype))
 			printf("exec: mount_%s", vfstype);
 		else
 			printf("mount -t %s", vfstype);
@@ -557,7 +562,7 @@
 		return (0);
 	}
 
-	if (use_mountprog(vfstype)) {
+	if (mountprog || use_mountprog(vfstype)) {
 		ret = exec_mountprog(name, execname, argv);
 	} else {
 		ret = mount_fs(vfstype, argc, argv); 
@@ -689,6 +694,12 @@
 				 * before mountd starts.
 				 */
 				continue;
+			} else if (strcmp(p, "mountprog") == 0) {
+				/*
+				 * "mountprog" is used to force the use of
+				 * userland mount programs.
+				 */
+				continue;
 			} else if (strcmp(p, "userquota") == 0) {
 				continue;
 			} else if (strncmp(p, userquotaeq,

Comment 20 Craig Rodrigues 2009-02-15 00:54:56 UTC

Responsible Changed
From-To: sam->rodrigc

Taking back, discussed with sam.

Comment 21 dfilter service 2009-03-05 08:57:50 UTC

Author: rodrigc
Date: Thu Mar  5 08:57:35 2009
New Revision: 189397
URL: http://svn.freebsd.org/changeset/base/189397

Log:
  Add a -o mountprog parameter to mount which explicitly allows
  an alternative program to be used for mounting a file system.
  Ideally, all file systems
  should be converted to pass string arguments to nmount(), so that
  /sbin/mount can handle them.  However, certain file systems such as FUSE have
  not done this, and want to have their own userland mount programs.
  
  For example, to mount an NTFS file system with the FUSE NTFS driver:
  
  mount -t ntfs -o mountprog=/usr/local/bin/ntfs-3g /dev/acd0 /mnt
  
  or via an fstab entry:
  
  /dev/acd0  /mnt  ntfs    ro,noauto,mountprog=/usr/local/bin/ntfs-3g       0       0
  
  PR:	120784
  Requested by: Dominic Fandrey

Modified:
  head/sbin/mount/mount.8
  head/sbin/mount/mount.c

Modified: head/sbin/mount/mount.8
==============================================================================
--- head/sbin/mount/mount.8	Thu Mar  5 08:08:09 2009	(r189396)
+++ head/sbin/mount/mount.8	Thu Mar  5 08:57:35 2009	(r189397)
@@ -163,6 +163,15 @@ is run with the
 flag but without the
 .Fl l
 flag.
+.It Cm mountprog Ns = Ns Aq Ar program
+Force
+.Nm
+to use the specified program to mount the file system, instead of calling
+.Xr nmount 2
+directly.  For example:
+.Bd -literal
+mount -t foofs -o mountprog=/mydir/fooprog /dev/acd0 /mnt
+.Ed
 .It Cm multilabel
 Enable multi-label Mandatory Access Control, or MAC, on the specified file
 system.

Modified: head/sbin/mount/mount.c
==============================================================================
--- head/sbin/mount/mount.c	Thu Mar  5 08:08:09 2009	(r189396)
+++ head/sbin/mount/mount.c	Thu Mar  5 08:57:35 2009	(r189397)
@@ -129,6 +129,8 @@ remountable_fs_names[] = {
 static const char userquotaeq[] = "userquota=";
 static const char groupquotaeq[] = "groupquota=";
 
+static char *mountprog = NULL;
+
 static int
 use_mountprog(const char *vfstype)
 {
@@ -143,6 +145,9 @@ use_mountprog(const char *vfstype)
 	NULL
 	};
 
+	if (mountprog != NULL)
+		return (1);
+
 	for (i = 0; fs[i] != NULL; ++i) {
 		if (strcmp(vfstype, fs[i]) == 0)
 			return (1);
@@ -165,8 +170,10 @@ exec_mountprog(const char *name, const c
 		/* Go find an executable. */
 		execvP(execname, _PATH_SYSPATH, argv);
 		if (errno == ENOENT) {
-			warn("exec %s not found in %s", execname,
-			    _PATH_SYSPATH);
+			warn("exec %s not found", execname);
+			if (execname[0] != '/') {
+				warnx("in path: %s", _PATH_SYSPATH);
+			}
 		}
 		exit(1);
 	default:				/* Parent. */
@@ -558,13 +565,16 @@ mountfs(const char *vfstype, const char 
 	mnt_argv.c = -1;
 	append_arg(&mnt_argv, execname);
 	mangle(optbuf, &mnt_argv);
+	if (mountprog != NULL)
+		strcpy(execname, mountprog);
+
 	append_arg(&mnt_argv, strdup(spec));
 	append_arg(&mnt_argv, strdup(name));
 	append_arg(&mnt_argv, NULL);
 
 	if (debug) {
 		if (use_mountprog(vfstype))
-			printf("exec: mount_%s", vfstype);
+			printf("exec: %s", execname);
 		else
 			printf("mount -t %s", vfstype);
 		for (i = 1; i < mnt_argv.c; i++)
@@ -681,7 +691,7 @@ catopt(char *s0, const char *s1)
 void
 mangle(char *options, struct cpa *a)
 {
-	char *p, *s;
+	char *p, *s, *val;
 
 	for (s = options; (p = strsep(&s, ",")) != NULL;)
 		if (*p != '\0') {
@@ -703,6 +713,22 @@ mangle(char *options, struct cpa *a)
 				 * before mountd starts.
 				 */
 				continue;
+			} else if (strncmp(p, "mountprog", 9) == 0) {
+				/*
+				 * "mountprog" is used to force the use of
+				 * userland mount programs.
+				 */
+				val = strchr(p, '=');
+                        	if (val != NULL) {
+                                	++val;
+					if (*val != '\0')
+						mountprog = strdup(val);
+				}
+
+				if (mountprog == NULL) {
+					errx(1, "Need value for -o mountprog");
+				}
+				continue;
 			} else if (strcmp(p, "userquota") == 0) {
 				continue;
 			} else if (strncmp(p, userquotaeq,
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"

Comment 22 Craig Rodrigues 2009-03-05 09:00:21 UTC

State Changed
From-To: open->patched

Committed a patch.

May I inquire exactly why -t is not sufficient to identify the mount program?

Why this complication by having to list it twice?

Comment 24 Craig Rodrigues 2009-03-24 03:05:11 UTC

State Changed
From-To: patched->closed

Committed to stable branch.