From db4735b7dba6a9011cbece6bf180d0ff07d0ce6a Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 27 Jun 2021 02:28:14 +0900 Subject: [PATCH 1/6] sd-netlink: shorten code a bit This also fixes comments, a log message, and coding style nits. --- src/libsystemd/sd-netlink/netlink-socket.c | 57 +++++++--------------- 1 file changed, 18 insertions(+), 39 deletions(-) diff --git a/src/libsystemd/sd-netlink/netlink-socket.c b/src/libsystemd/sd-netlink/netlink-socket.c index 273af6383e..a29210e1ce 100644 --- a/src/libsystemd/sd-netlink/netlink-socket.c +++ b/src/libsystemd/sd-netlink/netlink-socket.c @@ -35,8 +35,7 @@ static int broadcast_groups_get(sd_netlink *nl) { assert(nl); assert(nl->fd >= 0); - r = getsockopt(nl->fd, SOL_NETLINK, NETLINK_LIST_MEMBERSHIPS, NULL, &len); - if (r < 0) { + if (getsockopt(nl->fd, SOL_NETLINK, NETLINK_LIST_MEMBERSHIPS, NULL, &len) < 0) { if (errno != ENOPROTOOPT) return -errno; @@ -53,23 +52,18 @@ static int broadcast_groups_get(sd_netlink *nl) { old_len = len; - r = getsockopt(nl->fd, SOL_NETLINK, NETLINK_LIST_MEMBERSHIPS, groups, &len); - if (r < 0) + if (getsockopt(nl->fd, SOL_NETLINK, NETLINK_LIST_MEMBERSHIPS, groups, &len) < 0) return -errno; if (old_len != len) return -EIO; - r = hashmap_ensure_allocated(&nl->broadcast_group_refs, NULL); - if (r < 0) - return r; - for (unsigned i = 0; i < len; i++) for (unsigned j = 0; j < sizeof(uint32_t) * 8; j++) if (groups[i] & (1U << j)) { unsigned group = i * sizeof(uint32_t) * 8 + j + 1; - r = hashmap_put(nl->broadcast_group_refs, UINT_TO_PTR(group), UINT_TO_PTR(1)); + r = hashmap_ensure_put(&nl->broadcast_group_refs, NULL, UINT_TO_PTR(group), UINT_TO_PTR(1)); if (r < 0) return r; } @@ -88,7 +82,7 @@ int socket_bind(sd_netlink *nl) { addrlen = sizeof(nl->sockaddr); r = bind(nl->fd, &nl->sockaddr.sa, addrlen); - /* ignore EINVAL to allow opening an already bound socket */ + /* ignore EINVAL to allow binding an already bound socket */ if (r < 0 && errno != EINVAL) return -errno; @@ -110,11 +104,11 @@ static int broadcast_group_set_ref(sd_netlink *nl, unsigned group, unsigned n_re assert(nl); - r = hashmap_replace(nl->broadcast_group_refs, UINT_TO_PTR(group), UINT_TO_PTR(n_ref)); + r = hashmap_ensure_allocated(&nl->broadcast_group_refs, NULL); if (r < 0) return r; - return 0; + return hashmap_replace(nl->broadcast_group_refs, UINT_TO_PTR(group), UINT_TO_PTR(n_ref)); } static int broadcast_group_join(sd_netlink *nl, unsigned group) { @@ -136,23 +130,15 @@ int socket_broadcast_group_ref(sd_netlink *nl, unsigned group) { n_ref++; - r = hashmap_ensure_allocated(&nl->broadcast_group_refs, NULL); - if (r < 0) - return r; - r = broadcast_group_set_ref(nl, group, n_ref); if (r < 0) return r; if (n_ref > 1) - /* not yet in the group */ + /* already in the group */ return 0; - r = broadcast_group_join(nl, group); - if (r < 0) - return r; - - return 0; + return broadcast_group_join(nl, group); } static int broadcast_group_leave(sd_netlink *nl, unsigned group) { @@ -187,19 +173,12 @@ int socket_broadcast_group_unref(sd_netlink *nl, unsigned group) { /* still refs left */ return 0; - r = broadcast_group_leave(nl, group); - if (r < 0) - return r; - - return 0; + return broadcast_group_leave(nl, group); } /* returns the number of bytes sent, or a negative error code */ int socket_write_message(sd_netlink *nl, sd_netlink_message *m) { - union { - struct sockaddr sa; - struct sockaddr_nl nl; - } addr = { + union sockaddr_union addr = { .nl.nl_family = AF_NETLINK, }; ssize_t k; @@ -208,8 +187,7 @@ int socket_write_message(sd_netlink *nl, sd_netlink_message *m) { assert(m); assert(m->hdr); - k = sendto(nl->fd, m->hdr, m->hdr->nlmsg_len, - 0, &addr.sa, sizeof(addr)); + k = sendto(nl->fd, m->hdr, m->hdr->nlmsg_len, 0, &addr.sa, sizeof(addr)); if (k < 0) return -errno; @@ -218,22 +196,24 @@ int socket_write_message(sd_netlink *nl, sd_netlink_message *m) { int socket_writev_message(sd_netlink *nl, sd_netlink_message **m, size_t msgcount) { _cleanup_free_ struct iovec *iovs = NULL; + ssize_t k; assert(nl); assert(m); assert(msgcount > 0); - iovs = new0(struct iovec, msgcount); + iovs = new(struct iovec, msgcount); if (!iovs) return -ENOMEM; for (size_t i = 0; i < msgcount; i++) { - assert(m[i]->hdr != NULL); + assert(m[i]->hdr); assert(m[i]->hdr->nlmsg_len > 0); + iovs[i] = IOVEC_MAKE(m[i]->hdr, m[i]->hdr->nlmsg_len); } - ssize_t k = writev(nl->fd, iovs, msgcount); + k = writev(nl->fd, iovs, msgcount); if (k < 0) return -errno; @@ -266,7 +246,7 @@ static int socket_recv_message(int fd, struct iovec *iov, uint32_t *ret_mcast_gr if (sender.nl.nl_pid != 0) { /* not from the kernel, ignore */ - log_debug("rtnl: ignoring message from portid %"PRIu32, sender.nl.nl_pid); + log_debug("rtnl: ignoring message from PID %"PRIu32, sender.nl.nl_pid); if (peek) { /* drop the message */ @@ -336,13 +316,12 @@ int socket_read_message(sd_netlink *rtnl) { if (NLMSG_OK(rtnl->rbuffer, len) && rtnl->rbuffer->nlmsg_flags & NLM_F_MULTI) { multi_part = true; - for (i = 0; i < rtnl->rqueue_partial_size; i++) { + for (i = 0; i < rtnl->rqueue_partial_size; i++) if (rtnl_message_get_serial(rtnl->rqueue_partial[i]) == rtnl->rbuffer->nlmsg_seq) { first = rtnl->rqueue_partial[i]; break; } - } } for (struct nlmsghdr *new_msg = rtnl->rbuffer; NLMSG_OK(new_msg, len) && !done; new_msg = NLMSG_NEXT(new_msg, len)) { From eee15fff4005c3a7c7dffba8776a96c050464b1a Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 27 Jun 2021 02:30:52 +0900 Subject: [PATCH 2/6] sd-netlink: do not trigger assertion by calling socket_broadcast_group_unref() with an arbitrary group number socket_broadcast_group_unref() is only called in netlink_slot_disconnect(), so the assertion should not be triggered as the match slot was successfully created. But, we usually design `_ref/unref()` functions as they can be called for any inputs. So, let's also follow the design rule here. --- src/libsystemd/sd-netlink/netlink-socket.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libsystemd/sd-netlink/netlink-socket.c b/src/libsystemd/sd-netlink/netlink-socket.c index a29210e1ce..21ec00cca0 100644 --- a/src/libsystemd/sd-netlink/netlink-socket.c +++ b/src/libsystemd/sd-netlink/netlink-socket.c @@ -160,8 +160,8 @@ int socket_broadcast_group_unref(sd_netlink *nl, unsigned group) { assert(nl); n_ref = broadcast_group_get_ref(nl, group); - - assert(n_ref > 0); + if (n_ref == 0) + return 0; n_ref--; From 0f97cfb9bee009a95ac4af609d466d86d094779d Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 27 Jun 2021 03:00:42 +0900 Subject: [PATCH 3/6] sd-netlink: rename variables and functions for generic netlink `id` or `type` is ambiguous. Let's use `nlmsg_type`. Also, use `ret` for function arguments to store results. This also adds several assertions. --- src/libsystemd/sd-netlink/generic-netlink.c | 62 ++++++++++++--------- src/systemd/sd-netlink.h | 4 +- 2 files changed, 38 insertions(+), 28 deletions(-) diff --git a/src/libsystemd/sd-netlink/generic-netlink.c b/src/libsystemd/sd-netlink/generic-netlink.c index 3d1ee585c7..744c706f6a 100644 --- a/src/libsystemd/sd-netlink/generic-netlink.c +++ b/src/libsystemd/sd-netlink/generic-netlink.c @@ -34,7 +34,9 @@ static int genl_message_new(sd_netlink *nl, sd_genl_family_t family, uint16_t nl size_t size; int r; - assert_return(nl->protocol == NETLINK_GENERIC, -EINVAL); + assert(nl); + assert(nl->protocol == NETLINK_GENERIC); + assert(ret); r = type_system_get_type(&genl_family_type_system_root, &genl_cmd_type, family); if (r < 0) @@ -72,20 +74,24 @@ static int genl_message_new(sd_netlink *nl, sd_genl_family_t family, uint16_t nl return 0; } -static int lookup_id(sd_netlink *nl, sd_genl_family_t family, uint16_t *id) { +static int lookup_nlmsg_type(sd_netlink *nl, sd_genl_family_t family, uint16_t *ret) { _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *req = NULL, *reply = NULL; uint16_t u; void *v; int r; + assert(nl); + assert(nl->protocol == NETLINK_GENERIC); + assert(ret); + if (family == SD_GENL_ID_CTRL) { - *id = GENL_ID_CTRL; + *ret = GENL_ID_CTRL; return 0; } v = hashmap_get(nl->genl_family_to_nlmsg_type, INT_TO_PTR(family)); if (v) { - *id = PTR_TO_UINT(v); + *ret = PTR_TO_UINT(v); return 0; } @@ -113,36 +119,40 @@ static int lookup_id(sd_netlink *nl, sd_genl_family_t family, uint16_t *id) { if (r < 0) return r; - *id = u; + *ret = u; return 0; } int sd_genl_message_new(sd_netlink *nl, sd_genl_family_t family, uint8_t cmd, sd_netlink_message **ret) { - uint16_t id = 0; /* Unnecessary initialization to appease gcc */ + uint16_t nlmsg_type = 0; /* Unnecessary initialization to appease gcc */ int r; - r = lookup_id(nl, family, &id); - if (r < 0) - return r; - - return genl_message_new(nl, family, id, cmd, ret); -} - -int nlmsg_type_to_genl_family(const sd_netlink *nl, uint16_t type, sd_genl_family_t *ret) { - void *p; - assert_return(nl, -EINVAL); assert_return(nl->protocol == NETLINK_GENERIC, -EINVAL); + assert_return(ret, -EINVAL); + + r = lookup_nlmsg_type(nl, family, &nlmsg_type); + if (r < 0) + return r; + + return genl_message_new(nl, family, nlmsg_type, cmd, ret); +} + +int nlmsg_type_to_genl_family(const sd_netlink *nl, uint16_t nlmsg_type, sd_genl_family_t *ret) { + void *p; + + assert(nl); + assert(nl->protocol == NETLINK_GENERIC); assert(ret); - if (type == NLMSG_ERROR) + if (nlmsg_type == NLMSG_ERROR) *ret = SD_GENL_ERROR; - else if (type == NLMSG_DONE) + else if (nlmsg_type == NLMSG_DONE) *ret = SD_GENL_DONE; - else if (type == GENL_ID_CTRL) + else if (nlmsg_type == GENL_ID_CTRL) *ret = SD_GENL_ID_CTRL; else { - p = hashmap_get(nl->nlmsg_type_to_genl_family, UINT_TO_PTR(type)); + p = hashmap_get(nl->nlmsg_type_to_genl_family, UINT_TO_PTR(nlmsg_type)); if (!p) return -EOPNOTSUPP; @@ -152,18 +162,18 @@ int nlmsg_type_to_genl_family(const sd_netlink *nl, uint16_t type, sd_genl_famil return 0; } -int sd_genl_message_get_family(sd_netlink *nl, sd_netlink_message *m, sd_genl_family_t *family) { - uint16_t type; +int sd_genl_message_get_family(sd_netlink *nl, sd_netlink_message *m, sd_genl_family_t *ret) { + uint16_t nlmsg_type; int r; - assert_return(m, -EINVAL); assert_return(nl, -EINVAL); assert_return(nl->protocol == NETLINK_GENERIC, -EINVAL); - assert_return(family, -EINVAL); + assert_return(m, -EINVAL); + assert_return(ret, -EINVAL); - r = sd_netlink_message_get_type(m, &type); + r = sd_netlink_message_get_type(m, &nlmsg_type); if (r < 0) return r; - return nlmsg_type_to_genl_family(nl, type, family); + return nlmsg_type_to_genl_family(nl, nlmsg_type, ret); } diff --git a/src/systemd/sd-netlink.h b/src/systemd/sd-netlink.h index fb406169de..b7e1a90958 100644 --- a/src/systemd/sd-netlink.h +++ b/src/systemd/sd-netlink.h @@ -251,8 +251,8 @@ int sd_nfnl_nft_message_add_setelem_end(sd_netlink_message *m); /* genl */ int sd_genl_socket_open(sd_netlink **nl); -int sd_genl_message_new(sd_netlink *nl, sd_genl_family_t family, uint8_t cmd, sd_netlink_message **m); -int sd_genl_message_get_family(sd_netlink *nl, sd_netlink_message *m, sd_genl_family_t *family); +int sd_genl_message_new(sd_netlink *nl, sd_genl_family_t family, uint8_t cmd, sd_netlink_message **ret); +int sd_genl_message_get_family(sd_netlink *nl, sd_netlink_message *m, sd_genl_family_t *ret); /* slot */ sd_netlink_slot *sd_netlink_slot_ref(sd_netlink_slot *nl); From 7bb9f0d0140dd11e3d7fcaf8dd45aa4735142406 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 27 Jun 2021 03:58:30 +0900 Subject: [PATCH 4/6] sd-netlink: do not call lookup_nlmsg_type() for known generic netlink family --- src/libsystemd/sd-netlink/generic-netlink.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libsystemd/sd-netlink/generic-netlink.c b/src/libsystemd/sd-netlink/generic-netlink.c index 744c706f6a..69f1a0cef5 100644 --- a/src/libsystemd/sd-netlink/generic-netlink.c +++ b/src/libsystemd/sd-netlink/generic-netlink.c @@ -95,7 +95,7 @@ static int lookup_nlmsg_type(sd_netlink *nl, sd_genl_family_t family, uint16_t * return 0; } - r = sd_genl_message_new(nl, SD_GENL_ID_CTRL, CTRL_CMD_GETFAMILY, &req); + r = genl_message_new(nl, SD_GENL_ID_CTRL, GENL_ID_CTRL, CTRL_CMD_GETFAMILY, &req); if (r < 0) return r; From 2b01228800d33a6e73bba9be13a39c3ff2e56ef8 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 27 Jun 2021 03:28:16 +0900 Subject: [PATCH 5/6] sd-netlink: shorten code a bit This also reduces scope of variables, fixes and reduces indentations, drop unnecessary initializations. --- src/libsystemd/sd-netlink/sd-netlink.c | 87 +++++++++++++------------- 1 file changed, 44 insertions(+), 43 deletions(-) diff --git a/src/libsystemd/sd-netlink/sd-netlink.c b/src/libsystemd/sd-netlink/sd-netlink.c index 97f45c83af..819ce71359 100644 --- a/src/libsystemd/sd-netlink/sd-netlink.c +++ b/src/libsystemd/sd-netlink/sd-netlink.c @@ -254,12 +254,13 @@ int sd_netlink_send(sd_netlink *nl, return 1; } -int sd_netlink_sendv(sd_netlink *nl, - sd_netlink_message **messages, - size_t msgcount, - uint32_t **ret_serial) { +int sd_netlink_sendv( + sd_netlink *nl, + sd_netlink_message **messages, + size_t msgcount, + uint32_t **ret_serial) { + _cleanup_free_ uint32_t *serials = NULL; - unsigned i; int r; assert_return(nl, -EINVAL); @@ -268,13 +269,14 @@ int sd_netlink_sendv(sd_netlink *nl, assert_return(msgcount > 0, -EINVAL); if (ret_serial) { - serials = new0(uint32_t, msgcount); + serials = new(uint32_t, msgcount); if (!serials) return -ENOMEM; } - for (i = 0; i < msgcount; i++) { + for (unsigned i = 0; i < msgcount; i++) { assert_return(!messages[i]->sealed, -EPERM); + rtnl_seal_message(nl, messages[i]); if (serials) serials[i] = rtnl_message_get_serial(messages[i]); @@ -593,6 +595,7 @@ int sd_netlink_call_async( void *userdata, uint64_t usec, const char *description) { + _cleanup_free_ sd_netlink_slot *slot = NULL; int r, k; @@ -648,10 +651,12 @@ int sd_netlink_call_async( return k; } -int sd_netlink_read(sd_netlink *rtnl, - uint32_t serial, - uint64_t usec, - sd_netlink_message **ret) { +int sd_netlink_read( + sd_netlink *rtnl, + uint32_t serial, + uint64_t usec, + sd_netlink_message **ret) { + usec_t timeout; int r; @@ -662,42 +667,39 @@ int sd_netlink_read(sd_netlink *rtnl, for (;;) { usec_t left; - unsigned i; - for (i = 0; i < rtnl->rqueue_size; i++) { + for (unsigned i = 0; i < rtnl->rqueue_size; i++) { + _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *incoming = NULL; uint32_t received_serial; + uint16_t type; received_serial = rtnl_message_get_serial(rtnl->rqueue[i]); + if (received_serial != serial) + continue; - if (received_serial == serial) { - _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *incoming = NULL; - uint16_t type; + incoming = rtnl->rqueue[i]; - incoming = rtnl->rqueue[i]; + /* found a match, remove from rqueue and return it */ + memmove(rtnl->rqueue + i,rtnl->rqueue + i + 1, + sizeof(sd_netlink_message*) * (rtnl->rqueue_size - i - 1)); + rtnl->rqueue_size--; - /* found a match, remove from rqueue and return it */ - memmove(rtnl->rqueue + i,rtnl->rqueue + i + 1, - sizeof(sd_netlink_message*) * (rtnl->rqueue_size - i - 1)); - rtnl->rqueue_size--; + r = sd_netlink_message_get_errno(incoming); + if (r < 0) + return r; - r = sd_netlink_message_get_errno(incoming); - if (r < 0) - return r; + r = sd_netlink_message_get_type(incoming, &type); + if (r < 0) + return r; - r = sd_netlink_message_get_type(incoming, &type); - if (r < 0) - return r; - - if (type == NLMSG_DONE) { - *ret = NULL; - return 0; - } - - if (ret) - *ret = TAKE_PTR(incoming); - - return 1; + if (type == NLMSG_DONE) { + *ret = NULL; + return 0; } + + if (ret) + *ret = TAKE_PTR(incoming); + return 1; } r = socket_read_message(rtnl); @@ -721,15 +723,17 @@ int sd_netlink_read(sd_netlink *rtnl, r = rtnl_poll(rtnl, true, left); if (r < 0) return r; - else if (r == 0) + if (r == 0) return -ETIMEDOUT; } } -int sd_netlink_call(sd_netlink *rtnl, +int sd_netlink_call( + sd_netlink *rtnl, sd_netlink_message *message, uint64_t usec, sd_netlink_message **ret) { + uint32_t serial; int r; @@ -748,10 +752,7 @@ int sd_netlink_get_events(sd_netlink *rtnl) { assert_return(rtnl, -EINVAL); assert_return(!rtnl_pid_changed(rtnl), -ECHILD); - if (rtnl->rqueue_size == 0) - return POLLIN; - else - return 0; + return rtnl->rqueue_size == 0 ? POLLIN : 0; } int sd_netlink_get_timeout(sd_netlink *rtnl, uint64_t *timeout_usec) { From 11537375e38ecd63840c4a0b936b5d3279af2e62 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 27 Jun 2021 03:31:52 +0900 Subject: [PATCH 6/6] sd-netlink: use usec_sub_unsigned() and USEC_INFINITY And shorten code more. --- src/libsystemd/sd-netlink/sd-netlink.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/libsystemd/sd-netlink/sd-netlink.c b/src/libsystemd/sd-netlink/sd-netlink.c index 819ce71359..f8a1bde6a6 100644 --- a/src/libsystemd/sd-netlink/sd-netlink.c +++ b/src/libsystemd/sd-netlink/sd-netlink.c @@ -525,7 +525,7 @@ static usec_t calc_elapse(uint64_t usec) { return usec_add(now(CLOCK_MONOTONIC), usec); } -static int rtnl_poll(sd_netlink *rtnl, bool need_more, uint64_t timeout_usec) { +static int rtnl_poll(sd_netlink *rtnl, bool need_more, usec_t timeout_usec) { usec_t m = USEC_INFINITY; int r, e; @@ -541,23 +541,18 @@ static int rtnl_poll(sd_netlink *rtnl, bool need_more, uint64_t timeout_usec) { e |= POLLIN; else { usec_t until; + /* Caller wants to process if there is something to * process, but doesn't care otherwise */ r = sd_netlink_get_timeout(rtnl, &until); if (r < 0) return r; - if (r > 0) { - usec_t nw; - nw = now(CLOCK_MONOTONIC); - m = until > nw ? until - nw : 0; - } + + m = usec_sub_unsigned(until, now(CLOCK_MONOTONIC)); } - if (timeout_usec != UINT64_MAX && (m == USEC_INFINITY || timeout_usec < m)) - m = timeout_usec; - - r = fd_wait_for_event(rtnl->fd, e, m); + r = fd_wait_for_event(rtnl->fd, e, MIN(m, timeout_usec)); if (r <= 0) return r; @@ -716,9 +711,9 @@ int sd_netlink_read( if (n >= timeout) return -ETIMEDOUT; - left = timeout - n; + left = usec_sub_unsigned(timeout, n); } else - left = UINT64_MAX; + left = USEC_INFINITY; r = rtnl_poll(rtnl, true, left); if (r < 0)