Source-Changes-D archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: CVS commit: src/sys



> Module Name:    src
> Committed By:   thorpej
> Date:           Sat Apr 24 23:37:01 UTC 2021
> 
> Log Message:
> Merge thorpej-cfargs branch:
> 
> Simplify and make extensible the config_search() / config_found() /
> config_attach() interfaces: rather than having different variants for
> which arguments you want pass along, just have a single call that
> takes a variadic list of tag-value arguments.

I hate to do this for such a large merge, and I appreciate what you've
done to work on fixing fallout promptly, but... I have to ask that
this be reverted.

I didn't realize that your message to tech-kern meant `I am going to
merge this imminently', and although my response on tech-kern didn't
go out until after the merge (I'm behind on source-changes, didn't
notice you'd already merged a few hours before I replied), I had
registered the same objections privately before the merge:

This creates a new class of stack garbage bugs that the compiler will
do nothing to help detect (mismatched tag/value pairs, missing EOL),
and, worse, it affects many code paths that are essentially guaranteed
never to be exercised by automatic tests because they are all
scattered throughout device drivers for hardware which is mostly not
virtualized.

Even if the config_found_ia_ia_cthulhu_fhtagn API may have too many
variant functions, at least they have types the compiler can check
(aside from the attach args passed as void * -- which is a design
mistake we should fix!).  I'm still a little unclear on what the
benefits of the change are, but losing type safety here is a serious
regression.

We can discuss better ways to do the config_found* API, but that
discussion needs to happen first (and concerns addressed, not brushed
off) before investment in widespread changes.  Let's please take
advantage of C's type system to avoid creating new bug classes,
especially new bug classes that can't be automatically tested -- I
suggested a couple of ways to do that already, and I think we could do
even better with some tweaks to config(8) to enable typed interface
attributes.


Home | Main Index | Thread Index | Old Index