cvs commit: src/sbin/fsck_ffs main.c

[Yar Tikhiy]

On Wed, Mar 05, 2008 at 10:24:02PM +0000, Craig Rodrigues wrote:
> On Wed, Mar 05, 2008 at 03:20:29PM +0300, Yar Tikhiy wrote:
> > Your analysis of the problem sounds not quite correct to me.
> 
> You make some interesting points in your e-mail.
> I suggest that you summarize the points and
> post them to arch@ for further review and discussion.

My points are directly related to your commit.  To illustrate them
clearlier, I instrumented vfs_mount.c with the patch shown in the
attachment and investigated how fsck_ffs now affects the root mount
options.  My findings agree with my prognosis; they are as follows.

When fsck_ffs just invokes nmount(2), the initial mount options on
the root are:

	fstype, fspath, from.

Just before nmount() returns, the mount options on the root become:

	fstype, from, fspath, errmsg, update, ro.

Prior to your commit the final options were:

	fstype, from, fspath, errmsg.

Thus, due to your commit, fsck_ffs now sets two additional string
options on the root mount, "ro" and "update".  Both are permanent,
i.e., they remain there after fsck_ffs exited.  I maintain that:

1) Setting "ro" from fsck_ffs is but a hack needed because the
   option cannot be set on the root mount at boot time due
   to bugs.  The hack is tolerable, but you failed to mark it
   clearly as a temporary workaround, e.g., using a reasonable
   XXX-style comment providing enough details.

2) Setting "update" as a _permanent_ option on the mount point is
   just wrong because "update" isn't one.  The nmount(2) code cannot
   cope with "update" as a string option yet; it still needs to be
   spelled MNT_UPDATE.  And, to the best of my knowledge, there is
   no problem with still using that bit flag, although you hint at
   some `weird things'.  This part of the change should be reverted.

   (Another approach could be to fix vfs_donmount(), but that would
   require the architectural decision be made on whether nmount(2)
   modifiers are spelled as string options or bit flags.  Currently
   no FS code tries to check for "update", and "reload" isn't there
   yet, so the issue is still pending.  Feel free to conduct a thread
   on -arch.)

Now I expect you either prove my points wrong using technical facts,
or react to them by the respective commit(s) to fsck_ffs.  Otherwise
I'll be forced to request fsck_ffs/main.c rev. 1.49 be backed out
and PR 120319 be reopened.

Thanks!

-- 
Yar

P.S. Here's how I instrumented vfs_mount.c:

--- vfs_mount.c.orig	2008-02-19 20:04:36.000000000 +0300
+++ vfs_mount.c	2008-03-10 12:29:30.000000000 +0300
@@ -324,6 +324,19 @@
 	return (error);
 }
 
+static void
+vfs_printopts(struct vfsoptlist *opts)
+{
+	struct vfsopt *opt;
+	int first = 1;
+
+	TAILQ_FOREACH(opt, opts, link) {
+		printf("%s%s", first ? "" : " ", opt->name);
+		if (first)
+			first = 0;
+	}	
+}
+
 /*
  * Merge the old mount options with the new ones passed
  * in the MNT_UPDATE case.
@@ -948,7 +961,16 @@
 		MNT_IUNLOCK(mp);
 		VOP_UNLOCK(vp, 0);
 		mp->mnt_optnew = fsdata;
+		printf(">>> opts before: ");
+		vfs_printopts(mp->mnt_opt);
+		printf("\n");
+		printf(">>> opts new: ");
+		vfs_printopts(mp->mnt_optnew);
+		printf("\n");
 		vfs_mergeopts(mp->mnt_optnew, mp->mnt_opt);
+		printf(">>> opts merged: ");
+		vfs_printopts(mp->mnt_optnew);
+		printf("\n");
 	} else {
 		/*
 		 * If the user is not root, ensure that they own the directory
@@ -1027,6 +1049,9 @@
 		if (mp->mnt_opt != NULL)
 			vfs_freeopts(mp->mnt_opt);
 		mp->mnt_opt = mp->mnt_optnew;
+		printf(">>> opts final: ");
+		vfs_printopts(mp->mnt_opt);
+		printf("\n");
 		(void)VFS_STATFS(mp, &mp->mnt_stat, td);
 	}
 	/*