Changeset View
Standalone View
sys/netlink/netlink_sysevent.c
- This file was added.
| /*- | |||||||||
| * SPDX-License-Identifier: BSD-2-Clause-FreeBSD | |||||||||
| * | |||||||||
| * Copyright (c) 2022 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… | |||||||||
| #define DEBUG_MOD_NAME nl_sysevent | |||||||||
| #define DEBUG_MAX_LEVEL LOG_DEBUG3 | |||||||||
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? | |||||||||
| #include <netlink/netlink_debug.h> | |||||||||
| _DECLARE_DEBUG(LOG_DEBUG3); | |||||||||
| #include "netlink_sysevent.h" | |||||||||
| #define CTRL_FAMILY_NAME "nlsysevent" | |||||||||
| static uint32_t family_id; | |||||||||
| static struct sysevent { | |||||||||
| uint32_t id; | |||||||||
| const char *name; | |||||||||
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… | |||||||||
| } sysevents [] = { | |||||||||
| { 0, "ACPI"}, | |||||||||
| { 0, "AEON"}, | |||||||||
| { 0, "CAM"}, | |||||||||
| { 0, "CARP"}, | |||||||||
| { 0, "coretemp"}, | |||||||||
| { 0, "DEVFS"}, | |||||||||
| { 0, "ETHERNET"}, | |||||||||
| { 0, "GEOM"}, | |||||||||
| { 0, "HYPERV_NIC_VF"}, | |||||||||
| { 0, "IFNET"}, | |||||||||
| { 0, "INFINIBAND"}, | |||||||||
| { 0, "kern"}, | |||||||||
| { 0, "kernel"}, | |||||||||
| { 0, "nvme"}, | |||||||||
| { 0, "PMU"}, | |||||||||
| { 0, "RCTL"}, | |||||||||
| { 0, "USB"}, | |||||||||
| { 0, "VFS"}, | |||||||||
| { 0, "VT"}, | |||||||||
| { 0, "ZFS"}, | |||||||||
| }; | |||||||||
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… | |||||||||
| static void | |||||||||
| sysevent_send_to_group(uint32_t gid, const char *system, const char *subsystem, | |||||||||
| const char *type, const char *data) | |||||||||
| { | |||||||||
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. | |||||||||
| struct nl_writer nw = {}; | |||||||||
| struct nlmsghdr hdr = { .nlmsg_type = family_id }; | |||||||||
| struct genlmsghdr *ghdr; | |||||||||
| if (!nlmsg_get_group_writer(&nw, NLMSG_LARGE, NETLINK_GENERIC, gid)) { | |||||||||
| printf("nlsysevent: error allocating group writer"); | |||||||||
| return; | |||||||||
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(). | |||||||||
| } | |||||||||
| if (!nlmsg_reply(&nw, &hdr, sizeof(struct genlmsghdr))) | |||||||||
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… | |||||||||
| return; | |||||||||
| ghdr = nlmsg_reserve_object(&nw, struct genlmsghdr); | |||||||||
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… | |||||||||
| if (ghdr == NULL) { | |||||||||
| NL_LOG(LOG_ERR, "nlsysevent: unable to allocate memory"); | |||||||||
| return; | |||||||||
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… | |||||||||
| } | |||||||||
| ghdr->version = 0; | |||||||||
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… | |||||||||
| ghdr->cmd = NLSE_CMD_NEWEVENT; | |||||||||
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`). | |||||||||
| ghdr->reserved = 0; | |||||||||
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… | |||||||||
| nlattr_add_string(&nw, NLSE_SYSTEM, system); | |||||||||
| nlattr_add_string(&nw, NLSE_SUBSYSTEM, subsystem); | |||||||||
| nlattr_add_string(&nw, NLSE_TYPE, type); | |||||||||
| if (data != NULL) | |||||||||
| nlattr_add_string(&nw, NLSE_DATA, data); | |||||||||
| nlmsg_end(&nw); | |||||||||
| nlmsg_flush(&nw); | |||||||||
| } | |||||||||
| static void | |||||||||
| sysevent_send(const char *system, const char *subsystem, const char *type, | |||||||||
| const char *data) | |||||||||
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. | |||||||||
| { | |||||||||
| uint32_t group_id = 0; | |||||||||
| for (size_t i = 0; nitems(sysevents); i++) { | |||||||||
| if (strcmp(sysevents[i].name, system) == 0) { | |||||||||
| group_id = sysevents[i].id; | |||||||||
| break; | |||||||||
| } | |||||||||
| } | |||||||||
| if (group_id == 0) { | |||||||||
| NL_LOG(LOG_ERR, "nlsysevent: unknown system for '%s'\n", | |||||||||
| system); | |||||||||
Not Done Inline ActionsNit: no prefix needed, '\n' is redundant melifaro: Nit: no prefix needed, '\n' is redundant | |||||||||
| return; | |||||||||
| } | |||||||||
| CURVNET_SET(vnet0); | |||||||||
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… | |||||||||
| sysevent_send_to_group(group_id, system, subsystem, type, data); | |||||||||
| CURVNET_RESTORE(); | |||||||||
| } | |||||||||
| static void | |||||||||
| nlsysevent_load(void) | |||||||||
| { | |||||||||
| devctl_set_notify_hook(sysevent_send); | |||||||||
| family_id = genl_register_family(CTRL_FAMILY_NAME, 0, 2, CTRL_ATTR_MAX); | |||||||||
| for (size_t i = 0; i < nitems(sysevents); i++) | |||||||||
Done Inline Actions
melifaro: | |||||||||
| sysevents[i].id = genl_register_group(CTRL_FAMILY_NAME, sysevents[i].name); | |||||||||
| } | |||||||||
| static void | |||||||||
| nlsysevent_unload(void) | |||||||||
| { | |||||||||
| devctl_unset_notify_hook(); | |||||||||
| genl_unregister_family(CTRL_FAMILY_NAME); | |||||||||
| } | |||||||||
| 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.