Re: Some changes to autoconfiguration APIs

[Jason Thorpe <thorpej%me.com@localhost>]

> On Apr 25, 2021, at 4:26 AM, Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
> 
>> Date: Sat, 17 Apr 2021 17:10:44 -0700
>> From: Jason Thorpe <thorpej%me.com@localhost>
>> 
>> These changes simplify the API by removing a jumble of
>> config_{search,found}_this_that_and_the_other_thing() and going back
>> to just having config_search() and config_found(), but adding
>> tag-value variadic arguments to pass along additional information
>> when necessary.  This makes it easier to simplify the vast majority
>> of call sites, while also making it easier to add additional
>> parameters later as needed.
> 
> I tend to agree the pile of config_{search,found}_xyzzy() functions is
> confusing and I always have to pore through the man page to figure out
> which one means what.
> 
> (It might be nice to see a worked example of how this improves the API
> usage -- from a cursory skim of the branch's mechanical changes it's
> not clear to me, but I expect you have some highlights or something
> more in mind.)

If nothing else, you don't have to remember which order the arguments come in.  The new API also means you don't have to switch from e.g. config_found_ia() to config_found_sm_loc() just because you want to pass a new bit of info.

> However, tag/value variadic arguments lose the opportunity for type
> checking, and invite extremely mysterious failure modes from simple
> mistakes that the compiler won't detect, like missing CFARG_EOL, or
> missing a value next to a tag (which has already happened in the
> branch).  I don't think we should regress into that territory -- C has
> a type system, if a feeble one, and we should take advantage of it.

Actually, it's my experience that if you mess this up, you find out pretty quickly.  You're right, you see it at run-time, but you definitely see it (and the failures are extremely obvious when they happen).

> Here are two alternative approaches that also don't involve a
> menagerie of config_{search,found}_xyzzy functions:
> 
> 1. C99 initializers:
> 
> 	config_found(self, whatever, (const struct cfargs *){
> 	    .search = ...,
> 	    .locators = ...,
> 	})
> 
>   Privately you objected that this makes ABI extensions harder, but
>   we already have a workable compat mechanism with __RENAME and a
>   struct cfargs_v1.  (We also don't have a stable kernel ABI (yet)
>   anyway, so this is a bit of a moot point at the moment.)
> 
> 2. pthread_attr-style initialization functions:
> 
> 	struct cfargs cfa;
> 
> 	cfargs_init(&cfa);
> 	cfargs_search(&cfa, ...);
> 	cfargs_locators(&cfa, ...);
> 
> 	config_found(self, whatever, &cfa);

I don't like these approaches as much for two reasons: aesthetic (they're more complicated in the simple cases), and that they increase the ABI surface.

What I mean by the latter ... these two suggestions expose the cfargs structure size / layout to clients of the API, meaning that new optional cfargs fields break the ABI of callers who would otherwise be unaffected.  It has bothered me for years that this is the situation in NetBSD (where the layout of some internal kernel structure causes a version bump for e.g. network driver modules), and it seems dumb to keep piling it on.  Certainly, with my scheme, the equivalent of a "shlib minor bump" would be required so shield new consumers from old kernels, but existing modules would keep working.  Maybe I'm just spitting into the wind here.... As you said, we don't have a stable kernel ABI at the moment; I'm just looking to not make the problem worse.

Not really sure __RENAME is the solution we want to be crowing about.  I mean, it works I guess, and it's a tool in the toolbox, but not a particularly elegant one.  Not having to rename (or version) the symbol in the first place is better, IMO.

> One other concern:
> 
>   Author: thorpej <thorpej%NetBSD.org@localhost>
>   Date:   Sun Mar 21 17:35:39 2021 +0000
> 
>       CFARG_IATTR usage audit:
> 
>       If a device carries only one interface attribute, there is no need
>       to specify it when calling config_search(); that specification is
>       meant only to disambiguate which interface attribute (which is a
>       proxy for "what kind of attach args are being used") is having
>       children attached.  cfparent_match() will take care of ensuring that
>       any potential children can attach to one of the parent's iterface
>       attributes, and if the parent only carries one, no disambiguation is
>       necessary.
> 
> It's true that disambiguation isn't necessary, but it seems to me that
> this kind of mechanical change makes future errors more likely: if we
> ever add a new interface attribute with a different attach_args type,
> this seems like asking for trouble.

...and the code is written such that if a driver becomes inconsistent 

> What will happen if someone adds a new IA to a bus that had the IAs
> removed because no disambiguation was necessary, but doesn't find all
> the places where the IAs were removed?

Then the code I wrote will catch that.  My point was to make places that don't need to use complex API use simplified API.  It was REALLY obvious that there was a lot of blind copy-pasta lurking around as I audited everything.

> I've been thinking that it would be nice if, instead of writing the IA
> names literally as strings (and having multiple places for potential
> typos), we had a #define or something generated by config(8) for them,
> so that we could get more rather than less cross-checking of the IA
> names.

Feeeex eeeet!

> It would also be nice if we had formalized IA attach_args types --
> ideally, we would have a system where xyzzy_attach(...) and
> config_found(...) could type-check the IA attach_args pointer rather
> than losing it into a haze of void * everywhere.  Mismatched
> attach_args types is a class of bugs I've run into in the past and it
> doesn't seem like it should be terribly difficult to stamp out that
> entire class.

Sure, config could generate stubs for each iattr, and drivers could be modified to call them.  Or there could be another approach... but before we go there, for modules' sake, it would be good to develop a module / ABI versioning strategy.

-- thorpej