Re: cvs commit: src/sbin/umount umount.c

Ian Dowse wrote (2003/11/16):
>   Modified files:
>     sbin/umount          umount.c 
>   Log:
>   If the unmount by file system ID fails, don't warn before retrying
>   a non-fsid unmount if the file system ID is all zeros. This is a
>   temporary workaround for warnings that occur in the vfs.usermount=1
>   case because non-root users get a zeroed filesystem ID. I have a
>   more complete fix in the works, but I won't get it done for 5.2.
>   Revision  Changes    Path
>   1.41      +4 -1      src/sbin/umount/umount.c

Hello and thanks for fixing this! I had a plan to report this, but you
were faster :o) I'm interested in this area - please, can you tell, what
do you plan to do in your more complete fix?

When I looked at this issue, I thought about some things:

* Why is f_fsid zeroed for non-root users at all? Is there any reason?
  Maybe it would be good idea to document it in
  /usr/src/lib/libc/sys/getfsstat.2 and /usr/src/lib/libc/sys/statfs.2 -
  so one small point for Tim:

Tim J. Robbins wrote (2003/11/15):
>   Modified files:
>     lib/libc/sys         statfs.2
>   Log:
>   Resync. struct statfs and flag definitions with sys/mount.h.
>   Revision  Changes    Path
>   1.22      +57 -22    src/lib/libc/sys/statfs.2

* Tim, can you please update /usr/src/lib/libc/sys/getfsstat.2 to reflect
  current sys/mount.h too, as you did for /usr/src/lib/libc/sys/statfs.2?
  Manual page getfsstat.2 lists struct statfs too and there are still old
  fields and values like MNAMELEN is 90 and so on. And if there are no
  objections, is it possible to add sentence "Non-root users get always
  f_fsid zeroed." (with better english...) to both manual pages?

* There are small typos in umount.c:

--- umount.c.orig	Tue Nov 18 16:24:43 2003
+++ umount.c	Tue Nov 18 16:24:54 2003
_at__at_ -365,7 +365,7 _at__at_
 			warn("unmount of %s failed", sfs->f_mntonname);
 		if (errno != ENOENT)
 			return (1);
-		/* Compatability for old kernels. */
+		/* Compatibility for old kernels. */
 		warnx("retrying using path instead of file system ID");
 		if (unmount(sfs->f_mntonname, fflag) != 0) {
 			warn("unmount of %s failed", sfs->f_mntonname);
_at__at_ -557,7 +557,7 _at__at_
 }
 
 /*
- * Convert a hexidecimal filesystem ID to an fsid_t.
+ * Convert a hexadecimal filesystem ID to an fsid_t.
  * Returns 0 on success.
  */
 int
---

* Do you understand, why there is line in umount.c:376
  getmntentry(NULL, NULL, &sfs->f_fsid, REMOVE)
  ? I'm not sure, but if it is needed for some reason,
  I think that there should be used different getmntentry() according
  to the used unmount() method, like in this patch:

--- umount.c.orig	Tue Nov 18 14:59:00 2003
+++ umount.c	Tue Nov 18 15:26:59 2003
_at__at_ -370,10 +370,14 _at__at_
 		if (unmount(sfs->f_mntonname, fflag) != 0) {
 			warn("unmount of %s failed", sfs->f_mntonname);
 			return (1);
+		} else {
+			/* Mark this this file system as unmounted. */
+			getmntentry(NULL, sfs->f_mntonname, NULL, REMOVE);
 		}
+	} else {
+		/* Mark this this file system as unmounted. */
+		getmntentry(NULL, NULL, &sfs->f_fsid, REMOVE);
 	}
-	/* Mark this this file system as unmounted. */
-	getmntentry(NULL, NULL, &sfs->f_fsid, REMOVE);
 	if (vflag)
 		(void)printf("%s: unmount from %s\n", sfs->f_mntfromname,
 		    sfs->f_mntonname);
---

* /usr/src/sbin/mount/mount.c: If user uses mount -v, it prints false
  zeroed fsids - isn't it better to print just non-zero fsids, so that
  nobody is "mystified"? I have created two patches - I do not know
  which do you consider as a better:

--- mount.c.orig	Tue Nov 18 14:46:24 2003
+++ mount.c	Tue Nov 18 14:50:46 2003
_at__at_ -535,9 +535,11 _at__at_
 		if (sfp->f_syncreads != 0 || sfp->f_asyncreads != 0)
 			(void)printf(", reads: sync %ld async %ld",
 			    sfp->f_syncreads, sfp->f_asyncreads);
-		printf(", fsid ");
-		for (i = 0; i < sizeof(sfp->f_fsid); i++)
-			printf("%02x", ((u_char *)&sfp->f_fsid)[i]);
+		if (sfp->f_fsid.val[0] != 0 || sfp->f_fsid.val[1] != 0) {
+			printf(", fsid ");
+			for (i = 0; i < sizeof(sfp->f_fsid); i++)
+				printf("%02x", ((u_char *)&sfp->f_fsid)[i]);
+		}
 	}
 	(void)printf(")\n");
 }
---

  or

--- mount.c.orig	Tue Nov 18 14:46:24 2003
+++ mount.c	Tue Nov 18 14:57:30 2003
_at__at_ -508,7 +508,7 _at__at_
 prmount(sfp)
 	struct statfs *sfp;
 {
-	int flags, i;
+	int flags, i, fsid;
 	struct opt *o;
 	struct passwd *pw;
 
_at__at_ -535,9 +535,18 _at__at_
 		if (sfp->f_syncreads != 0 || sfp->f_asyncreads != 0)
 			(void)printf(", reads: sync %ld async %ld",
 			    sfp->f_syncreads, sfp->f_asyncreads);
-		printf(", fsid ");
-		for (i = 0; i < sizeof(sfp->f_fsid); i++)
-			printf("%02x", ((u_char *)&sfp->f_fsid)[i]);
+		fsid = 0;
+		for (i = 0; i < sizeof(sfp->f_fsid); i++) {
+			if (((u_char *)&sfp->f_fsid)[i] != 0) {
+				fsid = 1;
+				break;
+			}
+		}
+		if (fsid) {
+			printf(", fsid ");
+			for (i = 0; i < sizeof(sfp->f_fsid); i++)
+				printf("%02x", ((u_char *)&sfp->f_fsid)[i]);
+		}
 	}
 	(void)printf(")\n");
 }
---

Thanks.

-- 
Rudolf Cejka <cejkar at fit.vutbr.cz> http://www.fit.vutbr.cz/~cejkar
Brno University of Technology, Faculty of Information Technology
Bozetechova 2, 612 66  Brno, Czech Republic