Re: devctl_notify system is inconsistent

On Thu, Dec 1, 2022 at 7:59 AM Baptiste Daroussin <bapt@freebsd.org> wrote:

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

Either you need to drop down a level to where the formated events are
queued,
or you'll need to add something in devaddq() to deal with the events. These
are
a slightly different format than the devctl_notify() events because the
latter was
added later and I didn't want to cope with transitioning the old formatted
messages
to the new at that time (silly me).


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

We have no current mechanism to do that. We could add something that would
register / deregister them with a sysinit + call to something in
kern_devctl.c which
could do the trick (and also deal with the ordering issues), though having
netlink(4)
as a loadable modules might be an interesting case to get right.

If we did that, we could return a 'token' that you'd use to call a new
version of
devctl_notify(), perhaps with some glue for legacy users (or perhaps not:
we change
kernel interfaces all the time, and could just rename it and convert
everything in the
tree).

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

I love the idea. I got over any resistance I had when melifaro@ moved
things into kern_devctl.c and we talked through the issues at that time.
I've been hoping that someone would replace the hacky thing I did with
a 'routing socket'-like interface (I never could figure out hose to do it so
many years ago w/o gross hacks). netlink(4) has finally provided a way
to do it that doesn't get the routing code all jumbled up for this.

I just have some specific issues with your proposal. In hindsight, I should
have led with that in my first message :).

Warner