Re: devctl_notify system is inconsistent

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.


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


> 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