Page MenuHomeFreeBSD

nlsysevent: add a genetlink(4) module to report kernel events
Needs RevisionPublic

Authored by bapt on Nov 30 2022, 5:06 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jan 21, 11:51 PM
Unknown Object (File)
Sun, Jan 15, 2:36 PM
Unknown Object (File)
Jan 6 2023, 2:06 PM
Unknown Object (File)
Jan 4 2023, 10:04 PM
Unknown Object (File)
Dec 14 2022, 4:19 AM
Unknown Object (File)
Dec 4 2022, 6:40 AM
Unknown Object (File)
Dec 1 2022, 8:36 AM

Details

Reviewers
melifaro
imp
Summary

Hooked to devctl_notify, this allows consumers to received events
by subscribing to a system over a generic netlink protocol

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 48583
Build 45469: arc lint + arc unit

Event Timeline

bapt requested review of this revision.Nov 30 2022, 5:06 PM

note that it requires: https://reviews.freebsd.org/D37573
Also independant from this, it show inconsistencies in the devctl_notify "systems"
Userland example of usage will follow later

Generally LGTM!
Please see some minor comments inline.

sys/netlink/netlink_sysevent.c
39

That's an internal header, which shouldn't be used here.
Are there any places where it's actually required? I'm happy to help remove any dependencies.

88

I'd suggest using NL_LOG() for consistency in all reporting. It's easy to setup:

#define DEBUG_MOD_NAME  nl_sysevent
#define DEBUG_MAX_LEVEL LOG_DEBUG3
#include <netlink/netlink_debug.h>
_DECLARE_DEBUG(LOG_DEBUG3);

Also, given it's not a hot path, I'd probably skip predictors to improve readability.

91

Maybe it's worth splitting the function in two, with the first part getting the proper group, setting vnet & calling the second?

97

nlmsg_type should be equal to the family id allocated by the module (e.g. ctrl_family_id).
nlmsg_flags is already initialized to zero - that's guaranteed by the {} initializer.

110

I'd still allocate some CMD (NLS_NEWEVENT? ) to make it non-zero.

126

I'd probably consider renaming CTRL_ prefix to something like NLEVT_ or similar (or even remove it),
so it won't clash with genetlink control family.

bapt marked 6 inline comments as done.Dec 1 2022, 8:11 AM

Thank you for addressing the comments!
Added a couple of nitpicks that can be addressed at the commit time.

Q: what do you think of providing these notifications to each VNET? This can be addressed later, especially given the support should be implemented in the core netlink code. For now, I'm more curious about the concept.

sys/netlink/netlink_sysevent.c
85

I'd suggest either removing it or converting it to NL_LOG().

93

Nit: the message will be prefixed by [nl_sysevent] sysevent_send_to_group , so probably worth skipping 'nlsysevent: '.

Also: each memory allocation failure will be reported by the underlying nlmsg_refill_buffer(). Currently, it's not verbose, but this will change.

122

Nit: no prefix needed, '\n' is redundant

135
sys/netlink/netlink_sysevent.h
39

Nit: maybe NLSE_ATTR_MAX ?

This revision is now accepted and ready to land.Dec 1 2022, 11:29 AM

Thank you for addressing the comments!
Added a couple of nitpicks that can be addressed at the commit time.

Q: what do you think of providing these notifications to each VNET? This can be addressed later, especially given the support should be implemented in the core netlink code. For now, I'm more curious about the concept.

I don't know, so far I don't see any message going through devctl_notify which should be vnet aware, but let's see how this get used in the future

In D37574#854409, @bapt wrote:

Thank you for addressing the comments!
Added a couple of nitpicks that can be addressed at the commit time.

Q: what do you think of providing these notifications to each VNET? This can be addressed later, especially given the support should be implemented in the core netlink code. For now, I'm more curious about the concept.

I don't know, so far I don't see any message going through devctl_notify which should be vnet aware, but let's see how this get used in the future

Oh, sorry, let me be a bit more specific.
I'm talking about the case when a user wants to put some notification consumer to jail (for example, as a part of jail-service-by-default) with VNET. Currently, such a service won't be able to receive any notifications as it's not in the default VNET anymore.
So the questions is that if this behaviour is desired or not. If not, how should we proceed (have per-jail tunable to allow events passing etc) ?

I don't think putting some notification per jail (if not in the default vnet) is a desired behaviour.

imp requested changes to this revision.Dec 1 2022, 2:35 PM
In D37574#854445, @bapt wrote:

I don't think putting some notification per jail (if not in the default vnet) is a desired behaviour.

sys/netlink/netlink_sysevent.c
73

Hard no on this table.
devd hasn't needed it in the past and there's a lot of pain in having this list in a centralized place.
It prevents people from loading modules that have their own system class (which people have done in the past).

If you want to make this an 'enum', I'd do it old-school X11 way with
#define DEVCTL_SYSTEM_ACPI "ACPI"
while you won't get enforcement, it will be extensible.

This revision now requires changes to proceed.Dec 1 2022, 2:35 PM
sys/netlink/netlink_sysevent.c
73

that said, I'd be OK on the table if there were some plan to have a way to register all the names and call devctl_notify with a token from such a registration rather than just a bare string.