which way to update export_args structure?
Brooks Davis
brooks at freebsd.org
Mon Oct 22 16:05:17 UTC 2018
On Sat, Oct 20, 2018 at 01:17:37AM +0000, Rick Macklem wrote:
> 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
> @@ -277,6 +277,7 @@ 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);
> @@ -325,7 +326,7 @@ 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);
> @@ -335,6 +336,8 @@ vfs_buildopts(struct uio *auio, struct v
> if (error)
> goto bad;
> }
> + cp = (char *)opt->value;
> + cp[optlen] = '\0';
> }
> }
> vfs_sanitizeopts(opts);
> @@ -961,6 +964,8 @@ 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"));
> @@ -1033,36 +1038,117 @@ 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;
> + }
This pattern involving strtouq seems wrong to me. We should probably
pass a pointer to the integer type (which should always be an explicitly
sized type).
If it didn't contradict the first sentence of the description in
vfs_getopt.9, I'd say we should pass int and long values in the length
with a NULL buffer.
> + 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));
I think this is now redundant.
> + memset(&o2export, 0, sizeof(o2export));
This is certainly redundant given that you immediately copy over it.
> + 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
This is the direction I'd been thinking. FWIW, the usecase is more that
once you've moved away from the struct it's easy to make incremental
changes then to use a 32-bit mountd on a 64-bit kernel. Moving toward
size-independent interfaces helps both causes though.
-- Brooks
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-current/attachments/20181022/d3ff4677/attachment.sig>
More information about the freebsd-current
mailing list