If you move the final assignment to begin_envv after the last add_arg_str() call, the functions become almost identical, am I right ? The common code can be moved in the helper.

The begin_envvp member is managed outside the functions already, so I do not see a problem with an encapsulation.

The (void *) cast seems redundant? argp is a char * and the second argument to exec_args_add_arg() is a char *.

Same with this (void *) cast.

imgact_binmisc.c removed the bcopy() and all the math here, but imgact_shell.c is keeping it?

Should the (uintptr_t) cast be kept since argp is u_long? (The freebsd32 approach was a bit cleaner and would have a single 'u_long arg' that fueword() read that was then converted to a 'char *' argp or envp before this function removing the need for the cast in the function call, but possibly requiring a (uintptr_t) cast of the u_long before it was assigned to the (char *).)

s/possiably/possibly/

Thanks for catching this one. The broken code booted to multi-user, but was subtly broken (dhclient failed to configure the interface). I guess that goes to who that booting is less of a test then one might thing.

This seems to be not handled (or answered). Even with argp becoming char *, why do we need to cast between pointers using intermediate integer type ?

() are excessive.

Assert this ?

This is actually the file-scope reserved identifier, according to C11 7.1.3 Reserved identifiers.

C requires the cast through uintptr_t as non-intptr_t types may not be cast to pointers. To the extent that it's possible to read the standard another way, WG14 is working to correct that in future standards.

This a reasonably common idiom in our kernel for "internal use only." Does a struct member even count as an identifier?

6.3.2.3 Pointers states directly 'An integer may be converted to any pointer type.' I somehow doubt that this can be changed without breaking too much of compatibility.

6.2.1 Scopes of identifiers
1 An identifier can denote an object; a function; a tag or a member of a structure, union, or enumeration; a typedef name; a label name; a macro name; or a macro parameter.
So from my reading of the standard, member names are definitely identifiers.

We are sloppy with the proper namespacing, but I do not see a reason to add more of that.

Do you an alternative suggestion for marking members "internal use only?"

Just state so in the comment perhaps ? After all, this is kernel and the set of consumers of the structure is controllable. We do not need to make it controlled access like c++.

I'd prefer to keep the (uintptr_t) cast here as it was in the original. It doesn't matter in practice since C isn't used to manipulate the pointer in question, but it's the right thing and consistent with the style in this file

Re 'An integer may be converted to any pointer type.' future C standards will likely include a notion of pointer provenance where a pointer must be relative to some object and without out the cast through (uintptr_t) the compiler will be free to assume that random integers cast to pointers have no object backing and to treat access as UB. There will be ways to switch this off (particularly since Linux relies heavily on long->void * casts, but we should avoid adding dependencies on this).