Re: which way to update export_args structure?

Brooks Davis wrote:
> Yes, I think that's the right way foward.  Thanks for following up.
>Rick Macklem wrote:
>> Just in case you missed it in the email thread, in your general question below...
>> Did you mean/suggest that the fields of "struct export_args" be passed in as
>> separate options to nmount(2)?
>>
>> This sounds like a reasonable idea to me and I can ping Josh Paetzel w.r.t. the
>> changes to mountd.c to do it. (We are still in the testing stage for the updated
>> struct, so we might as well get that working first.)
>>
Well, Josh and I now have the code working via. passing the export_args
structure into the kernel using the "export"  nmount(2) option.

I have coded a partial patch (not complete nor tested) to pass the fields in as
separate nmount(2) arguments. Since the patch has gotten fairly large
already, I wanted to see if people do think this is the correct approach.
(I'll admit I don't understand why having the arguments would matter, given
 that only mountd does it. Would anyone run a 32bit mountd on a 64bit kernel?)

Anyhow, here's the partial patch showing the main changes when going from
passing in "struct export_args" to passing in separate fields:

--- kern/vfs_mount.c.nofsid2	2018-10-16 23:45:33.540348000 -0400
+++ kern/vfs_mount.c	2018-10-19 20:01:14.927370000 -0400
_at__at_ -277,6 +277,7 _at__at_ vfs_buildopts(struct uio *auio, struct v
 	size_t memused, namelen, optlen;
 	unsigned int i, iovcnt;
 	int error;
+	char *cp;
 
 	opts = malloc(sizeof(struct vfsoptlist), M_MOUNT, M_WAITOK);
 	TAILQ_INIT(opts);
_at__at_ -325,7 +326,7 _at__at_ vfs_buildopts(struct uio *auio, struct v
 		}
 		if (optlen != 0) {
 			opt->len = optlen;
-			opt->value = malloc(optlen, M_MOUNT, M_WAITOK);
+			opt->value = malloc(optlen + 1, M_MOUNT, M_WAITOK);
 			if (auio->uio_segflg == UIO_SYSSPACE) {
 				bcopy(auio->uio_iov[i + 1].iov_base, opt->value,
 				    optlen);
_at__at_ -335,6 +336,8 _at__at_ vfs_buildopts(struct uio *auio, struct v
 				if (error)
 					goto bad;
 			}
+			cp = (char *)opt->value;
+			cp[optlen] = '\0';
 		}
 	}
 	vfs_sanitizeopts(opts);
_at__at_ -961,6 +964,8 _at__at_ vfs_domount_update(
 	int error, export_error, i, len;
 	uint64_t flag;
 	struct o2export_args o2export;
+	char *endptr;
+	int gotexp;
 
 	ASSERT_VOP_ELOCKED(vp, __func__);
 	KASSERT((fsflags & MNT_UPDATE) != 0, ("MNT_UPDATE should be here"));
_at__at_ -1033,36 +1038,117 _at__at_ vfs_domount_update(
 
 	export_error = 0;
 	/* Process the export option. */
-	if (error == 0 && vfs_getopt(mp->mnt_optnew, "export", &bufp,
-	    &len) == 0) {
-		/* Assume that there is only 1 ABI for each length. */
-		switch (len) {
-		case (sizeof(struct oexport_args)):
-		case (sizeof(struct o2export_args)):
-			memset(&export, 0, sizeof(export));
-			memset(&o2export, 0, sizeof(o2export));
-			memcpy(&o2export, bufp, len);
-			export.ex_flags = (u_int)o2export.ex_flags;
-			export.ex_root = o2export.ex_root;
-			export.ex_anon = o2export.ex_anon;
-			export.ex_addr = o2export.ex_addr;
-			export.ex_addrlen = o2export.ex_addrlen;
-			export.ex_mask = o2export.ex_mask;
-			export.ex_masklen = o2export.ex_masklen;
-			export.ex_indexfile = o2export.ex_indexfile;
-			export.ex_numsecflavors = o2export.ex_numsecflavors;
-			for (i = 0; i < MAXSECFLAVORS; i++)
-				export.ex_secflavors[i] =
-				    o2export.ex_secflavors[i];
-			export_error = vfs_export(mp, &export);
-			break;
-		case (sizeof(export)):
-			bcopy(bufp, &export, len);
-			export_error = vfs_export(mp, &export);
-			break;
-		default:
-			export_error = EINVAL;
-			break;
+	if (error == 0) {
+		gotexp = 0;
+		memset(&export, 0, sizeof(export));
+		if (vfs_getopt(mp->mnt_optnew, "export.exflags", &bufp,
+		    &len) == 0) {
+			gotexp = 1;
+			export.ex_flags = strtouq(bufp, &endptr, 0);
+			if (endptr == bufp)
+				export_error = EINVAL;
+		}
+		if (vfs_getopt(mp->mnt_optnew, "export.root", &bufp,
+		    &len) == 0) {
+			gotexp = 1;
+			export.ex_root = strtouq(bufp, &endptr, 0);
+			if (endptr == bufp)
+				export_error = EINVAL;
+		}
+		if (vfs_getopt(mp->mnt_optnew, "export.anonuid", &bufp,
+		    &len) == 0) {
+			gotexp = 1;
+			export.ex_anon.cr_uid = strtouq(bufp, &endptr, 0);
+			if (endptr != bufp)
+				export.ex_anon.cr_version = XUCRED_VERSION;
+			else
+				export_error = EINVAL;
+		}
+		if (vfs_getopt(mp->mnt_optnew, "export.anongroups", &bufp,
+		    &len) == 0) {
+			gotexp = 1;
+			export.ex_anon.cr_ngroups = len / sizeof(gid_t);
+			if (export.ex_anon.cr_ngroups > XU_NGROUPS) {
+				export.ex_suppgroups = mallocarray(
+				    sizeof(gid_t),
+				    export.ex_anon.cr_ngroups, M_TEMP,
+				    M_WAITOK);
+				memcpy(export.ex_suppgroups, bufp, len);
+			} else
+				memcpy(export.ex_anon.cr_groups, bufp,
+				    len);
+		}
+		if (vfs_getopt(mp->mnt_optnew, "export.addr", &bufp,
+		    &len) == 0) {
+			gotexp = 1;
+			export.ex_addr = malloc(len, M_TEMP, M_WAITOK);
+			memcpy(export.ex_addr, bufp, len);
+			export.ex_addrlen = len;
+		}
+		if (vfs_getopt(mp->mnt_optnew, "export.mask", &bufp,
+		    &len) == 0) {
+			gotexp = 1;
+			export.ex_mask = malloc(len, M_TEMP, M_WAITOK);
+			memcpy(export.ex_mask, bufp, len);
+			export.ex_masklen = len;
+		}
+		if (vfs_getopt(mp->mnt_optnew, "export.indexfile", &bufp,
+		    &len) == 0) {
+			gotexp = 1;
+			export.ex_indexfile = malloc(len + 1, M_TEMP,
+			    M_WAITOK);
+			memcpy(export.ex_indexfile, bufp, len);
+			export.ex_indexfile[len] = '\0';
+		}
+		if (vfs_getopt(mp->mnt_optnew, "export.secflavors", &bufp,
+		    &len) == 0) {
+			gotexp = 1;
+			export.ex_numsecflavors = len / sizeof(uint32_t);
+			if (export.ex_numsecflavors <= MAXSECFLAVORS)
+				memcpy(export.ex_secflavors, bufp, len);
+			else
+				export_error = EINVAL;
+		}
+		if (vfs_getopt(mp->mnt_optnew, "export.fsid", &bufp,
+		    &len) == 0) {
+			gotexp = 1;
+			export.ex_fsid = strtouq(bufp, &endptr, 0);
+			if (endptr == bufp)
+				export_error = EINVAL;
+		}
+		if (vfs_getopt(mp->mnt_optnew, "export", &bufp, &len) == 0) {
+			/* Assume that there is only 1 ABI for each length. */
+			switch (len) {
+			case (sizeof(struct oexport_args)):
+			case (sizeof(struct o2export_args)):
+				memset(&export, 0, sizeof(export));
+				memset(&o2export, 0, sizeof(o2export));
+				memcpy(&o2export, bufp, len);
+				export.ex_flags = (u_int)o2export.ex_flags;
+				export.ex_root = o2export.ex_root;
+				export.ex_anon = o2export.ex_anon;
+				export.ex_addr = o2export.ex_addr;
+				export.ex_addrlen = o2export.ex_addrlen;
+				export.ex_mask = o2export.ex_mask;
+				export.ex_masklen = o2export.ex_masklen;
+				export.ex_indexfile = o2export.ex_indexfile;
+				export.ex_numsecflavors = o2export.ex_numsecflavors;
+				for (i = 0; i < MAXSECFLAVORS; i++)
+					export.ex_secflavors[i] =
+					    o2export.ex_secflavors[i];
+				export_error = vfs_export(mp, &export);
+				break;
+			default:
+				export_error = EINVAL;
+				break;
+			}
+		} else if (gotexp != 0) {
+			if (export_error == 0)
+				export_error = vfs_export(mp, &export);
+			free(export.ex_addr, M_TEMP);
+			free(export.ex_mask, M_TEMP);
+			free(export.ex_indexfile, M_TEMP);
+			free(export.ex_suppgroups, M_TEMP);
 		}
 	}
 
So, what to people think about this? rick