Merge pull request #17341 from yuwata/sd-dhcp-client-fix-eexist-issue-16964

sd-dhcp: make sd_dhcp_client_set_request_option() not return -EEXIST

TODO

@@ -29,10 +29,6 @@ Features:
 * Add service setting to run a service within the specified VRF. i.e. do the
   equivalent of "ip vrf exec".
 
-* systemd-analyze syscall-filter should show a list of syscalls listed in
-  @known but not in other groups (at least at debug level), since they are
-  candidates to be added to them.
-
 * export action of device object on sd-device, so that monitor becomes useful
 
 * add root=tmpfs that mounts a tmpfs to /sysroot (to be used in combination

src/analyze/analyze.c

@@ -1685,7 +1685,7 @@ static int load_kernel_syscalls(Set **ret) {
         return 0;
 }
 
-static void kernel_syscalls_remove(Set *s, const SyscallFilterSet *set) {
+static void syscall_set_remove(Set *s, const SyscallFilterSet *set) {
         const char *syscall;
 
         NULSTR_FOREACH(syscall, set->value) {
@@ -1716,9 +1716,14 @@ static int dump_syscall_filters(int argc, char *argv[], void *userdata) {
         (void) pager_open(arg_pager_flags);
 
         if (strv_isempty(strv_skip(argv, 1))) {
-                _cleanup_set_free_ Set *kernel = NULL;
+                _cleanup_set_free_ Set *kernel = NULL, *known = NULL;
+                const char *sys;
                 int i, k;
 
+                NULSTR_FOREACH(sys, syscall_filter_sets[SYSCALL_FILTER_SET_KNOWN].value)
+                        if (set_put_strdup(&known, sys) < 0)
+                                return log_oom();
+
                 k = load_kernel_syscalls(&kernel);
 
                 for (i = 0; i < _SYSCALL_FILTER_SET_MAX; i++) {
@@ -1727,10 +1732,30 @@ static int dump_syscall_filters(int argc, char *argv[], void *userdata) {
                                 puts("");
 
                         dump_syscall_filter(set);
-                        kernel_syscalls_remove(kernel, set);
+                        syscall_set_remove(kernel, set);
+                        if (i != SYSCALL_FILTER_SET_KNOWN)
+                                syscall_set_remove(known, set);
                         first = false;
                 }
 
+                if (!set_isempty(known)) {
+                        _cleanup_free_ char **l = NULL;
+                        char **syscall;
+
+                        printf("\n"
+                               "# %sUngrouped System Calls%s (known but not included in any of the groups except @known):\n",
+                               ansi_highlight(), ansi_normal());
+
+                        l = set_get_strv(known);
+                        if (!l)
+                                return log_oom();
+
+                        strv_sort(l);
+
+                        STRV_FOREACH(syscall, l)
+                                printf("#   %s\n", *syscall);
+                }
+
                 if (k < 0) {
                         fputc('\n', stdout);
                         fflush(stdout);

src/basic/macro.h

@@ -444,6 +444,9 @@ static inline int __coverity_check_and_return__(int condition) {
 #define PTR_TO_ULONG(p) ((unsigned long) ((uintptr_t) (p)))
 #define ULONG_TO_PTR(u) ((void *) ((uintptr_t) (u)))
 
+#define PTR_TO_UINT8(p) ((uint8_t) ((uintptr_t) (p)))
+#define UINT8_TO_PTR(u) ((void *) ((uintptr_t) (u)))
+
 #define PTR_TO_INT32(p) ((int32_t) ((intptr_t) (p)))
 #define INT32_TO_PTR(u) ((void *) ((intptr_t) (u)))
 #define PTR_TO_UINT32(p) ((uint32_t) ((uintptr_t) (p)))

src/libsystemd-network/sd-dhcp-client.c

@@ -25,6 +25,8 @@
 #include "io-util.h"
 #include "memory-util.h"
 #include "random-util.h"
+#include "set.h"
+#include "sort-util.h"
 #include "string-util.h"
 #include "strv.h"
 #include "utf8.h"
@@ -75,9 +77,7 @@ struct sd_dhcp_client {
         union sockaddr_union link;
         sd_event_source *receive_message;
         bool request_broadcast;
-        uint8_t *req_opts;
+        Set *req_opts;
-        size_t req_opts_allocated;
-        size_t req_opts_size;
         bool anonymize;
         be32_t last_addr;
         uint8_t mac_addr[MAX_MAC_ADDR_LEN];
@@ -230,8 +230,6 @@ int sd_dhcp_client_set_request_broadcast(sd_dhcp_client *client, int broadcast)
 }
 
 int sd_dhcp_client_set_request_option(sd_dhcp_client *client, uint8_t option) {
-        size_t i;
-
         assert_return(client, -EINVAL);
         assert_return(IN_SET(client->state, DHCP_STATE_INIT, DHCP_STATE_STOPPED), -EBUSY);
 
@@ -248,17 +246,7 @@ int sd_dhcp_client_set_request_option(sd_dhcp_client *client, uint8_t option) {
                 break;
         }
 
-        for (i = 0; i < client->req_opts_size; i++)
+        return set_ensure_put(&client->req_opts, NULL, UINT8_TO_PTR(option));
-                if (client->req_opts[i] == option)
-                        return -EEXIST;
-
-        if (!GREEDY_REALLOC(client->req_opts, client->req_opts_allocated,
-                            client->req_opts_size + 1))
-                return -ENOMEM;
-
-        client->req_opts[client->req_opts_size++] = option;
-
-        return 0;
 }
 
 int sd_dhcp_client_set_request_address(
@@ -725,6 +713,10 @@ static void client_stop(sd_dhcp_client *client, int error) {
         client_initialize(client);
 }
 
+static int cmp_uint8(const uint8_t *a, const uint8_t *b) {
+        return CMP(*a, *b);
+}
+
 static int client_message_init(
                 sd_dhcp_client *client,
                 DHCPPacket **ret,
@@ -835,10 +827,26 @@ static int client_message_init(
            MAY contain the Parameter Request List option. */
         /* NOTE: in case that there would be an option to do not send
          * any PRL at all, the size should be checked before sending */
-        if (client->req_opts_size > 0 && type != DHCP_RELEASE) {
+        if (!set_isempty(client->req_opts) && type != DHCP_RELEASE) {
+                _cleanup_free_ uint8_t *opts = NULL;
+                size_t n_opts, i = 0;
+                void *val;
+
+                n_opts = set_size(client->req_opts);
+                opts = new(uint8_t, n_opts);
+                if (!opts)
+                        return -ENOMEM;
+
+                SET_FOREACH(val, client->req_opts)
+                        opts[i++] = PTR_TO_UINT8(val);
+                assert(i == n_opts);
+
+                /* For anonymizing the request, let's sort the options. */
+                typesafe_qsort(opts, n_opts, cmp_uint8);
+
                 r = dhcp_option_append(&packet->dhcp, optlen, &optoffset, 0,
                                        SD_DHCP_OPTION_PARAMETER_REQUEST_LIST,
-                                       client->req_opts_size, client->req_opts);
+                                       n_opts, opts);
                 if (r < 0)
                         return r;
         }
@@ -2187,7 +2195,7 @@ static sd_dhcp_client *dhcp_client_free(sd_dhcp_client *client) {
 
         sd_dhcp_lease_unref(client->lease);
 
-        free(client->req_opts);
+        set_free(client->req_opts);
         free(client->hostname);
         free(client->vendor_class_identifier);
         free(client->mudurl);
@@ -2200,6 +2208,10 @@ static sd_dhcp_client *dhcp_client_free(sd_dhcp_client *client) {
 DEFINE_TRIVIAL_REF_UNREF_FUNC(sd_dhcp_client, sd_dhcp_client, dhcp_client_free);
 
 int sd_dhcp_client_new(sd_dhcp_client **ret, int anonymize) {
+        const uint8_t *opts;
+        size_t n_opts;
+        int r;
+
         assert_return(ret, -EINVAL);
 
         _cleanup_(sd_dhcp_client_unrefp) sd_dhcp_client *client = new(sd_dhcp_client, 1);
@@ -2219,14 +2231,18 @@ int sd_dhcp_client_new(sd_dhcp_client **ret, int anonymize) {
         };
         /* NOTE: this could be moved to a function. */
         if (anonymize) {
-                client->req_opts_size = ELEMENTSOF(default_req_opts_anonymize);
+                n_opts = ELEMENTSOF(default_req_opts_anonymize);
-                client->req_opts = memdup(default_req_opts_anonymize, client->req_opts_size);
+                opts = default_req_opts_anonymize;
         } else {
-                client->req_opts_size = ELEMENTSOF(default_req_opts);
+                n_opts = ELEMENTSOF(default_req_opts);
-                client->req_opts = memdup(default_req_opts, client->req_opts_size);
+                opts = default_req_opts;
+        }
+
+        for (size_t i = 0; i < n_opts; i++) {
+                r = sd_dhcp_client_set_request_option(client, opts[i]);
+                if (r < 0)
+                        return r;
         }
-        if (!client->req_opts)
-                return -ENOMEM;
 
         *ret = TAKE_PTR(client);
 

src/libsystemd-network/test-dhcp-client.c

@@ -71,40 +71,29 @@ static void test_request_basic(sd_event *e) {
         assert_se(sd_dhcp_client_set_hostname(client, "~host") == -EINVAL);
         assert_se(sd_dhcp_client_set_hostname(client, "~host.domain") == -EINVAL);
 
-        assert_se(sd_dhcp_client_set_request_option(client,
+        assert_se(sd_dhcp_client_set_request_option(client, SD_DHCP_OPTION_SUBNET_MASK) == 0);
-                                        SD_DHCP_OPTION_SUBNET_MASK) == -EEXIST);
+        assert_se(sd_dhcp_client_set_request_option(client, SD_DHCP_OPTION_ROUTER) == 0);
-        assert_se(sd_dhcp_client_set_request_option(client,
-                                        SD_DHCP_OPTION_ROUTER) == -EEXIST);
         /* This PRL option is not set when using Anonymize, but in this test
          * Anonymize settings are not being used. */
-        assert_se(sd_dhcp_client_set_request_option(client,
+        assert_se(sd_dhcp_client_set_request_option(client, SD_DHCP_OPTION_HOST_NAME) == 0);
-                                        SD_DHCP_OPTION_HOST_NAME) == -EEXIST);
+        assert_se(sd_dhcp_client_set_request_option(client, SD_DHCP_OPTION_DOMAIN_NAME) == 0);
-        assert_se(sd_dhcp_client_set_request_option(client,
+        assert_se(sd_dhcp_client_set_request_option(client, SD_DHCP_OPTION_DOMAIN_NAME_SERVER) == 0);
-                                        SD_DHCP_OPTION_DOMAIN_NAME) == -EEXIST);
-        assert_se(sd_dhcp_client_set_request_option(client,
-                                        SD_DHCP_OPTION_DOMAIN_NAME_SERVER) == -EEXIST);
 
-        assert_se(sd_dhcp_client_set_request_option(client,
+        assert_se(sd_dhcp_client_set_request_option(client, SD_DHCP_OPTION_PAD) == -EINVAL);
-                                        SD_DHCP_OPTION_PAD) == -EINVAL);
+        assert_se(sd_dhcp_client_set_request_option(client, SD_DHCP_OPTION_END) == -EINVAL);
-        assert_se(sd_dhcp_client_set_request_option(client,
+        assert_se(sd_dhcp_client_set_request_option(client, SD_DHCP_OPTION_MESSAGE_TYPE) == -EINVAL);
-                                        SD_DHCP_OPTION_END) == -EINVAL);
+        assert_se(sd_dhcp_client_set_request_option(client, SD_DHCP_OPTION_OVERLOAD) == -EINVAL);
-        assert_se(sd_dhcp_client_set_request_option(client,
+        assert_se(sd_dhcp_client_set_request_option(client, SD_DHCP_OPTION_PARAMETER_REQUEST_LIST) == -EINVAL);
-                                        SD_DHCP_OPTION_MESSAGE_TYPE) == -EINVAL);
-        assert_se(sd_dhcp_client_set_request_option(client,
-                                        SD_DHCP_OPTION_OVERLOAD) == -EINVAL);
-        assert_se(sd_dhcp_client_set_request_option(client,
-                                        SD_DHCP_OPTION_PARAMETER_REQUEST_LIST)
-                        == -EINVAL);
 
         /* RFC7844: option 33 (SD_DHCP_OPTION_STATIC_ROUTE) is set in the
          * default PRL when using Anonymize, so it is changed to other option
          * that is not set by default, to check that it was set successfully.
          * Options not set by default (using or not anonymize) are option 17
          * (SD_DHCP_OPTION_ROOT_PATH) and 42 (SD_DHCP_OPTION_NTP_SERVER) */
+        assert_se(sd_dhcp_client_set_request_option(client, 17) == 1);
+        assert_se(sd_dhcp_client_set_request_option(client, 17) == 0);
+        assert_se(sd_dhcp_client_set_request_option(client, 42) == 1);
         assert_se(sd_dhcp_client_set_request_option(client, 17) == 0);
-        assert_se(sd_dhcp_client_set_request_option(client, 17) == -EEXIST);
-        assert_se(sd_dhcp_client_set_request_option(client, 42) == 0);
-        assert_se(sd_dhcp_client_set_request_option(client, 17) == -EEXIST);
 
         sd_dhcp_client_unref(client);
 }
@@ -126,19 +115,15 @@ static void test_request_anonymize(sd_event *e) {
         r = sd_dhcp_client_attach_event(client, e, 0);
         assert_se(r >= 0);
 
-        assert_se(sd_dhcp_client_set_request_option(client,
+        assert_se(sd_dhcp_client_set_request_option(client, SD_DHCP_OPTION_NETBIOS_NAMESERVER) == 0);
-                                        SD_DHCP_OPTION_NETBIOS_NAMESERVER) == -EEXIST);
         /* This PRL option is not set when using Anonymize */
-        assert_se(sd_dhcp_client_set_request_option(client,
+        assert_se(sd_dhcp_client_set_request_option(client, SD_DHCP_OPTION_HOST_NAME) == 1);
-                                        SD_DHCP_OPTION_HOST_NAME) == 0);
+        assert_se(sd_dhcp_client_set_request_option(client, SD_DHCP_OPTION_PARAMETER_REQUEST_LIST) == -EINVAL);
-        assert_se(sd_dhcp_client_set_request_option(client,
-                                        SD_DHCP_OPTION_PARAMETER_REQUEST_LIST)
-                        == -EINVAL);
 
         /* RFC7844: option 101 (SD_DHCP_OPTION_NEW_TZDB_TIMEZONE) is not set in the
          * default PRL when using Anonymize, */
+        assert_se(sd_dhcp_client_set_request_option(client, 101) == 1);
         assert_se(sd_dhcp_client_set_request_option(client, 101) == 0);
-        assert_se(sd_dhcp_client_set_request_option(client, 101) == -EEXIST);
 
         sd_dhcp_client_unref(client);
 }

src/network/networkd-dhcp4.c

@@ -1357,10 +1357,6 @@ int dhcp4_configure(Link *link) {
                 uint32_t option = PTR_TO_UINT32(request_options);
 
                 r = sd_dhcp_client_set_request_option(link->dhcp_client, option);
-                if (r == -EEXIST) {
-                        log_link_debug(link, "DHCP4 CLIENT: Failed to set request flag for '%u' already exists, ignoring.", option);
-                        continue;
-                }
                 if (r < 0)
                         return log_link_error_errno(link, r, "DHCP4 CLIENT: Failed to set request flag for '%u': %m", option);
         }

src/network/networkd-link.c

@@ -1758,6 +1758,11 @@ static int link_joined(Link *link) {
                 return 0;
 
         link_set_state(link, LINK_STATE_CONFIGURING);
+
+        r = link_acquire_conf(link);
+        if (r < 0)
+                return r;
+
         return link_set_static_configs(link);
 }
 
@@ -2038,12 +2043,6 @@ static int link_configure_continue(Link *link) {
         if (r < 0)
                 log_link_warning_errno(link, r, "Cannot set IPv6 MTU for interface, ignoring: %m");
 
-        if (link_has_carrier(link) || link->network->configure_without_carrier) {
-                r = link_acquire_conf(link);
-                if (r < 0)
-                        return r;
-        }
-
         return link_enter_join_netdev(link);
 }