Re: Some changes to autoconfiguration APIs

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

> 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.)

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.

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);


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.

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?

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.

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.