Re: devctl_notify system is inconsistent

On Thu, Dec 01, 2022 at 07:45:32AM -0700, Warner Losh wrote:
> On Thu, Dec 1, 2022 at 1:36 AM Baptiste Daroussin <bapt@freebsd.org> wrote:
> 
> > Hello,
> >
> > After the addition of netlink(4) by melifaro@, I started working on a new
> > genetlink(4) module, to send kernel notification to the userland via
> > netlink.
> >
> > The goal is to be able to have multiple consumers without the need of devd
> > to be
> > running.
> >
> > The goal is also to be able subscribe to the events the consumer is
> > willing to
> > receive.
> >
> > https://reviews.freebsd.org/D37574
> >
> > I also added a hook to devctl_notify to make sure all its event got sent
> > via
> > nlsysevent. (https://reviews.freebsd.org/D37573)
> >
> 
> You're connecting it at the wrong level: it will miss all device events.
> devctl_notify
> is used by everything except newbus's device attach, detach and nomatch
> events, so none of those events will make it through.

Where should I hook to get the device events?
> 
> 
> > It works great and so far I am happy with it. on thing I figured out it is:
> > the "system" argment of devctl_notify is inconsistent:
> > Upper case vs lower case
> > "kern" vs "kernel"
> >
> 
> They are consistent. With one exception that I deprecated in 13.x to be
> removed in 14.x. I documented all of them at the time in devd.conf. I think
> I'll go ahead and complete the deprecation.
> 
> 
> > I intent to fix the following way:
> > Create a new function similar to devctl_notify but with the first argument
> > being
> > an enum.
> >
> 
> I'm a pretty hard no on the enum. I rejected doing it that way when I wrote
> devd
> years ago. These have always been strings, and need to continue to always be
> strings, or we're forever modifying devd(8) when we add a new one and
> introducing
> yet another opportunity for mismatch. I don't think this is a good idea at
> all.
> 
> There's been users of devd in the past that are loadable modules that pass
> their
> own system name in that devd.conf files would then process. Having an enum
> with
> enforcement would just get in the way of this.
> 
> I have toyed with the idea of having a centralized list in bus.h that's a
> bunch of
> #defines, ala old-school X11 resources (which this is very very loosely
> based on),
> but it didn't seem worth the effort.

That is fine with me and actually I do need the string name to register as group
name, that I didn't want to trash the strings away.

But I need a way to list them all.
> 
> 
> > Make the current devctl_notify convert its first argument into that enum
> > and
> > yell if an unkwown "system" is passed. (and probably declare devctl_notify
> > deprecated)
> >
> 
> I don't think this buys us anything. devctl_notify cannot possibly know
> about all
> the possible subsystems, so this is likely doomed to high maintenance that
> would
> be largely ineffective.
> 
> Then fix the inconsistencies: all upper case as it seems the most wildly use
> > case
> > s/kern/kernel/g
> >
> > WDYT?
> >
> 
> I wouldn't worry about the upper vs lower case. All the documented ones are
> upper case, except kernel. There's no harm it being one exception since
> changing
> it will break user's devd.conf setups. I got enough feedback when I changed
> "kern" to "kernel" last year for power events. And as far as I know, I've
> documented
> all the events that are generated today.
> 
> Warner


Note that if you think nlsysevent is a bad idea, I will just forget about it.

Best regards,
Bapt