mountd(8): parsecred(): uid:gid:... loop: Simplify a bit

[olce]

Summary

No functional change intended.

[rmacklem]

usr.sbin/mountd/mountd.c

3706

Since it adds a local variable and results in more
lines of code, I'll admit I do not see why it simplifies
the loop.

However, it does appear to be semantically equivalent,
so I have no problem with the change.

Maybe others can comment w.r.t. which form of the
loop they think is preferable?

[rmacklem]

usr.sbin/mountd/mountd.c

3708

Ok, it does appear that this case could result in cr_ngroups == 0.
I'd suggest adding a couple of lines here like:

else if (cr->cr_ngroups == 0) {

cr->cr_group[0] = GID_NOGROUP;
cr->cr_ngroups = 1;

}

[olce]

usr.sbin/mountd/mountd.c

3706

I introduced a new local variable to avoid duplicating the groups[cr->cr_ngroups++] = statement. Having only one of it makes it clearer (at least to me) that some slot of groups is assigned to group once per loop (provided we reach the end of the block). But this is marginal, and perhaps debatable.

The real simplification, in my mind, is rather getting rid of the extra if after the loop with same first conditions at those of the loop guard. In the new variant, it is much clearer (YMMV) and easier to figure out when exactly syslog(LOG_ERR, ...) is called and that this is another termination constraint for the loop.

3708

Yes, see also my comment in D46918. I think we should rather do nothing here, have nmount(2) accept an export with an empty array of groups (for compatibility) and just use the configured NFSD_VNET(nfsrv_defaultgid) as the fallback group, which defaults to GID_NOGROUP. Unless the purpose for NFSD_VNET(nfsrv_defaultgid) is different than what I suspect it is?

[rmacklem]

usr.sbin/mountd/mountd.c

3708

The "purpose" is mostly historical (and the fact that
the code was originally written to be portable across
the *BSDen).
nfsrv_defaultgid is set by nfsuserd when it is started
up. nfsuserd sets it to the gid for "nogroup".
--> The only way it becomes different than GID_NOGROUP

is if a site changes the numeric value for "nogroup" in
their group database and then runs nfsuserd.

Since many sites do not run nfsuserd, it might be better
to do a getgrpname("nogroup") and use the gid retuned,
if it succeeds, with GID_NOGROUP being the default otherwise.
(This handles the case where nfsuserd is not run consistently
with the behaviour when nfsuserd is run.)

[olce]

usr.sbin/mountd/mountd.c

3708

I see. This might be better indeed. Will update this revision to use getgrpnam_r().

This doesn't remove the need to have a fallback in the kernel in case no group is passed at all in ex_groups for compatibility (or we just decide to break it, and have nmount(2) export calls fail in this case), just to ensure that the struct ucred in field netc_anon is guaranteed to be correctly filled.

[rmacklem]

usr.sbin/mountd/mountd.c

3685

Yet another way to fix this would be to add this

if (*names == '\0') {
    syslog(LOG_ERR, "no group after colon");
    return;
 }

ie. Consider a case like "-maproot=1234:" an invalid one.

[olce]

usr.sbin/mountd/mountd.c

3685

I like that idea. Didn't propose it as it may break compatibility with existing configurations. If that's fine for you, I can do it for -CURRENT.

Still, I think I have to keep some fully compatible solutions as I intend all these changes to be MFCed, and for stable/13 and stable/14 breaking this compatibility is most probably not acceptable?