Date: Thu, 1 Dec 2022 08:38:37 -0700 From: Warner Losh <imp@bsdimp.com> To: Baptiste Daroussin <bapt@freebsd.org> Cc: hackers@freebsd.org Subject: Re: devctl_notify system is inconsistent Message-ID: <CANCZdfoxKs9FJMyRh=gVSo9JnX-Lb7-ybXmq==NvzjdKVpckBw@mail.gmail.com> In-Reply-To: <20221201145905.ua2jlh25usqrqjy4@aniel.nours.eu> References: <20221201083559.xx3v5jn7sf44rfmv@aniel.nours.eu> <CANCZdfpdkxGaan6fvTYxm4YO17-t=V4yuzPWYJnGd7Ao73NQgw@mail.gmail.com> <20221201145905.ua2jlh25usqrqjy4@aniel.nours.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
--000000000000721fab05eec604c2 Content-Type: text/plain; charset="UTF-8" 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 --000000000000721fab05eec604c2 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">= <div dir=3D"ltr" class=3D"gmail_attr">On Thu, Dec 1, 2022 at 7:59 AM Baptis= te Daroussin <<a href=3D"mailto:bapt@freebsd.org">bapt@freebsd.org</a>&g= t; wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0p= x 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu= , Dec 01, 2022 at 07:45:32AM -0700, Warner Losh wrote:<br> > On Thu, Dec 1, 2022 at 1:36 AM Baptiste Daroussin <<a href=3D"mailt= o:bapt@freebsd.org" target=3D"_blank">bapt@freebsd.org</a>> wrote:<br> > <br> > > Hello,<br> > ><br> > > After the addition of netlink(4) by melifaro@, I started working = on a new<br> > > genetlink(4) module, to send kernel notification to the userland = via<br> > > netlink.<br> > ><br> > > The goal is to be able to have multiple consumers without the nee= d of devd<br> > > to be<br> > > running.<br> > ><br> > > The goal is also to be able subscribe to the events the consumer = is<br> > > willing to<br> > > receive.<br> > ><br> > > <a href=3D"https://reviews.freebsd.org/D37574" rel=3D"noreferrer"= target=3D"_blank">https://reviews.freebsd.org/D37574</a><br>; > ><br> > > I also added a hook to devctl_notify to make sure all its event g= ot sent<br> > > via<br> > > nlsysevent. (<a href=3D"https://reviews.freebsd.org/D37573" rel= =3D"noreferrer" target=3D"_blank">https://reviews.freebsd.org/D37573</a>)<b= r> > ><br> > <br> > You're connecting it at the wrong level: it will miss all device e= vents.<br> > devctl_notify<br> > is used by everything except newbus's device attach, detach and no= match<br> > events, so none of those events will make it through.<br> <br> Where should I hook to get the device events?<br></blockquote><div><br></di= v><div>Either you need to drop down a level to where the formated events ar= e queued,</div><div>or you'll need to add something in devaddq() to dea= l with the events. These are</div><div>a slightly different format than the= devctl_notify() events because the latter was</div><div>added later and I = didn't want to cope with transitioning the old formatted messages</div>= <div>to the new at that time (silly me).</div><div>=C2=A0</div><blockquote = class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px sol= id rgb(204,204,204);padding-left:1ex"> > <br> > <br> > > It works great and so far I am happy with it. on thing I figured = out it is:<br> > > the "system" argment of devctl_notify is inconsistent:<= br> > > Upper case vs lower case<br> > > "kern" vs "kernel"<br> > ><br> > <br> > They are consistent. With one exception that I deprecated in 13.x to b= e<br> > removed in 14.x. I documented all of them at the time in devd.conf. I = think<br> > I'll go ahead and complete the deprecation.<br> > <br> > <br> > > I intent to fix the following way:<br> > > Create a new function similar to devctl_notify but with the first= argument<br> > > being<br> > > an enum.<br> > ><br> > <br> > I'm a pretty hard no on the enum. I rejected doing it that way whe= n I wrote<br> > devd<br> > years ago. These have always been strings, and need to continue to alw= ays be<br> > strings, or we're forever modifying devd(8) when we add a new one = and<br> > introducing<br> > yet another opportunity for mismatch. I don't think this is a good= idea at<br> > all.<br> > <br> > There's been users of devd in the past that are loadable modules t= hat pass<br> > their<br> > own system name in that devd.conf files would then process. Having an = enum<br> > with<br> > enforcement would just get in the way of this.<br> > <br> > I have toyed with the idea of having a centralized list in bus.h that&= #39;s a<br> > bunch of<br> > #defines, ala old-school X11 resources (which this is very very loosel= y<br> > based on),<br> > but it didn't seem worth the effort.<br> <br> That is fine with me and actually I do need the string name to register as = group<br> name, that I didn't want to trash the strings away.<br> <br> But I need a way to list them all.<br></blockquote><div><br></div><div>We h= ave no current mechanism to do that. We could add something that would</div= ><div>register / deregister them with a sysinit=C2=A0+ call to something in= kern_devctl.c which</div><div>could do the trick (and also deal with the o= rdering issues), though having netlink(4)</div><div>as a loadable modules m= ight be an interesting case to get right.</div><div><br></div><div>If we di= d that, we could return a 'token' that you'd use to call a new = version of</div><div>devctl_notify(), perhaps with some glue for legacy use= rs (or perhaps not: we change</div><div>kernel interfaces all the time, and= could just rename it and convert everything in the</div><div>tree).</div><= div><br></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px= 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> <br> > <br> > > Make the current devctl_notify convert its first argument into th= at enum<br> > > and<br> > > yell if an unkwown "system" is passed. (and probably de= clare devctl_notify<br> > > deprecated)<br> > ><br> > <br> > I don't think this buys us anything. devctl_notify cannot possibly= know<br> > about all<br> > the possible subsystems, so this is likely doomed to high maintenance = that<br> > would<br> > be largely ineffective.<br> > <br> > Then fix the inconsistencies: all upper case as it seems the most wild= ly use<br> > > case<br> > > s/kern/kernel/g<br> > ><br> > > WDYT?<br> > ><br> > <br> > I wouldn't worry about the upper vs lower case. All the documented= ones are<br> > upper case, except kernel. There's no harm it being one exception = since<br> > changing<br> > it will break user's devd.conf setups. I got enough feedback when = I changed<br> > "kern" to "kernel" last year for power events. And= as far as I know, I've<br> > documented<br> > all the events that are generated today.<br> > <br> > Warner<br> <br> <br> Note that if you think nlsysevent is a bad idea, I will just forget about i= t.<br></blockquote><div><br></div><div>I=C2=A0love the idea. I got over any= resistance I had when melifaro@ moved</div><div>things into kern_devctl.c = and we talked through the issues at that time.</div><div>I've been hopi= ng that someone would replace the hacky thing I did with</div><div>a 'r= outing socket'-like interface (I never could figure out hose to do it s= o</div><div>many years ago w/o gross hacks). netlink(4) has finally provide= d a way</div><div>to do it that doesn't get the routing code all jumble= d up for this.</div><div><br></div><div>I just have some specific issues wi= th your proposal. In hindsight, I should</div><div>have led with that in my= first message :).</div><div><br></div><div>Warner</div></div></div> --000000000000721fab05eec604c2--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfoxKs9FJMyRh=gVSo9JnX-Lb7-ybXmq==NvzjdKVpckBw>