I did not try to read all of the discussion in D46918.

My main question here is, is it so hard/messy to have the callers do this checking? It seems to me that if a non-empty groups array is a requirement of crsetgroups(9), nfs/rpc callers could do something like:

cr = crget();
if (ngrp != 0)
    crsetgroups(cr, ngrp, groups);
else
    cr->cr_groups[0] = nfs_fallback_gid;

This is based on the existing language in the crsetgroups(9) man page, stating:
"No other mechanism [than crsetgroups()] should be used to modify the cr_groups array except for updating the primary group via assignment to cr_groups[0]."

So, direct assignment to cr_groups[0] is legal with the established interface.

In D46917#1079835, @mhorne wrote:

My main question here is, is it so hard/messy to have the callers do this checking? It seems to me that if a non-empty groups array is a requirement of crsetgroups(9), nfs/rpc callers could do something like:

(elided)

So, direct assignment to cr_groups[0] is legal with the established interface.

crsetgroups() (and now, crsetgroups_empty_fallback()) is intended to be the sole way to initialize the groups related fields of struct ucred. This is not only my view, but also how I actually read crsetgroups(9). In particular, I interpret:

"No other mechanism [than crsetgroups()] should be used to modify the cr_groups array except for updating the primary group via assignment to cr_groups[0]."

as meaning that assigning to cr_groups[0] (or the cr_gid alias) is legal only after initialization, because of the verb "to modify".

I plan to amend the manpage to make it clearer on this front (later, in a separate commit).

We should really head to preserving abstractions here, in particular because supplementary groups need to be sorted and because of the existence of cr_smallgroups (which we might want to remove at some point). The fact that the code sketch you proposed is wrong, as it doesn't set cr_ngroups in the else part, also supports these views. Additionally, I don't see why we should duplicate code like this in all places. Even if there wasn't a cr_ngroups field to update, I would be quite reluctant to use an inline version as you propose, as even if it is somewhat more transparent, it is also more risky and makes it harder to evolve the implementation when needed (e.g., we might want to dissociate cr_gid from cr_groups[] one day, or might want to allow credentials without groups, etc.).

Unrelated question: Do you consider the new function's current name OK? I'm wondering if just crsetgroups_fallback() wouldn't be explicit enough already.

Renamed crsetgroups_empty_fallback() to crsetgroups_fallback() in my tree.