From nobody Thu Dec 01 14:45:32 2022 X-Original-To: hackers@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4NNJls175nz4jNgn for ; Thu, 1 Dec 2022 14:45:45 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-ej1-x629.google.com (mail-ej1-x629.google.com [IPv6:2a00:1450:4864:20::629]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4NNJls0hhCz41ZJ for ; Thu, 1 Dec 2022 14:45:45 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-ej1-x629.google.com with SMTP id ud5so4745177ejc.4 for ; Thu, 01 Dec 2022 06:45:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20210112.gappssmtp.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=LlJ2WkHhX6HPDDD4Z/U7Z0wQwTxiFO35qiqqFh7joRU=; b=wpXB12c4R72+BtdYuZiLiVDc9+bzdoswL2qJ8QRL+wWDUl2TDIb3huCKsPmDGha6wL ctP6h66uLai8y9vT8sODJIQ//YGiIJRfwwKT+ebvqnPl7zTMQOZWK3Gh7j5/GTEKJVUw vHm9lVzEpf0obUJjnJQyMPHMYxMkogs5Np/DoGaXZ7pCt0zfx1cjObNI3lwZoQ45e041 6Ecajchbbpicj27LDRX2g83R0YkmMv3DyC9LLaOKB0ctXrBJ1uzltRUi1eNZ6/rTSWyV 8IAhnYwXWcttsSKAnq2kNvVyI+qz8iT6eLNcAwKltaj70/hdzGoeO5Te42IMd0ZSbSrS mr1A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=LlJ2WkHhX6HPDDD4Z/U7Z0wQwTxiFO35qiqqFh7joRU=; b=TyGhLIjXjSSECi0hF7uavMxeTFSEn3DFOsob7vLy1GKzQC17nfAZAgXTdXA8Z3PDGF TEg3T1WyYXlX/vm3Cs0seuSpWhJ8FFMjHt50u0iV0w3+Flxpl6/OLaywxCeLYEhKm5j0 dM3tloWHgC/CK38u0RUrUitY/FHzPCenCJs1K0dJip7djaOK+h0q1B0X40ar5k/Kz/EH vqgkn0NIBp+oYTpbpnB2MjFce/oq2Fawf80h+V4V8KHHNy8Vmp1VIZPzZNuGZuUZdjMG cPVDwuIQhsDveifx+8L+VB2oNakrGyMFer5hB0i+AvBNy9feliuekRNmOQZd5ktY69kx /7Ng== X-Gm-Message-State: ANoB5plZ6H3yJFoJhK+fCFIHCUqwcCDviqqCUcsGqfRQE3yQyFHbVLp+ 5ytt2YX4r3h05iEe2vMH9RwODJ1ro5zKx7vuFE/ajhtW5nqnJA== X-Google-Smtp-Source: AA0mqf5tbHVEb7z+2v6U5FbFHjnVbT2aUiqR9sPIqeSERu7vxZ427vXAAmL5kkmNve9UcAJar79O27xXeoOHWffmT5U= X-Received: by 2002:a17:906:c40b:b0:7ae:1e53:95b2 with SMTP id u11-20020a170906c40b00b007ae1e5395b2mr55300285ejz.333.1669905943529; Thu, 01 Dec 2022 06:45:43 -0800 (PST) List-Id: Technical discussions relating to FreeBSD List-Archive: https://lists.freebsd.org/archives/freebsd-hackers List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-hackers@freebsd.org MIME-Version: 1.0 References: <20221201083559.xx3v5jn7sf44rfmv@aniel.nours.eu> In-Reply-To: <20221201083559.xx3v5jn7sf44rfmv@aniel.nours.eu> From: Warner Losh Date: Thu, 1 Dec 2022 07:45:32 -0700 Message-ID: Subject: Re: devctl_notify system is inconsistent To: Baptiste Daroussin Cc: hackers@freebsd.org Content-Type: multipart/alternative; boundary="00000000000092a94a05eec546f4" X-Rspamd-Queue-Id: 4NNJls0hhCz41ZJ X-Spamd-Bar: ---- X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US] X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-ThisMailContainsUnwantedMimeParts: N --00000000000092a94a05eec546f4 Content-Type: text/plain; charset="UTF-8" On Thu, Dec 1, 2022 at 1:36 AM Baptiste Daroussin 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


=
On Thu, Dec 1, 2022 at 1:36 AM Baptis= te Daroussin <bapt@freebsd.org&g= t; 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 netlin= k.

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 vi= a
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 exce= pt newbus's device attach, detach and nomatch
events, so none= of those events will make it through.
=C2=A0
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.
=C2=A0
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 th= e 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 th= ink this is a good idea at all.

There's b= een 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 a= n enum with
enforcement would just get in the way of this.<= /div>

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 did= n't seem worth the effort.
=C2=A0
Make the current devctl_notify convert its first argument into that enum an= d
yell if an unkwown "system" is passed. (and probably declare devc= tl_notify
deprecated)

I don't think this buys= us anything. devctl_notify cannot possibly know about all
the po= ssible subsystems, so this is likely doomed to high maintenance that would<= /div>
be largely ineffective.

Then fix the inconsistencies: all upper case a= s it seems the most wildly use
case
s/kern/kernel/g

WDYT?

I wouldn't worry about the up= per 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 c= hanged
"kern" to "kernel" last year for power= events. And as far as I know, I've documented
all the events= that are generated today.
=C2=A0
Warner
=
--00000000000092a94a05eec546f4--