Date: Mon, 22 Oct 2018 21:24:06 +0000 From: Rick Macklem <rmacklem@uoguelph.ca> To: Brooks Davis <brooks@freebsd.org> Cc: FreeBSD Current <freebsd-current@FreeBSD.org>, Josh Paetzel <josh@tcbug.org> Subject: Re: which way to update export_args structure? Message-ID: <YTOPR0101MB1162CABEA06CC78BB5D5DA63DDF40@YTOPR0101MB1162.CANPRD01.PROD.OUTLOOK.COM> In-Reply-To: <20181022160508.GB45769@spindle.one-eyed-alien.net> References: <YTOPR0101MB182021549F8CF8277477A4C5DDE90@YTOPR0101MB1820.CANPRD01.PROD.OUTLOOK.COM> <20181003155133.GA57729@spindle.one-eyed-alien.net> <YTOPR0101MB18207FF98DED0232B9BB1B4FDDE50@YTOPR0101MB1820.CANPRD01.PROD.OUTLOOK.COM> <20181008170428.GB9766@spindle.one-eyed-alien.net> <YTOPR0101MB11626B32F73B520FBDA3C633DDFA0@YTOPR0101MB1162.CANPRD01.PROD.OUTLOOK.COM>, <20181022160508.GB45769@spindle.one-eyed-alien.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Brooks Davis wrote:
On Sat, Oct 20, 2018 at 01:17:37AM +0000, Rick Macklem wrote:
[lots of stuff snipped]
>> + if (error =3D=3D 0) {
>> + gotexp =3D 0;
>> + memset(&export, 0, sizeof(export));
>> + if (vfs_getopt(mp->mnt_optnew, "export.exflags", &bufp,
>> + &len) =3D=3D 0) {
>> + gotexp =3D 1;
>> + export.ex_flags =3D strtouq(bufp, &endptr, 0);
>> + if (endptr =3D=3D bufp)
>> + export_error =3D 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).
Thanks for noticing this. I was "on the fence" w.r.t. whether the arguments=
should
all be architecture neutral (ascii representation or ???).
(I didn't bother doing the code for the net addresses in ascii notation.)
It looks like you don't see that as needed, so I will just pass the binary =
numbers in.
(We could put them in Big Endian order, if someone thinks that is necessary=
?)
>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.
Heh, heh. That would be a sneaky hack. I like it, but will resist and use t=
he bufp.
[more stuff snipped]
>> + if (vfs_getopt(mp->mnt_optnew, "export", &bufp, &len) =3D=
=3D 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.
I'll be looking at the code to-day, but I think there is at least one field
(ex_suppgroups) that needs to be set NULL so that the free() won't blow up.
(I tend to bzero()/memset(0) so that any future additional fields don't hav=
e to be
explicitly set 0/NULL in the compatibility code, but it there aren't curren=
tly any,
I may take this out. This code doesn't get executed frequently, so efficien=
cy at
the "save a few instructions" level doesn't matter, imho.)
>> + memset(&o2export, 0, sizeof(o2export));
>
>This is certainly redundant given that you immediately copy over it.
Yep. This was just cruft left over from a previous version of the patch.
>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.
Cool. I'll finish the patch to-day and then let Josh work on mountd.c.
Thanks for your comments, rick
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?YTOPR0101MB1162CABEA06CC78BB5D5DA63DDF40>