Date: Thu, 1 Dec 2022 07:45:32 -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: <CANCZdfpdkxGaan6fvTYxm4YO17-t=V4yuzPWYJnGd7Ao73NQgw@mail.gmail.com> In-Reply-To: <20221201083559.xx3v5jn7sf44rfmv@aniel.nours.eu> References: <20221201083559.xx3v5jn7sf44rfmv@aniel.nours.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
--00000000000092a94a05eec546f4 Content-Type: text/plain; charset="UTF-8" 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 --00000000000092a94a05eec546f4 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 1:36 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">Hello,= <br> <br> After the addition of netlink(4) by melifaro@, I started working on a new<b= r> genetlink(4) module, to send kernel notification to the userland via netlin= k.<br> <br> The goal is to be able to have multiple consumers without the need of devd = to be<br> running.<br> <br> The goal is also to be able subscribe to the events the consumer is 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 got sent vi= a<br> nlsysevent. (<a href=3D"https://reviews.freebsd.org/D37573" rel=3D"noreferr= er" target=3D"_blank">https://reviews.freebsd.org/D37573</a>)<br></blockquo= te><div><br></div><div>You're connecting it at the wrong level: it will= miss all device events. devctl_notify</div><div>is used by everything exce= pt newbus's device attach, detach and nomatch</div><div>events, so none= of those events will make it through.</div><div>=C2=A0</div><blockquote cl= ass=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid= rgb(204,204,204);padding-left:1ex"> 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></blockquote><div><br></div><div>= They are consistent. With one exception that I deprecated in 13.x to be</di= v><div>removed in 14.x. I documented all of them at the time in devd.conf. = I think</div><div>I'll go ahead and complete the deprecation.</div><div= >=C2=A0</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"> I intent to fix the following way:<br> Create a new function similar to devctl_notify but with the first argument = being<br> an enum.<br></blockquote><div><br></div><div>I'm a pretty hard no on th= e enum. I rejected doing it that way when I wrote devd</div><div>years ago.= These have always been strings, and need to continue to always be</div><di= v>strings, or we're forever modifying devd(8) when we add a new one and= introducing</div><div>yet another opportunity for mismatch. I don't th= ink this is a good idea at all.</div><div><br></div><div><div>There's b= een users of devd in the past that are loadable modules that pass their</di= v><div>own system name in that devd.conf files would then process. Having a= n enum with</div></div><div>enforcement would just get in the way of this.<= /div><div><br></div><div>I have toyed with the idea of having a centralized= list in bus.h that's a bunch of</div><div>#defines, ala old-school X11= resources (which this is very very loosely based on),</div><div>but it did= n't seem worth the effort.</div><div>=C2=A0</div><blockquote class=3D"g= mail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204= ,204,204);padding-left:1ex"> Make the current devctl_notify convert its first argument into that enum an= d<br> yell if an unkwown "system" is passed. (and probably declare devc= tl_notify<br> deprecated)<br></blockquote><div><br></div><div>I don't think this buys= us anything. devctl_notify cannot possibly know about all</div><div>the po= ssible subsystems, so this is likely doomed to high maintenance that would<= /div><div>be largely ineffective.</div><div><br></div><blockquote class=3D"= gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(20= 4,204,204);padding-left:1ex">Then fix the inconsistencies: all upper case a= s it seems the most wildly use<br> case<br> s/kern/kernel/g<br> <br> WDYT?<br></blockquote><div><br></div><div>I wouldn't worry about the up= per vs lower case. All the documented ones are</div><div>upper case, except= kernel. There's no harm it being one exception since changing</div><di= v>it will break user's devd.conf setups. I got enough feedback when I c= hanged</div><div>"kern" to "kernel" last year for power= events. And as far as I know, I've documented</div><div>all the events= that are generated today.</div><div>=C2=A0<br></div><div>Warner</div><div>= <br></div></div></div> --00000000000092a94a05eec546f4--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfpdkxGaan6fvTYxm4YO17-t=V4yuzPWYJnGd7Ao73NQgw>