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

[bapt]

Summary

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

[bapt]

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

[melifaro]

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]

Address comments from @melifaro

[bapt]

last CTRL -> NLSE

[melifaro]

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

86

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

94

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.

123

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

136

sys/netlink/netlink_sysevent.h

40

Nit: maybe NLSE_ATTR_MAX ?

[bapt]

In D37574#854398, @melifaro 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

[melifaro]

In D37574#854409, @bapt wrote:

In D37574#854398, @melifaro 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) ?

[bapt]

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

[imp]

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

74

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.

[imp]

sys/netlink/netlink_sysevent.c

74

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.

[bapt]

address comments

[bapt]

fix copyrights

[lwhsu]

sys/netlink/netlink_sysevent.h

3

We use BSD-2-Clause now, no -FreeBSD needed.

[bapt]

sys/netlink/netlink_sysevent.c

74

Yes I think we do need a discussion before I push this anywhere, the way we do define "systems" here is inconsistent (see my email to hackers@) we do need to find something imho more robust. (I had no plan to push this table as is, without a discussion)

[bapt]

Use NL_LOG

[bapt]

address some of the melifaro's comments

[bapt]

more of melifaro's comments

[bapt]

Remove use of internal header

[bapt]

split the main function to simplify code and restoration of vnet

[melifaro]

sys/netlink/netlink_sysevent.c

41

Is there any specific reason to have it different from the other headers?

52

Personally I read it as "maximum sysevents I can send". Maybe it's worth renaming it to something like "SYSTEVENT_TYPES" or similar?

79

I'd set some CMD here to adhere to the protocol.

96

Maybe it's worth having a separate function for finding/creating the event group. Like,

sysevent_get_group() that does exactly that. In this case there'll be no name clash between "sysevent_send" and "sysevent_write", it'll be just sysevent_write + sysevent_get_group

98

I'd probably factor out the code for creating new group into separate function to avoid duplication.

sys/netlink/netlink_sysevent.h

34

Nit: I'd add the comments to the attributes on their types and usage e.g. /* string, reporting system name */

[imp]

I think this approach is an innovative way to cope with things.