From 62a489fbf97ae3cbaf36cc1b2ad5260032423385 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 7 May 2026 21:43:32 +0200 Subject: [PATCH 1/7] tree-wide: rename unref_and_replace_full to unref_and_replace_new_ref We have a number of *_unref_and_replace macros. One could think that they are like the various free_and_replace variants, but they actually create a new ref to the passed object. The free_and_replace variants take ownership of the argument. This inconsistency is surprising. Rename all those functions to have "_new_ref" at the end to make the difference clear. --- src/basic/cleanup-util.h | 14 +++++++------- src/libsystemd-network/dhcp-lease-internal.h | 4 ++-- src/libsystemd-network/sd-dhcp-client.c | 6 +++--- src/libsystemd-network/sd-dhcp6-client.c | 2 +- src/libsystemd/sd-device/device-util.h | 4 ++-- src/network/networkd-dhcp6.c | 2 +- src/network/networkd-link.c | 2 +- src/network/networkd-wiphy.c | 4 ++-- src/shared/blockdev-util.c | 2 +- src/udev/udev-manager.c | 2 +- 10 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/basic/cleanup-util.h b/src/basic/cleanup-util.h index 041c37530a..e3a2ade4ed 100644 --- a/src/basic/cleanup-util.h +++ b/src/basic/cleanup-util.h @@ -19,13 +19,13 @@ typedef void* (*mfree_func_t)(void *p); /* This is similar to free_and_replace_full(), but NULL is not assigned to 'b', and its reference counter is * increased. */ -#define unref_and_replace_full(a, b, ref_func, unref_func) \ - ({ \ - typeof(a)* _a = &(a); \ - typeof(b) _b = ref_func(b); \ - unref_func(*_a); \ - *_a = _b; \ - 0; \ +#define unref_and_replace_new_ref(a, b, ref_func, unref_func) \ + ({ \ + typeof(a)* _a = &(a); \ + typeof(b) _b = ref_func(b); \ + unref_func(*_a); \ + *_a = _b; \ + 0; \ }) #define _DEFINE_TRIVIAL_REF_FUNC(type, name, scope) \ diff --git a/src/libsystemd-network/dhcp-lease-internal.h b/src/libsystemd-network/dhcp-lease-internal.h index 744580fc48..1eab7f89b6 100644 --- a/src/libsystemd-network/dhcp-lease-internal.h +++ b/src/libsystemd-network/dhcp-lease-internal.h @@ -97,5 +97,5 @@ void dhcp_lease_set_timestamp(sd_dhcp_lease *lease, const triple_timestamp *time int dhcp_lease_set_default_subnet_mask(sd_dhcp_lease *lease); int dhcp_lease_set_client_id(sd_dhcp_lease *lease, const sd_dhcp_client_id *client_id); -#define dhcp_lease_unref_and_replace(a, b) \ - unref_and_replace_full(a, b, sd_dhcp_lease_ref, sd_dhcp_lease_unref) +#define dhcp_lease_unref_and_replace_new_ref(a, b) \ + unref_and_replace_new_ref(a, b, sd_dhcp_lease_ref, sd_dhcp_lease_unref) diff --git a/src/libsystemd-network/sd-dhcp-client.c b/src/libsystemd-network/sd-dhcp-client.c index 9742ab833b..922dd5881c 100644 --- a/src/libsystemd-network/sd-dhcp-client.c +++ b/src/libsystemd-network/sd-dhcp-client.c @@ -1594,7 +1594,7 @@ static int client_handle_offer_or_rapid_ack(sd_dhcp_client *client, DHCPMessage dhcp_lease_set_timestamp(lease, timestamp); - dhcp_lease_unref_and_replace(client->lease, lease); + dhcp_lease_unref_and_replace_new_ref(client->lease, lease); if (client->lease->rapid_commit) { log_dhcp_client(client, "ACK"); @@ -1678,7 +1678,7 @@ static int client_handle_ack(sd_dhcp_client *client, DHCPMessage *message, size_ else r = SD_DHCP_CLIENT_EVENT_IP_CHANGE; - dhcp_lease_unref_and_replace(client->lease, lease); + dhcp_lease_unref_and_replace_new_ref(client->lease, lease); log_dhcp_client(client, "ACK"); return r; @@ -2281,7 +2281,7 @@ sd_event* sd_dhcp_client_get_event(sd_dhcp_client *client) { int sd_dhcp_client_attach_device(sd_dhcp_client *client, sd_device *dev) { assert_return(client, -EINVAL); - return device_unref_and_replace(client->dev, dev); + return device_unref_and_replace_new_ref(client->dev, dev); } static sd_dhcp_client* dhcp_client_free(sd_dhcp_client *client) { diff --git a/src/libsystemd-network/sd-dhcp6-client.c b/src/libsystemd-network/sd-dhcp6-client.c index ee67664364..5e71d63de5 100644 --- a/src/libsystemd-network/sd-dhcp6-client.c +++ b/src/libsystemd-network/sd-dhcp6-client.c @@ -1533,7 +1533,7 @@ sd_event *sd_dhcp6_client_get_event(sd_dhcp6_client *client) { int sd_dhcp6_client_attach_device(sd_dhcp6_client *client, sd_device *dev) { assert_return(client, -EINVAL); - return device_unref_and_replace(client->dev, dev); + return device_unref_and_replace_new_ref(client->dev, dev); } static sd_dhcp6_client *dhcp6_client_free(sd_dhcp6_client *client) { diff --git a/src/libsystemd/sd-device/device-util.h b/src/libsystemd/sd-device/device-util.h index 3bbe321f61..e4350a679b 100644 --- a/src/libsystemd/sd-device/device-util.h +++ b/src/libsystemd/sd-device/device-util.h @@ -6,8 +6,8 @@ #include "sd-forward.h" #include "log.h" -#define device_unref_and_replace(a, b) \ - unref_and_replace_full(a, b, sd_device_ref, sd_device_unref) +#define device_unref_and_replace_new_ref(a, b) \ + unref_and_replace_new_ref(a, b, sd_device_ref, sd_device_unref) #define FOREACH_DEVICE_PROPERTY(device, key, value) \ for (const char *value, *key = sd_device_get_property_first(device, &value); \ diff --git a/src/network/networkd-dhcp6.c b/src/network/networkd-dhcp6.c index c230a86587..799caeb15b 100644 --- a/src/network/networkd-dhcp6.c +++ b/src/network/networkd-dhcp6.c @@ -364,7 +364,7 @@ static int dhcp6_lease_information_acquired(sd_dhcp6_client *client, Link *link) if (r < 0) return log_link_error_errno(link, r, "Failed to get DHCPv6 lease: %m"); - unref_and_replace_full(link->dhcp6_lease, lease, sd_dhcp6_lease_ref, sd_dhcp6_lease_unref); + unref_and_replace_new_ref(link->dhcp6_lease, lease, sd_dhcp6_lease_ref, sd_dhcp6_lease_unref); link_dirty(link); return 0; diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index c6bc8fc4b1..dcd081039b 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -1678,7 +1678,7 @@ static int link_initialized(Link *link, sd_device *device) { /* Always replace with the new sd_device object. As the sysname (and possibly other properties * or sysattrs) may be outdated. */ - device_unref_and_replace(link->dev, device); + device_unref_and_replace_new_ref(link->dev, device); r = link_managed_by_us(link); if (r <= 0) diff --git a/src/network/networkd-wiphy.c b/src/network/networkd-wiphy.c index 1dde69a43b..f715f244d6 100644 --- a/src/network/networkd-wiphy.c +++ b/src/network/networkd-wiphy.c @@ -469,7 +469,7 @@ int manager_udev_process_wiphy(Manager *m, sd_device *device, sd_device_action_t return 0; } - return device_unref_and_replace(w->dev, action == SD_DEVICE_REMOVE ? NULL : device); + return device_unref_and_replace_new_ref(w->dev, action == SD_DEVICE_REMOVE ? NULL : device); } int manager_udev_process_rfkill(Manager *m, sd_device *device, sd_device_action_t action) { @@ -501,5 +501,5 @@ int manager_udev_process_rfkill(Manager *m, sd_device *device, sd_device_action_ return 0; } - return device_unref_and_replace(w->rfkill, action == SD_DEVICE_REMOVE ? NULL : device); + return device_unref_and_replace_new_ref(w->rfkill, action == SD_DEVICE_REMOVE ? NULL : device); } diff --git a/src/shared/blockdev-util.c b/src/shared/blockdev-util.c index 12a4f59c28..83eb67c541 100644 --- a/src/shared/blockdev-util.c +++ b/src/shared/blockdev-util.c @@ -207,7 +207,7 @@ int block_device_new_from_fd(int fd, BlockDeviceLookupFlags flags, sd_device **r r = block_device_get_originating(dev_whole_disk, &dev_origin, /* recursive= */ false); if (r >= 0) - device_unref_and_replace(dev, dev_origin); + device_unref_and_replace_new_ref(dev, dev_origin); else if (r != -ENOENT) return r; } diff --git a/src/udev/udev-manager.c b/src/udev/udev-manager.c index 7c2530f17f..44d2ee6400 100644 --- a/src/udev/udev-manager.c +++ b/src/udev/udev-manager.c @@ -683,7 +683,7 @@ static void event_find_blocker(Event *event) { log_device_debug(event->dev, "SEQNUM=%" PRIu64 " blocked by SEQNUM=%" PRIu64, event->seqnum, e->seqnum); - unref_and_replace_full(event->blocker, e, event_ref, event_unref); + unref_and_replace_new_ref(event->blocker, e, event_ref, event_unref); return; } From f3ee3510a3ce67bcde3016c6f26faf3a6a9f5150 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 7 May 2026 22:34:11 +0200 Subject: [PATCH 2/7] sd-dhcp-client: avoid taking and dropping a reference The helper would create a new ref, even though we had one handy and didn't need to create a new ref. So change the helper to take an existing reference. --- src/libsystemd-network/dhcp-lease-internal.h | 4 ++-- src/libsystemd-network/sd-dhcp-client.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libsystemd-network/dhcp-lease-internal.h b/src/libsystemd-network/dhcp-lease-internal.h index 1eab7f89b6..3f5d638d67 100644 --- a/src/libsystemd-network/dhcp-lease-internal.h +++ b/src/libsystemd-network/dhcp-lease-internal.h @@ -97,5 +97,5 @@ void dhcp_lease_set_timestamp(sd_dhcp_lease *lease, const triple_timestamp *time int dhcp_lease_set_default_subnet_mask(sd_dhcp_lease *lease); int dhcp_lease_set_client_id(sd_dhcp_lease *lease, const sd_dhcp_client_id *client_id); -#define dhcp_lease_unref_and_replace_new_ref(a, b) \ - unref_and_replace_new_ref(a, b, sd_dhcp_lease_ref, sd_dhcp_lease_unref) +#define dhcp_lease_unref_and_replace(a, b) \ + free_and_replace_full(a, b, sd_dhcp_lease_unref) diff --git a/src/libsystemd-network/sd-dhcp-client.c b/src/libsystemd-network/sd-dhcp-client.c index 922dd5881c..c6a4d02f96 100644 --- a/src/libsystemd-network/sd-dhcp-client.c +++ b/src/libsystemd-network/sd-dhcp-client.c @@ -1594,7 +1594,7 @@ static int client_handle_offer_or_rapid_ack(sd_dhcp_client *client, DHCPMessage dhcp_lease_set_timestamp(lease, timestamp); - dhcp_lease_unref_and_replace_new_ref(client->lease, lease); + dhcp_lease_unref_and_replace(client->lease, lease); if (client->lease->rapid_commit) { log_dhcp_client(client, "ACK"); @@ -1678,7 +1678,7 @@ static int client_handle_ack(sd_dhcp_client *client, DHCPMessage *message, size_ else r = SD_DHCP_CLIENT_EVENT_IP_CHANGE; - dhcp_lease_unref_and_replace_new_ref(client->lease, lease); + dhcp_lease_unref_and_replace(client->lease, lease); log_dhcp_client(client, "ACK"); return r; From 88d7e4439216b1ad2e3ba061dcf6ef99d9af9574 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 7 May 2026 22:53:27 +0200 Subject: [PATCH 3/7] Add json_variant_unref_and_replace and use it where appropriate JSON_VARIANT_REPLACE is similar, but doesn't try to nullify the second arg, so it can be used with an inline expression. --- src/home/homectl.c | 8 ++----- src/home/user-record-util.c | 32 +++++--------------------- src/libsystemd/sd-json/json-util.h | 13 +++++++---- src/libsystemd/sd-json/sd-json.c | 16 ++++++------- src/libsystemd/sd-json/test-json.c | 3 +-- src/libsystemd/sd-varlink/sd-varlink.c | 5 +--- src/resolve/resolvectl.c | 6 ++--- src/resolve/resolved-manager.c | 2 +- src/shared/user-record.c | 6 ++--- 9 files changed, 30 insertions(+), 61 deletions(-) diff --git a/src/home/homectl.c b/src/home/homectl.c index ce54da0d8c..80ee6e2155 100644 --- a/src/home/homectl.c +++ b/src/home/homectl.c @@ -1038,8 +1038,7 @@ static int apply_identity_changes(sd_json_variant **_v) { if (r < 0) return log_error_errno(r, "Failed to allocate new perMachine array: %m"); - sd_json_variant_unref(per_machine); - per_machine = TAKE_PTR(npm); + json_variant_unref_and_replace(per_machine, npm); } else { _cleanup_(sd_json_variant_unrefp) sd_json_variant *positive = sd_json_variant_ref(arg_identity_extra_this_machine), *negative = sd_json_variant_ref(arg_identity_extra_other_machines); @@ -1122,10 +1121,7 @@ static int apply_identity_changes(sd_json_variant **_v) { } } - sd_json_variant_unref(*_v); - *_v = TAKE_PTR(v); - - return 0; + return json_variant_unref_and_replace(*_v, v); } static int add_disposition(sd_json_variant **v) { diff --git a/src/home/user-record-util.c b/src/home/user-record-util.c index b27d993922..b419a85b68 100644 --- a/src/home/user-record-util.c +++ b/src/home/user-record-util.c @@ -376,8 +376,7 @@ int user_record_add_binding( if (r < 0) return r; - sd_json_variant_unref(new_binding_entry); - new_binding_entry = TAKE_PTR(be); + json_variant_unref_and_replace(new_binding_entry, be); } } @@ -787,12 +786,8 @@ int user_record_update_last_changed(UserRecord *h, bool with_password) { } h->last_change_usec = n; - - sd_json_variant_unref(h->json); - h->json = TAKE_PTR(v); - h->mask |= USER_RECORD_REGULAR; - return 0; + return json_variant_unref_and_replace(h->json, v); } int user_record_make_hashed_password(UserRecord *h, char **secret, bool extend) { @@ -1131,12 +1126,8 @@ int user_record_set_password_change_now(UserRecord *h, int b) { SET_FLAG(h->mask, USER_RECORD_PER_MACHINE, !sd_json_variant_is_blank_array(array)); } - sd_json_variant_unref(h->json); - h->json = TAKE_PTR(w); - h->password_change_now = b; - - return 0; + return json_variant_unref_and_replace(h->json, w); } int user_record_merge_secret(UserRecord *h, UserRecord *secret) { @@ -1227,14 +1218,10 @@ int user_record_good_authentication(UserRecord *h) { if (r < 0) return r; - sd_json_variant_unref(h->json); - h->json = TAKE_PTR(v); - h->good_authentication_counter = counter; h->last_good_authentication_usec = usec; - h->mask |= USER_RECORD_STATUS; - return 0; + return json_variant_unref_and_replace(h->json, v); } int user_record_bad_authentication(UserRecord *h) { @@ -1282,14 +1269,10 @@ int user_record_bad_authentication(UserRecord *h) { if (r < 0) return r; - sd_json_variant_unref(h->json); - h->json = TAKE_PTR(v); - h->bad_authentication_counter = counter; h->last_bad_authentication_usec = usec; - h->mask |= USER_RECORD_STATUS; - return 0; + return json_variant_unref_and_replace(h->json, v); } int user_record_ratelimit(UserRecord *h) { @@ -1344,13 +1327,10 @@ int user_record_ratelimit(UserRecord *h) { if (r < 0) return r; - sd_json_variant_unref(h->json); - h->json = TAKE_PTR(v); - h->ratelimit_begin_usec = new_ratelimit_begin_usec; h->ratelimit_count = new_ratelimit_count; - h->mask |= USER_RECORD_STATUS; + json_variant_unref_and_replace(h->json, v); return 1; } diff --git a/src/libsystemd/sd-json/json-util.h b/src/libsystemd/sd-json/json-util.h index 0db1e445e6..6f06f6fb63 100644 --- a/src/libsystemd/sd-json/json-util.h +++ b/src/libsystemd/sd-json/json-util.h @@ -9,14 +9,17 @@ #include "sd-forward.h" #include "string-util.h" /* IWYU pragma: keep */ -#define JSON_VARIANT_REPLACE(v, q) \ - do { \ - typeof(v)* _v = &(v); \ - typeof(q) _q = (q); \ +#define JSON_VARIANT_REPLACE(v, q) \ + do { \ + typeof(v)* _v = &(v); \ + typeof(q) _q = (q); \ sd_json_variant_unref(*_v); \ - *_v = _q; \ + *_v = _q; \ } while(false) +#define json_variant_unref_and_replace(a, b) \ + free_and_replace_full(a, b, sd_json_variant_unref) + static inline int json_variant_set_field_non_null(sd_json_variant **v, const char *field, sd_json_variant *value) { return value && !sd_json_variant_is_null(value) ? sd_json_variant_set_field(v, field, value) : 0; } diff --git a/src/libsystemd/sd-json/sd-json.c b/src/libsystemd/sd-json/sd-json.c index 659dffb2ba..fe8b8225c9 100644 --- a/src/libsystemd/sd-json/sd-json.c +++ b/src/libsystemd/sd-json/sd-json.c @@ -1974,7 +1974,7 @@ _public_ int sd_json_variant_filter(sd_json_variant **v, char **to_remove) { return r; json_variant_propagate_sensitive(*v, w); - JSON_VARIANT_REPLACE(*v, TAKE_PTR(w)); + json_variant_unref_and_replace(*v, w); return (int) n; } @@ -2043,7 +2043,7 @@ _public_ int sd_json_variant_set_field(sd_json_variant **v, const char *field, s return r; json_variant_propagate_sensitive(*v, w); - JSON_VARIANT_REPLACE(*v, TAKE_PTR(w)); + json_variant_unref_and_replace(*v, w); return 1; } @@ -2183,7 +2183,7 @@ _public_ int sd_json_variant_merge_object(sd_json_variant **v, sd_json_variant * json_variant_propagate_sensitive(*v, w); json_variant_propagate_sensitive(m, w); - JSON_VARIANT_REPLACE(*v, TAKE_PTR(w)); + json_variant_unref_and_replace(*v, w); return 1; } @@ -2262,9 +2262,7 @@ _public_ int sd_json_variant_append_array(sd_json_variant **v, sd_json_variant * } json_variant_propagate_sensitive(*v, nv); - JSON_VARIANT_REPLACE(*v, TAKE_PTR(nv)); - - return 0; + return json_variant_unref_and_replace(*v, nv); } _public_ int sd_json_variant_append_arrayb(sd_json_variant **v, ...) { @@ -2511,7 +2509,7 @@ static int json_variant_set_source(sd_json_variant **v, JsonSource *source, unsi w->line = line; w->column = column; - JSON_VARIANT_REPLACE(*v, w); + json_variant_unref_and_replace(*v, w); return 1; } @@ -5855,7 +5853,7 @@ _public_ int sd_json_variant_sort(sd_json_variant **v) { if (!n->sorted) /* Check if this worked. This will fail if there are multiple identical keys used. */ return -ENOTUNIQ; - JSON_VARIANT_REPLACE(*v, TAKE_PTR(n)); + json_variant_unref_and_replace(*v, n); return 1; } @@ -5910,7 +5908,7 @@ _public_ int sd_json_variant_normalize(sd_json_variant **v) { goto finish; } - JSON_VARIANT_REPLACE(*v, TAKE_PTR(n)); + json_variant_unref_and_replace(*v, n); r = 1; diff --git a/src/libsystemd/sd-json/test-json.c b/src/libsystemd/sd-json/test-json.c index 1785ef416f..3be4b09660 100644 --- a/src/libsystemd/sd-json/test-json.c +++ b/src/libsystemd/sd-json/test-json.c @@ -548,8 +548,7 @@ TEST(depth) { assert_se(r >= 0); - sd_json_variant_unref(v); - v = TAKE_PTR(w); + json_variant_unref_and_replace(v, w); } sd_json_variant_dump(v, 0, stdout, NULL); diff --git a/src/libsystemd/sd-varlink/sd-varlink.c b/src/libsystemd/sd-varlink/sd-varlink.c index 2a5f677ef3..8e43e38800 100644 --- a/src/libsystemd/sd-varlink/sd-varlink.c +++ b/src/libsystemd/sd-varlink/sd-varlink.c @@ -772,10 +772,7 @@ static int varlink_sanitize_incoming_parameters(sd_json_variant **v) { r = sd_json_variant_new_object(&empty, NULL, 0); if (r < 0) return r; - /* sd_json_variant_unref() is a NOP if *v is NULL */ - sd_json_variant_unref(*v); - *v = TAKE_PTR(empty); - return 0; + return json_variant_unref_and_replace(*v, empty); } /* Ensure we have an object */ diff --git a/src/resolve/resolvectl.c b/src/resolve/resolvectl.c index 015345fdba..e8ae7c5412 100644 --- a/src/resolve/resolvectl.c +++ b/src/resolve/resolvectl.c @@ -1251,8 +1251,7 @@ static int status_json_filter_links(sd_json_variant **configuration, char **link return r; } - JSON_VARIANT_REPLACE(*configuration, TAKE_PTR(v)); - return 0; + return json_variant_unref_and_replace(*configuration, v); } static int status_json_filter_fields(sd_json_variant **configuration, StatusMode mode) { @@ -1281,8 +1280,7 @@ static int status_json_filter_fields(sd_json_variant **configuration, StatusMode return r; } - JSON_VARIANT_REPLACE(*configuration, TAKE_PTR(v)); - return 0; + return json_variant_unref_and_replace(*configuration, v); } static int format_dns_server_one(DNSConfiguration *configuration, DNSServer *s, char **ret) { diff --git a/src/resolve/resolved-manager.c b/src/resolve/resolved-manager.c index 0a52e922c4..d7d7077265 100644 --- a/src/resolve/resolved-manager.c +++ b/src/resolve/resolved-manager.c @@ -2335,7 +2335,7 @@ int manager_send_dns_configuration_changed(Manager *m, Link *l, bool reset) { if (sd_json_variant_equal(configuration, m->dns_configuration_json)) return 0; - JSON_VARIANT_REPLACE(m->dns_configuration_json, TAKE_PTR(configuration)); + json_variant_unref_and_replace(m->dns_configuration_json, configuration); r = varlink_many_notify(m->varlink_dns_configuration_subscription, m->dns_configuration_json); if (r < 0) diff --git a/src/shared/user-record.c b/src/shared/user-record.c index 191870c8ea..7c64171bf5 100644 --- a/src/shared/user-record.c +++ b/src/shared/user-record.c @@ -2374,8 +2374,7 @@ static int remove_self_modifiable_json_fields_common(UserRecord *current, sd_jso return r; } - JSON_VARIANT_REPLACE(*target, TAKE_PTR(v)); - return 0; + return json_variant_unref_and_replace(*target, v); } static int remove_self_modifiable_json_fields(UserRecord *current, UserRecord *h, sd_json_variant **ret) { @@ -2460,8 +2459,7 @@ static int remove_self_modifiable_json_fields(UserRecord *current, UserRecord *h return r; } - JSON_VARIANT_REPLACE(*ret, TAKE_PTR(v)); - return 0; + return json_variant_unref_and_replace(*ret, v); } int user_record_self_changes_allowed(UserRecord *current, UserRecord *incoming) { From 576ca62b9b1b76c5621d29b9a3895ab21e645740 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 7 May 2026 22:55:00 +0200 Subject: [PATCH 4/7] userdbctl: split out parse_from_file() parse_from_file doesn't set arg_from_file itself, but returns a sd_json_variant ref to the caller. I think the change of arg_from_file is more readable with this structure. --- src/userdb/userdbctl.c | 40 +++++++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/src/userdb/userdbctl.c b/src/userdb/userdbctl.c index 6b4371aa50..75b30215b1 100644 --- a/src/userdb/userdbctl.c +++ b/src/userdb/userdbctl.c @@ -19,6 +19,7 @@ #include "format-util.h" #include "fs-util.h" #include "io-util.h" +#include "json-util.h" #include "log.h" #include "main-func.h" #include "mkdir.h" @@ -1590,6 +1591,30 @@ static int verb_help(int argc, char *argv[], uintptr_t _data, void *userdata) { return help(); } +static int parse_from_file(const char *arg, sd_json_variant **ret) { + sd_json_variant *v = NULL; + int r; + + assert(ret); + + if (!isempty(arg)) { + const char *fn = streq(arg, "-") ? NULL : arg; + unsigned line = 0; + r = sd_json_parse_file( + fn ? NULL : stdin, + fn ?: "", + SD_JSON_PARSE_MUST_BE_OBJECT | SD_JSON_PARSE_SENSITIVE, + &v, + &line, + /* reterr_column= */ NULL); + if (r < 0) + return log_syntax(/* unit= */ NULL, LOG_ERR, fn ?: "", line, r, "JSON parse failure."); + } + + *ret = v; + return 0; +} + static int parse_argv(int argc, char *argv[]) { enum { @@ -1832,20 +1857,13 @@ static int parse_argv(int argc, char *argv[]) { break; case 'F': { - if (isempty(optarg)) { - arg_from_file = sd_json_variant_unref(arg_from_file); - break; - } + sd_json_variant *v = NULL; /* initialization to appease gcc-14 */ - _cleanup_(sd_json_variant_unrefp) sd_json_variant *v = NULL; - const char *fn = streq(optarg, "-") ? NULL : optarg; - unsigned line = 0; - r = sd_json_parse_file(fn ? NULL : stdin, fn ?: "", SD_JSON_PARSE_MUST_BE_OBJECT|SD_JSON_PARSE_SENSITIVE, &v, &line, /* reterr_column= */ NULL); + r = parse_from_file(optarg, &v); if (r < 0) - return log_syntax(/* unit= */ NULL, LOG_ERR, fn ?: "", line, r, "JSON parse failure."); + return r; - sd_json_variant_unref(arg_from_file); - arg_from_file = TAKE_PTR(v); + json_variant_unref_and_replace(arg_from_file, v); break; } From cb8359877ecb4b361a4ff74ac839a4a55986560c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 May 2026 08:25:10 +0200 Subject: [PATCH 5/7] userdbctl: convert to OPTION and VERB macros MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The situation with --chain is complicated. The old code tried to use "+…" in getopt_long() to stop option parsing. But it didn't actually work. This logic was originally added in 8072a7e6a9eaf2de120797dd16c5e0baea606219. ef9c12b157a50d63e8a8eb710c013d16c2cea319 added an comment about 'optind=0' which explains why the code doesn't work, but the code wasn't changed. To wit: $ userdbctl.old --no-pager --chain ssh-authorized-keys zbyszek -- /bin/echo --asdf --asdf $ userdbctl.old --no-pager --chain ssh-authorized-keys zbyszek /bin/echo -- --asdf --asdf $ userdbctl.old --no-pager --chain ssh-authorized-keys zbyszek /bin/echo --asdf userdbctl.old: unrecognized option '--asdf' (Basically, if "--" is used, it can be anywhere, since getopt_long() doesn't do anything special after --chain and looks for the next option. There were some tests of --chain, but they all used the username as the positional argument, so it wasn't misinterpreted as an option.) This behaviour is preserved in the conversion. --help is generally the same except for expected formatting changes. --json= is moved above between --output= and -j. For some reason it was further down. Co-developed-by: Claude Opus 4.7 --- src/userdb/userdbctl.c | 344 ++++++++++++++++------------------------- 1 file changed, 136 insertions(+), 208 deletions(-) diff --git a/src/userdb/userdbctl.c b/src/userdb/userdbctl.c index 75b30215b1..60a6ff051a 100644 --- a/src/userdb/userdbctl.c +++ b/src/userdb/userdbctl.c @@ -1,10 +1,10 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ -#include #include #include #include "alloc-util.h" +#include "ansi-color.h" #include "bitfield.h" #include "build.h" #include "copy.h" @@ -18,14 +18,16 @@ #include "format-table.h" #include "format-util.h" #include "fs-util.h" +#include "glyph-util.h" +#include "help-util.h" #include "io-util.h" #include "json-util.h" #include "log.h" #include "main-func.h" #include "mkdir.h" +#include "options.h" #include "pager.h" #include "parse-argument.h" -#include "pretty-print.h" #include "recurse-dir.h" #include "socket-util.h" #include "string-table.h" @@ -408,6 +410,8 @@ static int table_add_uid_map( return n_added; } +VERB(verb_display_user, "user", "[USER…]", VERB_ANY, VERB_ANY, VERB_DEFAULT, + "Inspect user"); static int verb_display_user(int argc, char *argv[], uintptr_t _data, void *userdata) { _cleanup_(table_unrefp) Table *table = NULL; bool draw_separator = false; @@ -751,6 +755,8 @@ static int add_unavailable_gid(Table *table, uid_t start, uid_t end) { return 2; } +VERB(verb_display_group, "group", "[GROUP…]", VERB_ANY, VERB_ANY, 0, + "Inspect group"); static int verb_display_group(int argc, char *argv[], uintptr_t _data, void *userdata) { _cleanup_(table_unrefp) Table *table = NULL; bool draw_separator = false; @@ -952,6 +958,10 @@ static int show_membership(const char *user, const char *group, Table *table) { return 0; } +VERB(verb_display_memberships, "users-in-group", "[GROUP…]", VERB_ANY, VERB_ANY, 0, + "Show users that are members of specified groups"); +VERB(verb_display_memberships, "groups-of-user", "[USER…]", VERB_ANY, VERB_ANY, 0, + "Show groups the specified users are members of"); static int verb_display_memberships(int argc, char *argv[], uintptr_t _data, void *userdata) { _cleanup_(table_unrefp) Table *table = NULL; int ret = 0, r; @@ -1048,6 +1058,8 @@ static int verb_display_memberships(int argc, char *argv[], uintptr_t _data, voi return ret; } +VERB(verb_display_services, "services", NULL, VERB_ANY, 1, 0, + "Show enabled database services"); static int verb_display_services(int argc, char *argv[], uintptr_t _data, void *userdata) { _cleanup_(table_unrefp) Table *t = NULL; _cleanup_closedir_ DIR *d = NULL; @@ -1111,8 +1123,11 @@ static int verb_display_services(int argc, char *argv[], uintptr_t _data, void * return 0; } +VERB(verb_ssh_authorized_keys, "ssh-authorized-keys", "USER", 2, VERB_ANY, 0, + "Show SSH authorized keys for user"); static int verb_ssh_authorized_keys(int argc, char *argv[], uintptr_t _data, void *userdata) { _cleanup_(user_record_unrefp) UserRecord *ur = NULL; + const char *username; char **chain_invocation; int r; @@ -1121,6 +1136,8 @@ static int verb_ssh_authorized_keys(int argc, char *argv[], uintptr_t _data, voi if (arg_from_file) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "--from-file= not supported when showing SSH authorized keys, refusing."); + username = argv[1]; + if (arg_chain) { /* If --chain is specified, the rest of the command line is the chain command */ @@ -1147,18 +1164,18 @@ static int verb_ssh_authorized_keys(int argc, char *argv[], uintptr_t _data, voi chain_invocation = NULL; } - r = userdb_by_name(argv[1], /* match= */ NULL, arg_userdb_flags, &ur); + r = userdb_by_name(username, /* match= */ NULL, arg_userdb_flags, &ur); if (r == -ESRCH) - log_error_errno(r, "User %s does not exist.", argv[1]); + log_error_errno(r, "User %s does not exist.", username); else if (r == -EHOSTDOWN) log_error_errno(r, "Selected user database service is not available for this request."); else if (r == -EINVAL) - log_error_errno(r, "Failed to find user %s: %m (Invalid user name?)", argv[1]); + log_error_errno(r, "Failed to find user %s: %m (Invalid user name?)", username); else if (r < 0) - log_error_errno(r, "Failed to find user %s: %m", argv[1]); + log_error_errno(r, "Failed to find user %s: %m", username); else { if (strv_isempty(ur->ssh_authorized_keys)) - log_debug("User record for %s has no public SSH keys.", argv[1]); + log_debug("User record for %s has no public SSH keys.", username); else STRV_FOREACH(i, ur->ssh_authorized_keys) printf("%s\n", *i); @@ -1494,6 +1511,8 @@ static int load_credential_one( return 0; } +VERB(verb_load_credentials, "load-credentials", NULL, VERB_ANY, 1, 0, + "Write static user/group records from credentials"); static int verb_load_credentials(int argc, char *argv[], uintptr_t _data, void *userdata) { int r; @@ -1530,66 +1549,39 @@ static int verb_load_credentials(int argc, char *argv[], uintptr_t _data, void * } static int help(void) { - _cleanup_free_ char *link = NULL; + _cleanup_(table_unrefp) Table *verbs = NULL, *options = NULL; int r; + r = verbs_get_help_table(&verbs); + if (r < 0) + return r; + + r = option_parser_get_help_table(&options); + if (r < 0) + return r; + + (void) table_sync_column_widths(0, verbs, options); + pager_open(arg_pager_flags); - r = terminal_urlify_man("userdbctl", "1", &link); + help_cmdline("[OPTIONS…] COMMAND …"); + help_abstract("Show user and group information."); + + help_section("Commands"); + r = table_print_or_warn(verbs); if (r < 0) - return log_oom(); + return r; - printf("%s [OPTIONS...] COMMAND ...\n\n" - "%sShow user and group information.%s\n" - "\nCommands:\n" - " user [USER…] Inspect user\n" - " group [GROUP…] Inspect group\n" - " users-in-group [GROUP…] Show users that are members of specified groups\n" - " groups-of-user [USER…] Show groups the specified users are members of\n" - " services Show enabled database services\n" - " ssh-authorized-keys USER Show SSH authorized keys for user\n" - " load-credentials Write static user/group records from credentials\n" - "\nOptions:\n" - " -h --help Show this help\n" - " --version Show package version\n" - " --no-pager Do not pipe output into a pager\n" - " --no-legend Do not show the headers and footers\n" - " --output=MODE Select output mode (classic, friendly, table, json)\n" - " -j Equivalent to --output=json\n" - " -s --service=SERVICE[:SERVICE…]\n" - " Query the specified service\n" - " --with-nss=BOOL Control whether to include glibc NSS data\n" - " -N Do not synthesize or include glibc NSS data\n" - " (Same as --synthesize=no --with-nss=no)\n" - " --synthesize=BOOL Synthesize root/nobody user\n" - " --with-dropin=BOOL Control whether to include drop-in records\n" - " --with-varlink=BOOL Control whether to talk to services at all\n" - " --multiplexer=BOOL Control whether to use the multiplexer\n" - " --json=pretty|short JSON output mode\n" - " --chain Chain another command\n" - " --uid-min=ID Filter by minimum UID/GID (default 0)\n" - " --uid-max=ID Filter by maximum UID/GID (default 4294967294)\n" - " --uuid=UUID Filter by UUID\n" - " -z --fuzzy Do a fuzzy name search\n" - " --disposition=VALUE Filter by disposition\n" - " -I Equivalent to --disposition=intrinsic\n" - " -S Equivalent to --disposition=system\n" - " -R Equivalent to --disposition=regular\n" - " --boundaries=BOOL Show/hide UID/GID range boundaries in output\n" - " -B Equivalent to --boundaries=no\n" - " -F --from-file=PATH Read JSON record from file\n" - "\nSee the %s for details.\n", - program_invocation_short_name, - ansi_highlight(), - ansi_normal(), - link); + help_section("Options"); + r = table_print_or_warn(options); + if (r < 0) + return r; + help_man_page_reference("userdbctl", "1"); return 0; } -static int verb_help(int argc, char *argv[], uintptr_t _data, void *userdata) { - return help(); -} +VERB_COMMON_HELP_HIDDEN(help); static int parse_from_file(const char *arg, sd_json_variant **ret) { sd_json_variant *v = NULL; @@ -1615,56 +1607,13 @@ static int parse_from_file(const char *arg, sd_json_variant **ret) { return 0; } -static int parse_argv(int argc, char *argv[]) { - - enum { - ARG_VERSION = 0x100, - ARG_NO_PAGER, - ARG_NO_LEGEND, - ARG_OUTPUT, - ARG_WITH_NSS, - ARG_WITH_DROPIN, - ARG_WITH_VARLINK, - ARG_SYNTHESIZE, - ARG_MULTIPLEXER, - ARG_JSON, - ARG_CHAIN, - ARG_UID_MIN, - ARG_UID_MAX, - ARG_UUID, - ARG_DISPOSITION, - ARG_BOUNDARIES, - }; - - static const struct option options[] = { - { "help", no_argument, NULL, 'h' }, - { "version", no_argument, NULL, ARG_VERSION }, - { "no-pager", no_argument, NULL, ARG_NO_PAGER }, - { "no-legend", no_argument, NULL, ARG_NO_LEGEND }, - { "output", required_argument, NULL, ARG_OUTPUT }, - { "service", required_argument, NULL, 's' }, - { "with-nss", required_argument, NULL, ARG_WITH_NSS }, - { "with-dropin", required_argument, NULL, ARG_WITH_DROPIN }, - { "with-varlink", required_argument, NULL, ARG_WITH_VARLINK }, - { "synthesize", required_argument, NULL, ARG_SYNTHESIZE }, - { "multiplexer", required_argument, NULL, ARG_MULTIPLEXER }, - { "json", required_argument, NULL, ARG_JSON }, - { "chain", no_argument, NULL, ARG_CHAIN }, - { "uid-min", required_argument, NULL, ARG_UID_MIN }, - { "uid-max", required_argument, NULL, ARG_UID_MAX }, - { "uuid", required_argument, NULL, ARG_UUID }, - { "fuzzy", no_argument, NULL, 'z' }, - { "disposition", required_argument, NULL, ARG_DISPOSITION }, - { "boundaries", required_argument, NULL, ARG_BOUNDARIES }, - { "from-file", required_argument, NULL, 'F' }, - {} - }; - +static int parse_argv(int argc, char *argv[], char ***remaining_args) { const char *e; int r; assert(argc >= 0); assert(argv); + assert(remaining_args); /* We are going to update this environment variable with our own, hence let's first read what is already set */ e = getenv("SYSTEMD_ONLY_USERDB"); @@ -1679,122 +1628,137 @@ static int parse_argv(int argc, char *argv[]) { arg_services = l; } - /* Resetting to 0 forces the invocation of an internal initialization routine of getopt_long() - * that checks for GNU extensions in optstring ('-' or '+' at the beginning). */ - optind = 0; - - for (;;) { - int c; - - c = getopt_long(argc, argv, - arg_chain ? "+hjs:NISRzBF:" : "hjs:NISRzBF:", /* When --chain was used disable parsing of further switches */ - options, NULL); - if (c < 0) - break; + OptionParser opts = { argc, argv }; + FOREACH_OPTION_OR_RETURN(c, &opts) switch (c) { - case 'h': + OPTION_COMMON_HELP: return help(); - case ARG_VERSION: + OPTION_COMMON_VERSION: return version(); - case ARG_NO_PAGER: + OPTION_COMMON_NO_PAGER: arg_pager_flags |= PAGER_DISABLE; break; - case ARG_NO_LEGEND: + OPTION_COMMON_NO_LEGEND: arg_legend = false; break; - case ARG_OUTPUT: - if (streq(optarg, "help")) + OPTION_LONG("output", "MODE", + "Select output mode (classic, friendly, table, json)"): + if (streq(opts.arg, "help")) return DUMP_STRING_TABLE(output, Output, _OUTPUT_MAX); - arg_output = output_from_string(optarg); + arg_output = output_from_string(opts.arg); if (arg_output < 0) - return log_error_errno(arg_output, "Invalid --output= mode: %s", optarg); + return log_error_errno(arg_output, "Invalid --output= mode: %s", opts.arg); arg_json_format_flags = arg_output == OUTPUT_JSON ? SD_JSON_FORMAT_PRETTY|SD_JSON_FORMAT_COLOR_AUTO : SD_JSON_FORMAT_OFF; break; - case ARG_JSON: - r = parse_json_argument(optarg, &arg_json_format_flags); + OPTION_COMMON_JSON: + r = parse_json_argument(opts.arg, &arg_json_format_flags); if (r <= 0) return r; arg_output = sd_json_format_enabled(arg_json_format_flags) ? OUTPUT_JSON : _OUTPUT_INVALID; break; - case 'j': + OPTION_SHORT('j', NULL, "Equivalent to --output=json"): arg_json_format_flags = SD_JSON_FORMAT_PRETTY|SD_JSON_FORMAT_COLOR_AUTO; arg_output = OUTPUT_JSON; break; - case 's': - if (isempty(optarg)) + OPTION('s', "service", "SERVICE[:SERVICE…]", "Query the specified service"): + if (isempty(opts.arg)) arg_services = strv_free(arg_services); else { - r = strv_split_and_extend(&arg_services, optarg, ":", /* filter_duplicates= */ true); + r = strv_split_and_extend(&arg_services, opts.arg, ":", /* filter_duplicates= */ true); if (r < 0) return log_error_errno(r, "Failed to parse -s/--service= argument: %m"); } break; - case 'N': - arg_userdb_flags |= USERDB_EXCLUDE_NSS|USERDB_DONT_SYNTHESIZE_INTRINSIC|USERDB_DONT_SYNTHESIZE_FOREIGN; - break; - - case ARG_WITH_NSS: - r = parse_boolean_argument("--with-nss=", optarg, NULL); + OPTION_LONG("with-nss", "BOOL", "Control whether to include glibc NSS data"): + r = parse_boolean_argument("--with-nss=", opts.arg, NULL); if (r < 0) return r; SET_FLAG(arg_userdb_flags, USERDB_EXCLUDE_NSS, !r); break; - case ARG_WITH_DROPIN: - r = parse_boolean_argument("--with-dropin=", optarg, NULL); - if (r < 0) - return r; - - SET_FLAG(arg_userdb_flags, USERDB_EXCLUDE_DROPIN, !r); + OPTION_SHORT('N', NULL, + "Do not synthesize or include glibc NSS data " + "(Same as --synthesize=no --with-nss=no)"): + arg_userdb_flags |= USERDB_EXCLUDE_NSS|USERDB_DONT_SYNTHESIZE_INTRINSIC|USERDB_DONT_SYNTHESIZE_FOREIGN; break; - case ARG_WITH_VARLINK: - r = parse_boolean_argument("--with-varlink=", optarg, NULL); - if (r < 0) - return r; - - SET_FLAG(arg_userdb_flags, USERDB_EXCLUDE_VARLINK, !r); - break; - - case ARG_SYNTHESIZE: - r = parse_boolean_argument("--synthesize=", optarg, NULL); + OPTION_LONG("synthesize", "BOOL", "Synthesize root/nobody user"): + r = parse_boolean_argument("--synthesize=", opts.arg, NULL); if (r < 0) return r; SET_FLAG(arg_userdb_flags, USERDB_DONT_SYNTHESIZE_INTRINSIC|USERDB_DONT_SYNTHESIZE_FOREIGN, !r); break; - case ARG_MULTIPLEXER: - r = parse_boolean_argument("--multiplexer=", optarg, NULL); + OPTION_LONG("with-dropin", "BOOL", "Control whether to include drop-in records"): + r = parse_boolean_argument("--with-dropin=", opts.arg, NULL); + if (r < 0) + return r; + + SET_FLAG(arg_userdb_flags, USERDB_EXCLUDE_DROPIN, !r); + break; + + OPTION_LONG("with-varlink", "BOOL", "Control whether to talk to services at all"): + r = parse_boolean_argument("--with-varlink=", opts.arg, NULL); + if (r < 0) + return r; + + SET_FLAG(arg_userdb_flags, USERDB_EXCLUDE_VARLINK, !r); + break; + + OPTION_LONG("multiplexer", "BOOL", "Control whether to use the multiplexer"): + r = parse_boolean_argument("--multiplexer=", opts.arg, NULL); if (r < 0) return r; SET_FLAG(arg_userdb_flags, USERDB_AVOID_MULTIPLEXER, !r); break; - case ARG_CHAIN: + OPTION_LONG("chain", NULL, "Chain another command"): arg_chain = true; break; - case ARG_DISPOSITION: { - UserDisposition d = user_disposition_from_string(optarg); + OPTION_LONG("uid-min", "ID", "Filter by minimum UID/GID (default 0)"): + r = parse_uid(opts.arg, &arg_uid_min); + if (r < 0) + return log_error_errno(r, "Failed to parse --uid-min= value: %s", opts.arg); + break; + + OPTION_LONG("uid-max", "ID", "Filter by maximum UID/GID (default 4294967294)"): + r = parse_uid(opts.arg, &arg_uid_max); + if (r < 0) + return log_error_errno(r, "Failed to parse --uid-max= value: %s", opts.arg); + break; + + OPTION_LONG("uuid", "UUID", "Filter by UUID"): + r = sd_id128_from_string(opts.arg, &arg_uuid); + if (r < 0) + return log_error_errno(r, "Failed to parse --uuid= value: %s", opts.arg); + break; + + OPTION('z', "fuzzy", NULL, "Do a fuzzy name search"): + arg_fuzzy = true; + break; + + OPTION_LONG("disposition", "VALUE", "Filter by disposition"): { + UserDisposition d = user_disposition_from_string(opts.arg); if (d < 0) - return log_error_errno(d, "Unknown user disposition: %s", optarg); + return log_error_errno(d, "Unknown user disposition: %s", opts.arg); if (arg_disposition_mask == UINT64_MAX) arg_disposition_mask = 0; @@ -1803,80 +1767,54 @@ static int parse_argv(int argc, char *argv[]) { break; } - case 'I': + OPTION_SHORT('I', NULL, "Equivalent to --disposition=intrinsic"): if (arg_disposition_mask == UINT64_MAX) arg_disposition_mask = 0; arg_disposition_mask |= UINT64_C(1) << USER_INTRINSIC; break; - case 'S': + OPTION_SHORT('S', NULL, "Equivalent to --disposition=system"): if (arg_disposition_mask == UINT64_MAX) arg_disposition_mask = 0; arg_disposition_mask |= UINT64_C(1) << USER_SYSTEM; break; - case 'R': + OPTION_SHORT('R', NULL, "Equivalent to --disposition=regular"): if (arg_disposition_mask == UINT64_MAX) arg_disposition_mask = 0; arg_disposition_mask |= UINT64_C(1) << USER_REGULAR; break; - case ARG_UID_MIN: - r = parse_uid(optarg, &arg_uid_min); - if (r < 0) - return log_error_errno(r, "Failed to parse --uid-min= value: %s", optarg); - break; - - case ARG_UID_MAX: - r = parse_uid(optarg, &arg_uid_max); - if (r < 0) - return log_error_errno(r, "Failed to parse --uid-max= value: %s", optarg); - break; - - case ARG_UUID: - r = sd_id128_from_string(optarg, &arg_uuid); - if (r < 0) - return log_error_errno(r, "Failed to parse --uuid= value: %s", optarg); - break; - - case 'z': - arg_fuzzy = true; - break; - - case ARG_BOUNDARIES: - r = parse_boolean_argument("boundaries", optarg, &arg_boundaries); + OPTION_LONG("boundaries", "BOOL", + "Show/hide UID/GID range boundaries in output"): + r = parse_boolean_argument("boundaries", opts.arg, &arg_boundaries); if (r < 0) return r; break; - case 'B': + OPTION_SHORT('B', NULL, "Equivalent to --boundaries=no"): arg_boundaries = false; break; - case 'F': { + OPTION('F', "from-file", "PATH", "Read JSON record from file"): { sd_json_variant *v = NULL; /* initialization to appease gcc-14 */ - r = parse_from_file(optarg, &v); + r = parse_from_file(opts.arg, &v); if (r < 0) return r; json_variant_unref_and_replace(arg_from_file, v); break; } - - case '?': - return -EINVAL; - - default: - assert_not_reached(); } - } if (arg_uid_min > arg_uid_max) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Minimum UID/GID " UID_FMT " is above maximum UID/GID " UID_FMT ", refusing.", arg_uid_min, arg_uid_max); + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Minimum UID/GID " UID_FMT " is above maximum UID/GID " UID_FMT ", refusing.", + arg_uid_min, arg_uid_max); /* If not mask was specified, use the all bits on mask */ if (arg_disposition_mask == UINT64_MAX) @@ -1885,27 +1823,17 @@ static int parse_argv(int argc, char *argv[]) { if (arg_from_file) arg_boundaries = false; + *remaining_args = option_parser_get_args(&opts); return 1; } static int run(int argc, char *argv[]) { - static const Verb verbs[] = { - { "help", VERB_ANY, VERB_ANY, 0, verb_help }, - { "user", VERB_ANY, VERB_ANY, VERB_DEFAULT, verb_display_user }, - { "group", VERB_ANY, VERB_ANY, 0, verb_display_group }, - { "users-in-group", VERB_ANY, VERB_ANY, 0, verb_display_memberships }, - { "groups-of-user", VERB_ANY, VERB_ANY, 0, verb_display_memberships }, - { "services", VERB_ANY, 1, 0, verb_display_services }, - { "ssh-authorized-keys", 2, VERB_ANY, 0, verb_ssh_authorized_keys }, - { "load-credentials", VERB_ANY, 1, 0, verb_load_credentials }, - {} - }; - + char **args = NULL; int r; log_setup(); - r = parse_argv(argc, argv); + r = parse_argv(argc, argv, &args); if (r <= 0) return r; @@ -1923,7 +1851,7 @@ static int run(int argc, char *argv[]) { } else assert_se(unsetenv("SYSTEMD_ONLY_USERDB") == 0); - return dispatch_verb(argc, argv, verbs, NULL); + return dispatch_verb_with_args(args, NULL); } DEFINE_MAIN_FUNCTION(run); From b2c6cc6a8e0c11bb59b46999ab4c865dd448ccba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 May 2026 15:47:18 +0200 Subject: [PATCH 6/7] userdbctl: actually implement option parsing stop after --chain The basic idea is that --chain should stop option parsing. But previously this didn't work, so --chain could be specified anywhere in the command line. To maintain with compatibility with that, allow --chain to be specified anywhere until the first positional arg or option in the command string. This allows options to be passed in the expected fashion: userdbctl --chain ssh-authorized-keys user cmd --opt1 --opt2 userdbctl --chain ssh-authorized-keys user -- cmd --opt1 --opt2 but also allows the invocations which worked previously: userdbctl ssh-authorized-keys user --chain cmd userdbctl ssh-authorized-keys user cmd --chain Fixes 8072a7e6a9eaf2de120797dd16c5e0baea606219. The error messages are extended a bit. "binary path" is misleading: we support all kinds of executables, not only compiled programs. --- src/userdb/userdbctl.c | 34 ++++++++++++++++++++++++++++------ test/units/TEST-46-HOMED.sh | 9 +++++++++ 2 files changed, 37 insertions(+), 6 deletions(-) diff --git a/src/userdb/userdbctl.c b/src/userdb/userdbctl.c index 60a6ff051a..35a17c5021 100644 --- a/src/userdb/userdbctl.c +++ b/src/userdb/userdbctl.c @@ -1148,11 +1148,13 @@ static int verb_ssh_authorized_keys(int argc, char *argv[], uintptr_t _data, voi /* Make similar restrictions on the chain command as OpenSSH itself makes on the primary command. */ if (!path_is_absolute(argv[2])) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Chain invocation of ssh-authorized-keys commands requires an absolute binary path argument."); + "Chain invocation of ssh-authorized-keys commands requires an absolute program path (got '%s').", + argv[2]); if (!path_is_normalized(argv[2])) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Chain invocation of ssh-authorized-keys commands requires an normalized binary path argument."); + "Chain invocation of ssh-authorized-keys commands requires a normalized program path (got '%s').", + argv[2]); chain_invocation = argv + 2; } else { @@ -1628,9 +1630,10 @@ static int parse_argv(int argc, char *argv[], char ***remaining_args) { arg_services = l; } - OptionParser opts = { argc, argv }; + OptionParser opts = { argc, argv, OPTION_PARSER_RETURN_POSITIONAL_ARGS }; + _cleanup_strv_free_ char **args = NULL; - FOREACH_OPTION_OR_RETURN(c, &opts) + FOREACH_OPTION_OR_RETURN(c, &opts) { switch (c) { OPTION_COMMON_HELP: @@ -1733,6 +1736,12 @@ static int parse_argv(int argc, char *argv[], char ***remaining_args) { arg_chain = true; break; + OPTION_POSITIONAL: + r = strv_extend(&args, opts.arg); + if (r < 0) + return log_oom(); + break; + OPTION_LONG("uid-min", "ID", "Filter by minimum UID/GID (default 0)"): r = parse_uid(opts.arg, &arg_uid_min); if (r < 0) @@ -1811,6 +1820,15 @@ static int parse_argv(int argc, char *argv[], char ***remaining_args) { } } + /* When --chain was seen, stop parsing switches after the second positional argument: + * [OPTS0…, VERB, OPTS1…, USERNAME, OPTS2…, COMMAND, OPTS3…] + * We shall parse OPTS0, OPTS1, OPTS2, but OPTS3 are for COMMAND. + * --chain can be anywhere in OPTS0, OPTS1, OPTS2, or first in OPTS3. + */ + if (arg_chain && strv_length(args) >= 3) + opts.state = OPTION_PARSER_DONE; + } + if (arg_uid_min > arg_uid_max) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Minimum UID/GID " UID_FMT " is above maximum UID/GID " UID_FMT ", refusing.", @@ -1823,12 +1841,16 @@ static int parse_argv(int argc, char *argv[], char ***remaining_args) { if (arg_from_file) arg_boundaries = false; - *remaining_args = option_parser_get_args(&opts); + /* We gathered some positional args in 'args' ourselves. Append the remaining ones. */ + if (strv_extend_strv(&args, option_parser_get_args(&opts), /* filter_duplicates= */ false) < 0) + return log_oom(); + + *remaining_args = TAKE_PTR(args); return 1; } static int run(int argc, char *argv[]) { - char **args = NULL; + _cleanup_strv_free_ char **args = NULL; int r; log_setup(); diff --git a/test/units/TEST-46-HOMED.sh b/test/units/TEST-46-HOMED.sh index 4b81799ef3..5afa42d739 100755 --- a/test/units/TEST-46-HOMED.sh +++ b/test/units/TEST-46-HOMED.sh @@ -586,6 +586,15 @@ EOF (! userdbctl ssh-authorized-keys dropin-user --chain '') (! SYSTEMD_LOG_LEVEL=debug userdbctl ssh-authorized-keys dropin-user --chain /usr/bin/false) + # Check that invocations with --chain work as expected + userdbctl ssh-authorized-keys --chain dropin-user /bin/echo --asdf | grep -e --asdf + userdbctl ssh-authorized-keys dropin-user --chain /bin/echo --asdf | grep -e --asdf + userdbctl ssh-authorized-keys dropin-user /bin/echo --chain --asdf | grep -e --asdf + userdbctl ssh-authorized-keys --chain dropin-user -- /bin/echo --asdf | grep -e --asdf + userdbctl ssh-authorized-keys --chain -- dropin-user /bin/echo --asdf | grep -e --asdf + userdbctl --chain -- ssh-authorized-keys dropin-user /bin/echo --asdf | grep -e --asdf + (! userdbctl --chain -- ssh-authorized-keys dropin-user -- /bin/echo --asdf) + (! userdbctl '') for opt in json multiplexer output synthesize with-dropin with-nss with-varlink; do (! userdbctl "--$opt=''") From c0cda5d91c8a6e08839ee004044c0bdea2faafa7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 May 2026 17:25:41 +0200 Subject: [PATCH 7/7] userdbctl: fix erroneous errno Fixes 1604937f83d3154fb1c3b5ef053f7fccd0825ce6. --- src/userdb/userdbctl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/userdb/userdbctl.c b/src/userdb/userdbctl.c index 35a17c5021..dd6fc0f5c1 100644 --- a/src/userdb/userdbctl.c +++ b/src/userdb/userdbctl.c @@ -1866,7 +1866,8 @@ static int run(int argc, char *argv[]) { if (!e) return log_oom(); - if (setenv("SYSTEMD_ONLY_USERDB", e, true) < 0) + r = RET_NERRNO(setenv("SYSTEMD_ONLY_USERDB", e, true)); + if (r < 0) return log_error_errno(r, "Failed to set $SYSTEMD_ONLY_USERDB: %m"); log_info("Enabled services: %s", e);