Re: which way to update export_args structure?

Brooks Davis wrote:
On Sat, Oct 20, 2018 at 01:17:37AM +0000, Rick Macklem wrote:
[lots of stuff snipped]
>> +     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).
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 the bufp.

[more stuff snipped]
>> +             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.
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 have to be
explicitly set 0/NULL in the compatibility code, but it there aren't currently any,
I may take this out. This code doesn't get executed frequently, so efficiency 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