Re: Some changes to autoconfiguration APIs
> Also, is there some way to distinguish an integer valued attribute
> which is explicitly set to 0 from one which isn't set at all
Not using the initialized-struct paradigm; there is no way to tell the
difference between
(struct foo){...no mention of y...}
and
(struct foo){y=0, ...no mention of y...}
because they compile to the same bits.
This is actually an advantage of the method I sketched upthread of
using no-op functions for type-checking with a varargs API function:
"not provided" can be handled differently from "provided" regardless of
the value provided.
/~\ The ASCII Mouse
\ / Ribbon Campaign
X Against HTMLmo...@rodents-montreal.org
/ \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: Some changes to autoconfiguration APIs
> Date: Sat, 07 Aug 2021 22:59:02 +0700 > From: Robert Elz > > Date:Wed, 4 Aug 2021 17:52:46 -0700 > From:Jason Thorpe > Message-ID: <68ff8737-f347-4a7f-960b-9e4a6ca9e...@me.com> > > | It addresses the concerns about compile-time type checking > | by using an anonymous structure constructed in-line > > Is there something in the C definition of such things which guarantees > that the un-init'd fields all get set to 0/NULL ? Or is that just > happening because of the "const" spread all over - which might be causing > the compiler to allocate static storage, but which is not required of const? > > Also, is there some way to distinguish an integer valued attribute which > is explicitly set to 0 from one which isn't set at all (in which case we > might want to default it to some other value) or do we only ever use > attributes via various kinds of pointers (or perhaps never have any, > anywhere ever, where 0 is a sensible value) ? C11, Sec. 6.7.9 `Initialization', paragraph 19, p. 141: `The initialization shall occur in initializer list order, each initializer provided for a particular subobject overriding any previously listed initializer for the same subobject; all subobjects that are not initialized explicitly shall be initialized implicitly the same as objects that have static storage duration.' And paragraph 10, p. 140: `If an object that has static or thread storage duration is not initialized explicitly, then: `--- if it has pointer type, it is initialized to a null pointer; `--- if it has arithmetic tpye, it is initialized to (positive or unsigned) zero; `--- if it is an aggregate, every member is initialized (recursively) according to these rules, and any padding is initialized to zero bits; `--- if it is a union, the first named member is initialized (recursively) according to these rules, and any padding is initialized to zero bits.' So pointers are nulled and integers are zero'd here -- and const is not relevant, nor does the storage for these objects actually have static duration; it's just that the anonymous struct objects with designated initializers are initialized the same way as if they did have static storage duration.
Re: Some changes to autoconfiguration APIs
> On Aug 7, 2021, at 8:59 AM, Robert Elz wrote: > > Is there something in the C definition of such things which guarantees > that the un-init'd fields all get set to 0/NULL ? Yes, as part of designated initializers in C, omitted fields are initialized in the same manner as static storage. -- thorpej
Re: Some changes to autoconfiguration APIs
Date:Wed, 4 Aug 2021 17:52:46 -0700 From:Jason Thorpe Message-ID: <68ff8737-f347-4a7f-960b-9e4a6ca9e...@me.com> | It addresses the concerns about compile-time type checking | by using an anonymous structure constructed in-line Is there something in the C definition of such things which guarantees that the un-init'd fields all get set to 0/NULL ? Or is that just happening because of the "const" spread all over - which might be causing the compiler to allocate static storage, but which is not required of const? Also, is there some way to distinguish an integer valued attribute which is explicitly set to 0 from one which isn't set at all (in which case we might want to default it to some other value) or do we only ever use attributes via various kinds of pointers (or perhaps never have any, anywhere ever, where 0 is a sensible value) ? kre
Re: Some changes to autoconfiguration APIs
> On Aug 7, 2021, at 8:12 AM, Taylor R Campbell > wrote: > >> Date: Wed, 4 Aug 2021 17:52:46 -0700 >> From: Jason Thorpe >> >>> On Aug 1, 2021, at 11:10 AM, Martin Husemann wrote: >>> >>> On Sun, Aug 01, 2021 at 07:57:20AM -0700, Jason Thorpe wrote: The situation hasn't changed. I'm still waiting for concrete proposals. >>> >>> The concrete proposal is backout - if there is no better idea how to deal >>> with it properly. >> >> I have reworked it on the thorpej-cfargs2 branch. It addresses the >> concerns about compile-time type checking by using an anonymous >> structure constructed in-line with the help of a variadic >> preprocessor macro (to save wear and tear on your fingers and >> keyboard that might otherwise occur because of annoying boilerplate >> syntax to construct the structure in-line). > > Thanks, this looks much better! No more objections from me. Great, I’ll merge the branch today. -- thorpej
Re: Some changes to autoconfiguration APIs
> Date: Wed, 4 Aug 2021 17:52:46 -0700
> From: Jason Thorpe
>
> > On Aug 1, 2021, at 11:10 AM, Martin Husemann wrote:
> >
> > On Sun, Aug 01, 2021 at 07:57:20AM -0700, Jason Thorpe wrote:
> >> The situation hasn't changed. I'm still waiting for concrete proposals.
> >
> > The concrete proposal is backout - if there is no better idea how to deal
> > with it properly.
>
> I have reworked it on the thorpej-cfargs2 branch. It addresses the
> concerns about compile-time type checking by using an anonymous
> structure constructed in-line with the help of a variadic
> preprocessor macro (to save wear and tear on your fingers and
> keyboard that might otherwise occur because of annoying boilerplate
> syntax to construct the structure in-line).
Thanks, this looks much better! No more objections from me.
For future work (definitely not a blocker for this or netbsd-10 or
anything) it would be nice if we could incorporate typed interface
attributes rather than just strings that are copied & pasted into the
source code and never checked until runtime. For example, in
files.usb we could have something like
define usbdevif { [port = -1], ... }
which would lead config(8) to generate
static struct ifattachargs
IA_USBDEVIF(struct usb_attach_args *args)
{
return (struct ifattachargs) {
.iaa_name = "usbdevif",
.iaa_args = args,
};
}
and in usb_subr.c the code would be
dv = config_found(parent, IA_USBDEVIF(), ...);
with everything here verified by the compiler at compile-time.
Shouldn't be much effort to implement this -- just local changes in
config(8). (I did something similar in picopb, e.g.
and
.)
A stretch goal would be to push the types into the attach function
too, e.g.
int
uwhatever_attach(device_t parent, device_t self,
struct usb_attach_args *uaa)
{
...
}
but that might take a bit more work.
Re: Some changes to autoconfiguration APIs
> On Aug 4, 2021, at 10:19 PM, David Holland wrote: > > Seems like a definite improvement as long as .submatch and whatnot are > typed (and not (void *)). Yes, they’re typed. -- thorpej
Re: Some changes to autoconfiguration APIs
On Wed, Aug 04, 2021 at 05:52:46PM -0700, Jason Thorpe wrote: > Old example: > > c->c_dev = config_found(sc->sc_dev, , pciprint, > CFARG_SUBMATCH, config_stdsubmatch, > CFARG_LOCATORS, locs, > CFARG_DEVHANDLE, devhandle, > CFARG_EOL); > > New example: > > c->c_dev = config_found(sc->sc_dev, , pciprint, > CFARGS(.submatch = config_stdsubmatch, >.locators = locs, >.devhandle = devhandle)); > > Acceptable? Seems like a definite improvement as long as .submatch and whatnot are typed (and not (void *)). -- David A. Holland dholl...@netbsd.org
Re: Some changes to autoconfiguration APIs
> I have reworked it on the thorpej-cfargs2 branch. It addresses the concerns$
> Old example:
> c->c_dev = config_found(sc->sc_dev, , pciprint,
> CFARG_SUBMATCH, config_stdsubmatch,
> CFARG_LOCATORS, locs,
> CFARG_DEVHANDLE, devhandle,
> CFARG_EOL);
> New example:
> c->c_dev = config_found(sc->sc_dev, , pciprint,
> CFARGS(.submatch = config_stdsubmatch,
>.locators = locs,
>.devhandle = devhandle));
May or may not be useful here, but when I've wanted to do something
like that and have had concernes about type safety, I've done things
like (to map to this problem space)
c->c_dev = config_found(sc->sc_dev, , pciprint,
CFARG_SUBMATCH(config_stdsubmatch),
CFARG_LOCATORS(locs),
CFARG_DEVHANDLE(devhandle),
CFARG_EOL);
via macros like
#define CFARG__SUBMATCH 0x70001
#define CFARG_SUBMATCH(x) CFARG__SUBMATCH, cfarg_check_submatch((x))
extern int (*cfarg_check_submatch(int (*)(device_t, whatever)))(device_t,
whatever);
#define CFARG__LOCATORS 0x70002
#define CFARG_LOCATORS(x) CFARG__LOCATORS, cfarg_check_locators((x))
extern whatever *cfarg_check_locators(whatever *);
Then cfarg_check_* simply return their arguments directly:
int (*cfarg_check_submatch(int (*fn)(device_t, whatever)))(device_t, whatever)
{
return(fn);
}
whatever *cfarg_check_locators(whatever *arg)
{
return(arg);
}
(I usually pick sequential values some distance from zero for what here
are CFARG__*, to reduce the risk of hitting a valid value
accidentally.)
If the argument-checking functions prove to be a performance problem,
make them extern inline, I think it is, though that's
compiler-specific. I think I've even seen
__attribute__((__force_inline__)) or some such.
After all, the best way to check that something is suitable to pass as
an argument of a specific type to a function is to pass it as one!
Completely avoids struct versioning headaches, too.
Skip the inlines and this technique is portable to even old C variants,
as long as they support some form of varargs/stdarg.
/~\ The ASCII Mouse
\ / Ribbon Campaign
X Against HTMLmo...@rodents-montreal.org
/ \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: Some changes to autoconfiguration APIs
> On Aug 1, 2021, at 11:10 AM, Martin Husemann wrote: > > On Sun, Aug 01, 2021 at 07:57:20AM -0700, Jason Thorpe wrote: >> The situation hasn't changed. I'm still waiting for concrete proposals. > > The concrete proposal is backout - if there is no better idea how to deal > with it properly. I have reworked it on the thorpej-cfargs2 branch. It addresses the concerns about compile-time type checking by using an anonymous structure constructed in-line with the help of a variadic preprocessor macro (to save wear and tear on your fingers and keyboard that might otherwise occur because of annoying boilerplate syntax to construct the structure in-line). Old example: c->c_dev = config_found(sc->sc_dev, , pciprint, CFARG_SUBMATCH, config_stdsubmatch, CFARG_LOCATORS, locs, CFARG_DEVHANDLE, devhandle, CFARG_EOL); New example: c->c_dev = config_found(sc->sc_dev, , pciprint, CFARGS(.submatch = config_stdsubmatch, .locators = locs, .devhandle = devhandle)); Acceptable? -- thorpej
Re: Some changes to autoconfiguration APIs
And as a possible optional extra, one fairly easy way to add type checking woukd be to add an extra dummp printf format string arg, unused by config_found (would cost one useless ptr push at each call, but we can bear that), declare config_fiund __printf_like, and let the compiker do arg verification (correct types, etc) for us. We'd miss correct pointed at type verification but everyone avoids tgat using (void *) anyway. Provide a few standard format strings for the common use cases, whatever they are, so people just naturally use those (they'd just be things like "%d%d%d%p%u" where the %d%d and %d%p are two key abd data pairs, and the %u is for a null key that terminates things. Seems like it could work to me, but I haven't worken on autoconf cide since the early 80's I think so who knows. kre
Re: Some changes to autoconfiguration APIs
Date:Mon, 2 Aug 2021 01:36:26 +0100 From:David Brownlee Message-ID: | 3) This email takes one of Taylor's suggestions and hangs an explicit | version on the calls, which should give reasonable forward | compatibility That solves the wrong problem, there is (one exception) no binary compat issue, when the kernel is recompiled with a new CF_VERSION all the code is recompiled with that new version ID, they all get it fron the sane .h file, so all depend upon it. The exception is modules, if one wanted to be able to change versions without a kernel vers bump, but I see no reason at all to ever need, or even want, that, it isn't as if anything here makes kernel version bumps less needed. The issue, if you looked at Jason's message is what happens when the semantics of an option are to change, or similar, not trying to keep some kind of ABI stability. Personally I'm something of a fan of trusting the caoabilities of the developers who work in this area, make the interface simple enough to be understood, then just let people use it. kre
Re: Some changes to autoconfiguration APIs
As an alternative to switching config_found() to a C99 init variant... Code could be added to a tool which processes the source (cough cough "lint") to scan config_found() calls and pick up semantically invalid parameter uses David
Re: Some changes to autoconfiguration APIs
On Sun, 1 Aug 2021 at 22:47, Jason Thorpe wrote:
>
> > On Aug 1, 2021, at 1:56 PM, Mouse wrote:
> >
> >>> config_found(CF_VERSION, self, whatever, (const struct cfargs *){
> >>> .search = ...,
> >>> .locators = ...,
> >>> })
> >
> >> What do you propose should be the behavior if the versions don't match? I
> >> h$
> >
> > I thought the mail you replied to said, though admittedly partly by
> > implication:
> >
> >>> config_found() needs to check passed cf_version and convert for old
> >>> versions. We are still left with a long tail of conversion code in
> >>> config_found(), but callers Just Work.
>
> Right, "callers Just Work" is carrying a lot of water here. I want to know
> specifically how people think it should behave. For example: What should
> happen in the case of a semantic conflict that can't be resolved during
> conversion?
>
> (If you can't tell, I'm a bit annoyed about folks having plenty of energy to
> express their distaste with one solution, only to float a hand-wavy
> alternative lacking specifics that also has flaws; sorry, abs@, I'm not
> trying to pick on you here...).
Not at all - my goal was to propose a potential alternative, and
poking at gaps helps evaluation :)
As I see it:
1) netbsd-9 had an API which provided some degree of type safety, but
was the result of accreting a baroque combinations of functions and
parameters to the point where it was difficult to use correctly - and
the tree had any number of examples which were actively wrong, and
would fail at runtime, mostly with misbehaviour, but potentially with
panics
2) current has an API which is much easier to understand and use, had
a nice degree of forward compatibility, though introduces some
potential misuse cases which can only be detected at runtime - as a
deliberate tradeoff to achieve a simple, compat calling API given the
limitations of C
3) This email takes one of Taylor's suggestions and hangs an explicit
version on the calls, which should give reasonable forward
compatibility (not as good as 2, but better than 1), keeps his
improved type safety, to hopefully give a more limited set of cases
which would fail at runtime (mis-specified cfargs contents, and cases
where a valid cfargs_v1 cannot be converted into a current cfargs)
Focussing on 2 & 3, the runtime issues are
a) Tag params missing value params & similar (applicable to 2)
b) Semantically valid options which do not make sense (applicable to 2 & 3)
For both of these the kernel can panic, or fail the attach with a
nasty loud message (which I rather prefer), but we have the same
runtime issue to handle for both 2 & 3
c) Parameters which made sense for an earlier version of the kernel
API, but do not now (applicable to 2 & 3)
The obvious reply is "Don't do that", but if for some reason we have
to, option 3 potentially has an advantage here, as for example the
conversion code called by config_found() can know that the "search"
value in cfargs_v1 needs to be swizzled differently to that of
cfargs_v2
tl;dr - all options allow code to call into config with bad data,
which it has to handle (panic or log & fail attach), we can only try
to reduce, not eliminate that.
(Let me know if I've reduced the hand waving in the right area :)
David
Re: Some changes to autoconfiguration APIs
> Date: Sun, 1 Aug 2021 14:47:39 -0700
> From: Jason Thorpe
>
> >>> config_found() needs to check passed cf_version and convert for old
> >>> versions. We are still left with a long tail of conversion code in
> >>> config_found(), but callers Just Work.
>
> Right, "callers Just Work" is carrying a lot of water here. I want
> to know specifically how people think it should behave. For
> example: What should happen in the case of a semantic conflict that
> can't be resolved during conversion?
We could use __RENAME("config_found_v123") to make it a linker error
if there's a semantic conflict that can't be resolved dynamically by
the implementation of config_found. We could even make config_found a
macro so that it automagically passes a minor version for semantic
changes that can be handled, without adding extra verbiage to all the
callers:
#define config_found(x,y,z) _config_found(CF_MINOR_VERSION,x,y,z)
device_t _config_found(int,foo,bar,baz) __RENAME("_config_found_v123");
But, can we go back to what problem this is trying to solve in the
first place? What extensions to config_found have happened or are
planned to happen so frequently that losing all type safety in the API
is worth not bumping the kernel version number once in a while?
(As an aside: We really need to change the API at a deeper level
anyway so that config_found returns some kind of weak pointer to a
device, since the current model of device_t just fundamentally doesn't
work with concurrent detach...)
Re: Some changes to autoconfiguration APIs
> On Aug 1, 2021, at 1:56 PM, Mouse wrote:
>
>>> config_found(CF_VERSION, self, whatever, (const struct cfargs *){
>>> .search = ...,
>>> .locators = ...,
>>> })
>
>> What do you propose should be the behavior if the versions don't match? I h$
>
> I thought the mail you replied to said, though admittedly partly by
> implication:
>
>>> config_found() needs to check passed cf_version and convert for old
>>> versions. We are still left with a long tail of conversion code in
>>> config_found(), but callers Just Work.
Right, "callers Just Work" is carrying a lot of water here. I want to know
specifically how people think it should behave. For example: What should
happen in the case of a semantic conflict that can't be resolved during
conversion?
(If you can't tell, I'm a bit annoyed about folks having plenty of energy to
express their distaste with one solution, only to float a hand-wavy alternative
lacking specifics that also has flaws; sorry, abs@, I'm not trying to pick on
you here...).
-- thorpej
Re: Some changes to autoconfiguration APIs
>> config_found(CF_VERSION, self, whatever, (const struct cfargs *){
>> .search = ...,
>> .locators = ...,
>> })
> What do you propose should be the behavior if the versions don't match? I h$
I thought the mail you replied to said, though admittedly partly by
implication:
>> config_found() needs to check passed cf_version and convert for old
>> versions. We are still left with a long tail of conversion code in
>> config_found(), but callers Just Work.
/~\ The ASCII Mouse
\ / Ribbon Campaign
X Against HTMLmo...@rodents-montreal.org
/ \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: Some changes to autoconfiguration APIs
On Sun, 1 Aug 2021 at 21:50, Jason Thorpe wrote:
>
> > On Aug 1, 2021, at 12:48 PM, David Brownlee wrote:
> >
> > Possible thought to provide type safety with automatic versioning.
> >
> > Use C99 initializers with a CF_VERSION define. When cfargs changes we
> > bump CF_VERSION.
> >
> > config_found() needs to check passed cf_version and convert for old
> > versions. We are still left with a long tail of conversion code in
> > config_found(), but callers Just Work.
> >
> > config_found(CF_VERSION, self, whatever, (const struct cfargs *){
> > .search = ...,
> > .locators = ...,
> > })
>
> I would probably hide it in a macro (part of what I object to about this
> method, which was floated before, is that it is needlessly verbose).
>
> What do you propose should be the behavior if the versions don't match? I
> have an idea in mind, but I want to hear a concrete proposal first.
Well, we're well into into perl TMTOWTDI territory here, but my first
thought would be:
- We start with CF_VERSION 1 and struct cfargs
- when bumping from 1 to 2, copy the existing cfargs to cfargs_v1 then
update, and add a convert_from_cfargs_v1 function
- config_found() starts by checking if cf_version != CF_VERSION and
calls convert_from_cfargs_v1 as needed
- when bumping from 2 to 3, repeat with _v2, plus update
convert_from_cfargs_v1, and add a new case to the start of
config_found()
David
Re: Some changes to autoconfiguration APIs
> On Aug 1, 2021, at 12:48 PM, David Brownlee wrote:
>
> On Sun, 1 Aug 2021 at 15:57, Jason Thorpe wrote:
>>
>>> On Aug 1, 2021, at 5:15 AM, Martin Husemann wrote:
>>>
>>> On Mon, May 10, 2021 at 10:30:09PM -0700, Jason Thorpe wrote:
> On May 10, 2021, at 7:58 PM, matthew green wrote:
>
> please, can we revert and re-do with a type-safe API.
I don't plan to revert, but I will consider a betterly-typed API
that's not extremely cumbersome to use. I am not a fan of Taylor's
proposals. Concrete proposals welcome.
>>>
>>> Ping?
>>>
>>> A decision on this API needs to happen before the netbsd-10 branch
>>> (this is on the branch blocker list) - we need to either backout or move
>>> forward some way.
>>
>> The situation hasn’t changed. I’m still waiting for concrete proposals.
>>
>
> Possible thought to provide type safety with automatic versioning.
>
> Use C99 initializers with a CF_VERSION define. When cfargs changes we
> bump CF_VERSION.
>
> config_found() needs to check passed cf_version and convert for old
> versions. We are still left with a long tail of conversion code in
> config_found(), but callers Just Work.
>
> config_found(CF_VERSION, self, whatever, (const struct cfargs *){
> .search = ...,
> .locators = ...,
> })
I would probably hide it in a macro (part of what I object to about this
method, which was floated before, is that it is needlessly verbose).
What do you propose should be the behavior if the versions don't match? I have
an idea in mind, but I want to hear a concrete proposal first.
-- thorpej
Re: Some changes to autoconfiguration APIs
On Sun, 1 Aug 2021 at 15:57, Jason Thorpe wrote:
>
> > On Aug 1, 2021, at 5:15 AM, Martin Husemann wrote:
> >
> > On Mon, May 10, 2021 at 10:30:09PM -0700, Jason Thorpe wrote:
> >>
> >>> On May 10, 2021, at 7:58 PM, matthew green wrote:
> >>>
> >>> please, can we revert and re-do with a type-safe API.
> >>
> >> I don't plan to revert, but I will consider a betterly-typed API
> >> that's not extremely cumbersome to use. I am not a fan of Taylor's
> >> proposals. Concrete proposals welcome.
> >
> > Ping?
> >
> > A decision on this API needs to happen before the netbsd-10 branch
> > (this is on the branch blocker list) - we need to either backout or move
> > forward some way.
>
> The situation hasn’t changed. I’m still waiting for concrete proposals.
>
Possible thought to provide type safety with automatic versioning.
Use C99 initializers with a CF_VERSION define. When cfargs changes we
bump CF_VERSION.
config_found() needs to check passed cf_version and convert for old
versions. We are still left with a long tail of conversion code in
config_found(), but callers Just Work.
config_found(CF_VERSION, self, whatever, (const struct cfargs *){
.search = ...,
.locators = ...,
})
David
Re: Some changes to autoconfiguration APIs
On Sun, Aug 01, 2021 at 07:57:20AM -0700, Jason Thorpe wrote: > The situation hasn't changed. I'm still waiting for concrete proposals. The concrete proposal is backout - if there is no better idea how to deal with it properly. Martin
Re: Some changes to autoconfiguration APIs
> On Aug 1, 2021, at 5:15 AM, Martin Husemann wrote: > > On Mon, May 10, 2021 at 10:30:09PM -0700, Jason Thorpe wrote: >> >>> On May 10, 2021, at 7:58 PM, matthew green wrote: >>> >>> please, can we revert and re-do with a type-safe API. >> >> I don't plan to revert, but I will consider a betterly-typed API >> that's not extremely cumbersome to use. I am not a fan of Taylor's >> proposals. Concrete proposals welcome. > > Ping? > > A decision on this API needs to happen before the netbsd-10 branch > (this is on the branch blocker list) - we need to either backout or move > forward some way. The situation hasn’t changed. I’m still waiting for concrete proposals. It seems that we have already moved forward. -- thorpej
Re: Some changes to autoconfiguration APIs
On Mon, May 10, 2021 at 10:30:09PM -0700, Jason Thorpe wrote: > > > On May 10, 2021, at 7:58 PM, matthew green wrote: > > > > please, can we revert and re-do with a type-safe API. > > I don't plan to revert, but I will consider a betterly-typed API > that's not extremely cumbersome to use. I am not a fan of Taylor's > proposals. Concrete proposals welcome. Ping? A decision on this API needs to happen before the netbsd-10 branch (this is on the branch blocker list) - we need to either backout or move forward some way. Martin
Re: Some changes to autoconfiguration APIs
> On May 10, 2021, at 7:58 PM, matthew green wrote: > > please, can we revert and re-do with a type-safe API. I don't plan to revert, but I will consider a betterly-typed API that's not extremely cumbersome to use. I am not a fan of Taylor's proposals. Concrete proposals welcome. -- thorpej
re: Some changes to autoconfiguration APIs
> > It was REALLY obvious that there was a lot of blind copy-pasta > > lurking around as I audited everything. > > I appreciate the work you did to audit this! But we should make it > easier, not harder, for the compiler to audit mistakes if we're going > to make tree-wide changes; this appears to be strictly a regression on > technical grounds, for a change in cosmetics. The same audit outcome > could have applied just as well to the type-safe(r) API we had. having looked at this issue closely, and spent the last few days pondering, i've come to agree quite strongly with Taylor's POV here. please, can we revert and re-do with a type-safe API. thanks. .mrg.
Re: Some changes to autoconfiguration APIs
> Date: Fri, 30 Apr 2021 18:14:36 -0700
> From: Jason Thorpe
>
> > On Apr 25, 2021, at 4:26 AM, Taylor R Campbell
> > wrote:
> >
> > (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.
This seems like a trivial cosmetic advantage for a serious regression
in the type safety of a central kernel API? I'm struggling to see the
value proposition for such a large rototill (other than the auditing
that it invited).
> > Here are two alternative approaches that also don't involve a
> > menagerie of config_{search,found}_xyzzy functions:
> >
> > 1. C99 initializers:
> > 2. pthread_attr-style initialization functions:
>
> 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.
If they're more complicated in simple cases, we could have
(type-safe!) shortcuts for the simple cases like config_found_ia...
Even if we had a stable kernel ABI (which we don't), we have ways to
make the ABI stable: we can use __RENAME, we can have struct cfargs be
a pointer to an opaque object allocated by cfargs_init and freed by
config_found* or a new cfargs_destroy, But we can also just do
what we've always done and bump the kernel version when the module ABI
changes.
I'm also not clear on why we need to make this extensible for every
bus that can't be done with bus-specific attach_args. But my first
concern is that this is
(a) a regression from a type-safe API to a type-unsafe API for a
central part of the NetBSD driver interface, and
(b) a tree-wide change, applying to every bus in the tree, which, by
design, cannot be meaningfully compile-tested.
> It was REALLY obvious that there was a lot of blind copy-pasta
> lurking around as I audited everything.
I appreciate the work you did to audit this! But we should make it
easier, not harder, for the compiler to audit mistakes if we're going
to make tree-wide changes; this appears to be strictly a regression on
technical grounds, for a change in cosmetics. The same audit outcome
could have applied just as well to the type-safe(r) API we had.
C hackery [was Re: Some changes to autoconfiguration APIs]
>> [...] statement expression [...]
> I think statement expressions can be a rather "dangerous"
> complication in C
I am moderately sure statement expressions don't exist in C, at least
as of C99; I believe them to be a gccism. (I have never seen a spec
for any later C, though I have seen it said they've included things I
think have no business in C, such as closures, so maybe.)
Many gccisms can be dangerous, indeed. Many of them - the same ones,
in some cases - can also be extremely useful and clarifying.
> I've only ever found them to be truly useful within a macro when I'm
> trying to avoid, or do something different than, the "usual
> promotions".
I find them, combined with nested functions, useful in that the result
can be close to lambda functions:
n = map_sum_over(list, ({ int foo(struct thingy *x)
{ ...
return(n);
}
}));
However, some of the gcc variants I use produce either broken code or
broken debugging info when faced with that, so these days I am more
likely to make the function a nested function within a containing
block, creating a small block if necessary to keep point of definition
close to point of use.
> Kind of related to this, I have the following comment in my notes
> about C:
> - Positional parameters are evil (or at least error prone),
>especially for variable numbers of parameters.
Personally, I think it's less "for variable numbers of parameters" and
more "for large numbers of parameters".
> Named parameters can be simulated in modern C with full structure
> passing:
...at the price of defining a special-purpose struct for each such
function, or at least each such argument signature, and losing
unused-argument warnings (possibly among others, though none others
come to mind immediately).
/~\ The ASCII Mouse
\ / Ribbon Campaign
X Against HTMLmo...@rodents-montreal.org
/ \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: Some changes to autoconfiguration APIs
At Fri, 30 Apr 2021 23:05:48 -0400 (EDT), Mouse
wrote:
Subject: Re: Some changes to autoconfiguration APIs
>
> However, I see little reason to do the
> statement expression rather than
>
> { static const struct cfargs foo = { ... };
> config_found(..., );
> }
That's a very good point!
I think statement expressions can be a rather "dangerous" complication
in C -- I've only ever found them to be truly useful within a macro when
I'm trying to avoid, or do something different than, the "usual
promotions".
Kind of related to this, I have the following comment in my notes about C:
- Positional parameters are evil (or at least error prone), especially
for variable numbers of parameters.
Named parameters can be simulated in modern C with full structure
passing:
struct fooness {
int blah;
};
struct somefunc_params {
char *p1;
int i1;
struct fooness foo;
};
int
somefunc(struct somefunc_params p)
{
if (p.i1)
printf("%s", p.p1);
return 0;
}
res = somefunc((struct somefunc_params)
{.p1 = "foo",
.i1 = 1,
.foo = (struct fooness) {.blah = 4}});
A working example with more rants and ravings about C, and some other
ideas about hiding the struct references within the function
implementation is here:
https://github.com/robohack/experiments/blob/master/tc99namedparams.c
--
Greg A. Woods
Kelowna, BC +1 250 762-7675 RoboHack
Planix, Inc. Avoncote Farms
pgpLOiWfxFc4v.pgp
Description: OpenPGP Digital Signature
Re: Some changes to autoconfiguration APIs
>> Here are two alternative approaches [...]
>> 1. C99 initializers:
>>
>> config_found(self, whatever, (const struct cfargs *){
>> .search = ...,
>> .locators = ...,
>> })
That's not how C99 compound literals work. That creates an anonymous
const struct cfargs * (ie, a pointer object) but then tries to
initialize it as if it instead were the struct it points to. What you
probably want is more like
config_found(self,whatever,&(const struct cfargs){
...
});
but you have to be aware that the anonymous const struct cfargs "has
automatic storage duration associated with the enclosing block"; it
goes away once the block exits. Provided nobody saves pointers to the
struct cfargs or any of its components, this is fine, but I haven't
read the relevant code, so I don't know whether that's so.
There *is* an example (6.5.2.5 [#11]) that applies & to a compound
literal of struct type
[#11] EXAMPLE 3 Initializers with designations can be
combined with compound literals. Structure objects created
using compound literals can be passed to functions without
depending on member order:
drawline((struct point){.x=1, .y=1},
(struct point){.x=3, .y=4});
Or, if drawline instead expected pointers to struct point:
drawline(&(struct point){.x=1, .y=1},
&(struct point){.x=3, .y=4});
so I'm reasonably sure that has to work in C99.
If you want it to have static storage duration, I know of no way to do
that in C short of declaring a variable. It can be done in gcc with a
statement expression
config_found(..., ({ static const struct cfargs foo = { ... };
}));
but I don't know how widely supported that particular extension is, and
I seem to recall seeing it said in some form that depending on gcc-only
features was frowned upon. However, I see little reason to do the
statement expression rather than
{ static const struct cfargs foo = { ... };
config_found(..., );
}
/~\ The ASCII Mouse
\ / Ribbon Campaign
X Against HTMLmo...@rodents-montreal.org
/ \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: Some changes to autoconfiguration APIs
> On Apr 25, 2021, at 4:26 AM, Taylor R Campbell
> wrote:
>
>> Date: Sat, 17 Apr 2021 17:10:44 -0700
>> From: Jason Thorpe
>>
>> 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();
> cfargs_search(, ...);
> cfargs_locators(, ...);
>
> config_found(self, whatever, );
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
> Date: Sun Mar 21 17:35:39 2021 +
>
> 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
Re: Some changes to autoconfiguration APIs
> Date: Sat, 17 Apr 2021 17:10:44 -0700
> From: Jason Thorpe
>
> 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();
cfargs_search(, ...);
cfargs_locators(, ...);
config_found(self, whatever, );
One other concern:
Author: thorpej
Date: Sun Mar 21 17:35:39 2021 +
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.
