Changeset View
Standalone View
sys/netlink/netlink_sysevent.c
- This file was added.
| /*- | |||||||||
| * SPDX-License-Identifier: BSD-2-Clause | |||||||||
| * | |||||||||
| * Copyright (c) 2023 Baptiste Daroussin <bapt@FreeBSD.org> | |||||||||
| * | |||||||||
| * Redistribution and use in source and binary forms, with or without | |||||||||
| * modification, are permitted provided that the following conditions | |||||||||
| * are met: | |||||||||
| * 1. Redistributions of source code must retain the above copyright | |||||||||
| * notice, this list of conditions and the following disclaimer. | |||||||||
| * 2. Redistributions in binary form must reproduce the above copyright | |||||||||
| * notice, this list of conditions and the following disclaimer in the | |||||||||
| * documentation and/or other materials provided with the distribution. | |||||||||
| * | |||||||||
| * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND | |||||||||
| * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE | |||||||||
| * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE | |||||||||
| * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE | |||||||||
| * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL | |||||||||
| * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS | |||||||||
| * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) | |||||||||
| * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT | |||||||||
| * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY | |||||||||
| * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF | |||||||||
| * SUCH DAMAGE. | |||||||||
| */ | |||||||||
| #include <sys/param.h> | |||||||||
| #include <sys/types.h> | |||||||||
| #include <sys/devctl.h> | |||||||||
| #include <sys/errno.h> | |||||||||
| #include <sys/module.h> | |||||||||
| #include <sys/kernel.h> | |||||||||
| #include <sys/malloc.h> | |||||||||
| #include <net/vnet.h> | |||||||||
| #include <netlink/netlink.h> | |||||||||
| #include <netlink/netlink_ctl.h> | |||||||||
| #include <netlink/netlink_generic.h> | |||||||||
melifaro: That's an internal header, which shouldn't be used here.
Are there any places where it's… | |||||||||
| #include "netlink_sysevent.h" | |||||||||
Not Done Inline ActionsIs there any specific reason to have it different from the other headers? melifaro: Is there any specific reason to have it different from the other headers? | |||||||||
| #define DEBUG_MOD_NAME nl_sysevent | |||||||||
| #define DEBUG_MAX_LEVEL LOG_DEBUG3 | |||||||||
| #include <netlink/netlink_debug.h> | |||||||||
| _DECLARE_DEBUG(LOG_INFO); | |||||||||
| MALLOC_DEFINE(M_NLSE, "nlsysevent", "Memory used for Netlink sysevent"); | |||||||||
| #define NLSE_FAMILY_NAME "nlsysevent" | |||||||||
| static uint32_t ctrl_family_id; | |||||||||
| #define MAX_SYSEVENTS 64 | |||||||||
| static struct sysevent { | |||||||||
Not Done Inline ActionsPersonally I read it as "maximum sysevents I can send". Maybe it's worth renaming it to something like "SYSTEVENT_TYPES" or similar? melifaro: Personally I read it as "maximum sysevents I can send". Maybe it's worth renaming it to… | |||||||||
| char *name; | |||||||||
| uint32_t id; | |||||||||
| } sysevents[MAX_SYSEVENTS] = {}; | |||||||||
| static void | |||||||||
| sysevent_write(struct sysevent *se, const char *subsystem, const char *type, | |||||||||
| const char *data) | |||||||||
| { | |||||||||
| struct nl_writer nw = {}; | |||||||||
| if (!nlmsg_get_group_writer(&nw, NLMSG_LARGE, NETLINK_GENERIC, se->id)) { | |||||||||
| NL_LOG(LOG_DEBUG, "error allocating group writer"); | |||||||||
| return; | |||||||||
| } | |||||||||
| struct nlmsghdr hdr = { .nlmsg_type = ctrl_family_id }; | |||||||||
| if (!nlmsg_reply(&nw, &hdr, sizeof(struct genlmsghdr))) { | |||||||||
| return; | |||||||||
| } | |||||||||
| struct genlmsghdr *ghdr = nlmsg_reserve_object(&nw, struct genlmsghdr); | |||||||||
| if (ghdr == NULL) { | |||||||||
| NL_LOG(LOG_DEBUG, "unable to allocate memory"); | |||||||||
Done Inline ActionsHard no on this table. If you want to make this an 'enum', I'd do it old-school X11 way with imp: Hard no on this table.
devd hasn't needed it in the past and there's a lot of pain in having… | |||||||||
Done Inline ActionsYes 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: Yes I think we do need a discussion before I push this anywhere, the way we do define "systems"… | |||||||||
Done Inline Actionsthat 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. imp: that said, I'd be OK on the table if there were some plan to have a way to register all the… | |||||||||
| return; | |||||||||
| } | |||||||||
| ghdr->version = 0; | |||||||||
| ghdr->cmd = 0; | |||||||||
| ghdr->reserved = 0; | |||||||||
Not Done Inline ActionsI'd set some CMD here to adhere to the protocol. melifaro: I'd set some CMD here to adhere to the protocol. | |||||||||
| nlattr_add_string(&nw, NLSE_ATTR_SYSTEM, se->name); | |||||||||
| nlattr_add_string(&nw, NLSE_ATTR_SUBSYSTEM, subsystem); | |||||||||
| nlattr_add_string(&nw, NLSE_ATTR_TYPE, type); | |||||||||
| if (data != NULL) | |||||||||
| nlattr_add_string(&nw, NLSE_ATTR_DATA, data); | |||||||||
| nlmsg_end(&nw); | |||||||||
| nlmsg_flush(&nw); | |||||||||
Done Inline ActionsI'd suggest either removing it or converting it to NL_LOG(). melifaro: I'd suggest either removing it or converting it to NL_LOG(). | |||||||||
| } | |||||||||
Done Inline ActionsI'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. melifaro: I'd suggest using `NL_LOG()` for consistency in all reporting. It's easy to setup:
```
#define… | |||||||||
| static void | |||||||||
| sysevent_send(const char *system, const char *subsystem, const char *type, | |||||||||
| const char *data) | |||||||||
Done Inline ActionsMaybe it's worth splitting the function in two, with the first part getting the proper group, setting vnet & calling the second? melifaro: Maybe it's worth splitting the function in two, with the first part getting the proper group… | |||||||||
| { | |||||||||
| struct sysevent *se = NULL; | |||||||||
Done Inline ActionsNit: 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. melifaro: Nit: the message will be prefixed by `[nl_sysevent] sysevent_send_to_group `, so probably worth… | |||||||||
| for (size_t i = 0; i < MAX_SYSEVENTS; i++) { | |||||||||
| if (sysevents[i].name == NULL) { | |||||||||
Not Done Inline ActionsMaybe 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 melifaro: Maybe it's worth having a separate function for finding/creating the event group. Like… | |||||||||
| sysevents[i].name = strdup(system, M_NLSE); | |||||||||
Done Inline Actionsnlmsg_type should be equal to the family id allocated by the module (e.g. ctrl_family_id). melifaro: `nlmsg_type` should be equal to the family id allocated by the module (e.g. `ctrl_family_id`). | |||||||||
| sysevents[i].id = genl_register_group(NLSE_FAMILY_NAME, | |||||||||
Not Done Inline ActionsI'd probably factor out the code for creating new group into separate function to avoid duplication. melifaro: I'd probably factor out the code for creating new group into separate function to avoid… | |||||||||
| system); | |||||||||
| se = &sysevents[i]; | |||||||||
| break; | |||||||||
| } | |||||||||
| if (strcmp(sysevents[i].name, system) == 0) { | |||||||||
| se = &sysevents[i]; | |||||||||
| break; | |||||||||
| } | |||||||||
| } | |||||||||
| if (se == NULL) { | |||||||||
| NL_LOG(LOG_WARNING, "impossible to add the event %s, " | |||||||||
| "too many events\n", system); | |||||||||
Done Inline ActionsI'd still allocate some CMD (NLS_NEWEVENT? ) to make it non-zero. melifaro: I'd still allocate some CMD (NLS_NEWEVENT? ) to make it non-zero. | |||||||||
| return; | |||||||||
| } | |||||||||
| CURVNET_SET(vnet0); | |||||||||
| sysevent_write(se, subsystem, type, data); | |||||||||
| CURVNET_RESTORE(); | |||||||||
| } | |||||||||
| static void | |||||||||
| nlsysevent_load(void) | |||||||||
| { | |||||||||
| devctl_set_notify_hook(sysevent_send); | |||||||||
| ctrl_family_id = genl_register_family(NLSE_FAMILY_NAME, 0, 2, NLSE_ATTR_MAX); | |||||||||
Not Done Inline ActionsNit: no prefix needed, '\n' is redundant melifaro: Nit: no prefix needed, '\n' is redundant | |||||||||
| for (size_t i = 0; i < nitems(devctl_systems); i++) { | |||||||||
| if (i >= MAX_SYSEVENTS) { | |||||||||
| NL_LOG(LOG_WARNING, "impossible to add the event %s, too many events\n", devctl_systems[i]); | |||||||||
Done Inline ActionsI'd probably consider renaming CTRL_ prefix to something like NLEVT_ or similar (or even remove it), melifaro: I'd probably consider renaming `CTRL_` prefix to something like `NLEVT_` or similar (or even… | |||||||||
| continue; | |||||||||
| } | |||||||||
| sysevents[i].name = strdup(devctl_systems[i], M_NLSE); | |||||||||
| sysevents[i].id = genl_register_group(NLSE_FAMILY_NAME, devctl_systems[i]); | |||||||||
| } | |||||||||
| } | |||||||||
| static void | |||||||||
| nlsysevent_unload(void) | |||||||||
| { | |||||||||
Done Inline Actions
melifaro: | |||||||||
| devctl_unset_notify_hook(); | |||||||||
| genl_unregister_family(NLSE_FAMILY_NAME); | |||||||||
| for (size_t i = 0; i < MAX_SYSEVENTS; i++) { | |||||||||
| if (sysevents[i].name == NULL) | |||||||||
| break; | |||||||||
| free(sysevents[i].name, M_NLSE); | |||||||||
| } | |||||||||
| } | |||||||||
| static int | |||||||||
| nlsysevent_loader(module_t mod __unused, int what, void *priv __unused) | |||||||||
| { | |||||||||
| int err = 0; | |||||||||
| switch (what) { | |||||||||
| case MOD_LOAD: | |||||||||
| nlsysevent_load(); | |||||||||
| break; | |||||||||
| case MOD_UNLOAD: | |||||||||
| nlsysevent_unload(); | |||||||||
| break; | |||||||||
| default: | |||||||||
| err = EOPNOTSUPP; | |||||||||
| break; | |||||||||
| } | |||||||||
| return (err); | |||||||||
| } | |||||||||
| static moduledata_t nlsysevent_mod = { "nlsysevent", nlsysevent_loader, NULL}; | |||||||||
| DECLARE_MODULE(nlsysevent, nlsysevent_mod, SI_SUB_PSEUDO, SI_ORDER_ANY); | |||||||||
| MODULE_DEPEND(nlsysevent, netlink, 1, 1, 1); | |||||||||
| MODULE_VERSION(nlsysevent, 1); | |||||||||
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.