From 15f624f80f27747b6bbe22bf320f045bb3fb2df3 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 17 Jan 2024 00:54:34 +0900 Subject: [PATCH 1/7] sd-network: introduce three helper functions for LinkOperationalState --- src/libsystemd/sd-network/network-util.h | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/libsystemd/sd-network/network-util.h b/src/libsystemd/sd-network/network-util.h index c47e271a76..54cb7c4913 100644 --- a/src/libsystemd/sd-network/network-util.h +++ b/src/libsystemd/sd-network/network-util.h @@ -84,3 +84,16 @@ typedef struct LinkOperationalStateRange { int parse_operational_state_range(const char *str, LinkOperationalStateRange *out); int network_link_get_operational_state(int ifindex, LinkOperationalState *ret); + +static inline bool operational_state_is_valid(LinkOperationalState s) { + return s >= 0 && s < _LINK_OPERSTATE_MAX; +} +static inline bool operational_state_range_is_valid(const LinkOperationalStateRange *range) { + return range && + operational_state_is_valid(range->min) && + operational_state_is_valid(range->max) && + range->min <= range->max; +} +static inline bool operational_state_is_in_range(LinkOperationalState s, const LinkOperationalStateRange *range) { + return range && range->min <= s && s <= range->max; +} From 2db2979505554d7554258ec0196e1a45b5ca0115 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 16 Jan 2024 23:54:39 +0900 Subject: [PATCH 2/7] sd-network: modernize parse_operational_state_range() - rename 'out' -> 'ret', - introduce LINK_OPERSTATE_RANGE_INVALID, - constify LINK_OPERSTATE_RANGE_DEFAULT, - drop spurious const specifier for allocated string, - etc,. --- src/libsystemd/sd-network/network-util.c | 61 ++++++++++++------------ src/libsystemd/sd-network/network-util.h | 15 ++++-- 2 files changed, 42 insertions(+), 34 deletions(-) diff --git a/src/libsystemd/sd-network/network-util.c b/src/libsystemd/sd-network/network-util.c index 2059567ef8..25c6e44a77 100644 --- a/src/libsystemd/sd-network/network-util.c +++ b/src/libsystemd/sd-network/network-util.c @@ -90,49 +90,48 @@ static const char *const link_online_state_table[_LINK_ONLINE_STATE_MAX] = { DEFINE_STRING_TABLE_LOOKUP(link_online_state, LinkOnlineState); -int parse_operational_state_range(const char *str, LinkOperationalStateRange *out) { - LinkOperationalStateRange range = { _LINK_OPERSTATE_INVALID, _LINK_OPERSTATE_INVALID }; - _cleanup_free_ const char *min = NULL; +int parse_operational_state_range(const char *s, LinkOperationalStateRange *ret) { + LinkOperationalStateRange range = LINK_OPERSTATE_RANGE_INVALID; + _cleanup_free_ char *buf = NULL; const char *p; - assert(str); - assert(out); + assert(s); + assert(ret); + + /* allowed formats: "min", "min:", "min:max", ":max" */ + + if (isempty(s) || streq(s, ":")) + return -EINVAL; + + p = strchr(s, ':'); + if (!p || isempty(p + 1)) + range.max = LINK_OPERSTATE_ROUTABLE; + else { + range.max = link_operstate_from_string(p + 1); + if (range.max < 0) + return -EINVAL; + } - p = strchr(str, ':'); if (p) { - min = strndup(str, p - str); + buf = strndup(s, p - s); + if (!buf) + return -ENOMEM; - if (!isempty(p + 1)) { - range.max = link_operstate_from_string(p + 1); - if (range.max < 0) - return -EINVAL; - } - } else - min = strdup(str); + s = buf; + } - if (!min) - return -ENOMEM; - - if (!isempty(min)) { - range.min = link_operstate_from_string(min); + if (isempty(s)) + range.min = LINK_OPERSTATE_MISSING; + else { + range.min = link_operstate_from_string(s); if (range.min < 0) return -EINVAL; } - /* Fail on empty strings. */ - if (range.min == _LINK_OPERSTATE_INVALID && range.max == _LINK_OPERSTATE_INVALID) + if (!operational_state_range_is_valid(&range)) return -EINVAL; - if (range.min == _LINK_OPERSTATE_INVALID) - range.min = LINK_OPERSTATE_MISSING; - if (range.max == _LINK_OPERSTATE_INVALID) - range.max = LINK_OPERSTATE_ROUTABLE; - - if (range.min > range.max) - return -EINVAL; - - *out = range; - + *ret = range; return 0; } diff --git a/src/libsystemd/sd-network/network-util.h b/src/libsystemd/sd-network/network-util.h index 54cb7c4913..6fc6015902 100644 --- a/src/libsystemd/sd-network/network-util.h +++ b/src/libsystemd/sd-network/network-util.h @@ -79,10 +79,19 @@ typedef struct LinkOperationalStateRange { LinkOperationalState max; } LinkOperationalStateRange; -#define LINK_OPERSTATE_RANGE_DEFAULT (LinkOperationalStateRange) { LINK_OPERSTATE_DEGRADED, \ - LINK_OPERSTATE_ROUTABLE } +#define LINK_OPERSTATE_RANGE_DEFAULT \ + (const LinkOperationalStateRange) { \ + .min = LINK_OPERSTATE_DEGRADED, \ + .max = LINK_OPERSTATE_ROUTABLE, \ + } -int parse_operational_state_range(const char *str, LinkOperationalStateRange *out); +#define LINK_OPERSTATE_RANGE_INVALID \ + (const LinkOperationalStateRange) { \ + .min = _LINK_OPERSTATE_INVALID, \ + .max = _LINK_OPERSTATE_INVALID, \ + } + +int parse_operational_state_range(const char *s, LinkOperationalStateRange *ret); int network_link_get_operational_state(int ifindex, LinkOperationalState *ret); static inline bool operational_state_is_valid(LinkOperationalState s) { From ba04f957fe1161a9b2c13ea9771bdb2e5e74fcee Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 17 Jan 2024 00:00:45 +0900 Subject: [PATCH 3/7] network: drop unnecessary temporary variables --- src/network/networkd-network.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/network/networkd-network.c b/src/network/networkd-network.c index 2d5c847a6a..845a8130bd 100644 --- a/src/network/networkd-network.c +++ b/src/network/networkd-network.c @@ -1213,8 +1213,6 @@ int config_parse_required_for_online( void *userdata) { Network *network = ASSERT_PTR(userdata); - LinkOperationalStateRange range; - bool required = true; int r; assert(filename); @@ -1227,7 +1225,7 @@ int config_parse_required_for_online( return 0; } - r = parse_operational_state_range(rvalue, &range); + r = parse_operational_state_range(rvalue, &network->required_operstate_for_online); if (r < 0) { r = parse_boolean(rvalue); if (r < 0) { @@ -1237,13 +1235,12 @@ int config_parse_required_for_online( return 0; } - required = r; - range = LINK_OPERSTATE_RANGE_DEFAULT; + network->required_for_online = r; + network->required_operstate_for_online = LINK_OPERSTATE_RANGE_DEFAULT; + return 0; } - network->required_for_online = required; - network->required_operstate_for_online = range; - + network->required_for_online = true; return 0; } From 2278d9f66ead0b68acc75c09d27a7761bdee5fb3 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 17 Jan 2024 01:01:32 +0900 Subject: [PATCH 4/7] network: several cleanups for LinkOperationalState - introduce link_required_operstate_for_online() helper function, - use recently introduced macros and helper functions, - unconditionally serialize the minimum and maximum of required operational state. --- src/network/networkd-link.c | 22 ++++++++++++++++++---- src/network/networkd-link.h | 2 ++ src/network/networkd-state-file.c | 10 +++++----- 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 9e69624fd2..d553a716fa 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -71,6 +71,16 @@ #include "udev-util.h" #include "vrf.h" +void link_required_operstate_for_online(Link *link, LinkOperationalStateRange *ret) { + assert(link); + assert(ret); + + if (link->network && operational_state_range_is_valid(&link->network->required_operstate_for_online)) + *ret = link->network->required_operstate_for_online; + else + *ret = LINK_OPERSTATE_RANGE_DEFAULT; +} + bool link_ipv6_enabled(Link *link) { assert(link); @@ -1845,12 +1855,16 @@ void link_update_operstate(Link *link, bool also_update_master) { else operstate = LINK_OPERSTATE_ENSLAVED; + LinkOperationalStateRange req; + link_required_operstate_for_online(link, &req); + /* Only determine online state for managed links with RequiredForOnline=yes */ if (!link->network || !link->network->required_for_online) online_state = _LINK_ONLINE_STATE_INVALID; - else if (operstate < link->network->required_operstate_for_online.min || - operstate > link->network->required_operstate_for_online.max) + + else if (!operational_state_is_in_range(operstate, &req)) online_state = LINK_ONLINE_STATE_OFFLINE; + else { AddressFamily required_family = link->network->required_family_for_online; bool needs_ipv4 = required_family & ADDRESS_FAMILY_IPV4; @@ -1861,14 +1875,14 @@ void link_update_operstate(Link *link, bool also_update_master) { * to offline in the blocks below. */ online_state = LINK_ONLINE_STATE_ONLINE; - if (link->network->required_operstate_for_online.min >= LINK_OPERSTATE_DEGRADED) { + if (req.min >= LINK_OPERSTATE_DEGRADED) { if (needs_ipv4 && ipv4_address_state < LINK_ADDRESS_STATE_DEGRADED) online_state = LINK_ONLINE_STATE_OFFLINE; if (needs_ipv6 && ipv6_address_state < LINK_ADDRESS_STATE_DEGRADED) online_state = LINK_ONLINE_STATE_OFFLINE; } - if (link->network->required_operstate_for_online.min >= LINK_OPERSTATE_ROUTABLE) { + if (req.min >= LINK_OPERSTATE_ROUTABLE) { if (needs_ipv4 && ipv4_address_state < LINK_ADDRESS_STATE_ROUTABLE) online_state = LINK_ONLINE_STATE_OFFLINE; if (needs_ipv6 && ipv6_address_state < LINK_ADDRESS_STATE_ROUTABLE) diff --git a/src/network/networkd-link.h b/src/network/networkd-link.h index 05b028dff3..b5b1995361 100644 --- a/src/network/networkd-link.h +++ b/src/network/networkd-link.h @@ -259,3 +259,5 @@ int manager_rtnl_process_link(sd_netlink *rtnl, sd_netlink_message *message, Man int link_flags_to_string_alloc(uint32_t flags, char **ret); const char *kernel_operstate_to_string(int t) _const_; + +void link_required_operstate_for_online(Link *link, LinkOperationalStateRange *ret); diff --git a/src/network/networkd-state-file.c b/src/network/networkd-state-file.c index 9f0e365c22..b79b898832 100644 --- a/src/network/networkd-state-file.c +++ b/src/network/networkd-state-file.c @@ -630,11 +630,11 @@ static int link_save(Link *link) { fprintf(f, "REQUIRED_FOR_ONLINE=%s\n", yes_no(link->network->required_for_online)); - LinkOperationalStateRange st = link->network->required_operstate_for_online; - fprintf(f, "REQUIRED_OPER_STATE_FOR_ONLINE=%s%s%s\n", - strempty(link_operstate_to_string(st.min)), - st.max != LINK_OPERSTATE_RANGE_DEFAULT.max ? ":" : "", - st.max != LINK_OPERSTATE_RANGE_DEFAULT.max ? strempty(link_operstate_to_string(st.max)) : ""); + LinkOperationalStateRange st; + link_required_operstate_for_online(link, &st); + + fprintf(f, "REQUIRED_OPER_STATE_FOR_ONLINE=%s:%s\n", + link_operstate_to_string(st.min), link_operstate_to_string(st.max)); fprintf(f, "REQUIRED_FAMILY_FOR_ONLINE=%s\n", link_required_address_family_to_string(link->network->required_family_for_online)); From 2e59ba4e24f29901789b7845cc0087f1414eb9d9 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 17 Jan 2024 01:04:18 +0900 Subject: [PATCH 5/7] wait-online: several cleanups for LinkOperationalState - fix memleak in parser, - fix missing return in parser on failure, - drop unnecessary temporary argument in command line argument parser, - use recently introduced macros and helper functions. --- src/network/wait-online/manager.c | 26 +++++++++++--------------- src/network/wait-online/wait-online.c | 26 ++++++++++---------------- 2 files changed, 21 insertions(+), 31 deletions(-) diff --git a/src/network/wait-online/manager.c b/src/network/wait-online/manager.c index 9213795f54..da5eed5d73 100644 --- a/src/network/wait-online/manager.c +++ b/src/network/wait-online/manager.c @@ -52,7 +52,7 @@ static bool manager_ignore_link(Manager *m, Link *link) { return false; } -static int manager_link_is_online(Manager *m, Link *l, LinkOperationalStateRange s) { +static int manager_link_is_online(Manager *m, Link *l, const LinkOperationalStateRange *state_range) { AddressFamily required_family; bool needs_ipv4; bool needs_ipv6; @@ -91,25 +91,23 @@ static int manager_link_is_online(Manager *m, Link *l, LinkOperationalStateRange "link is being processed by networkd: setup state is %s.", l->state); - if (s.min < 0) - s.min = m->required_operstate.min >= 0 ? m->required_operstate.min - : l->required_operstate.min; + const LinkOperationalStateRange *range; + FOREACH_POINTER(range, state_range, &m->required_operstate, &l->required_operstate) + if (operational_state_range_is_valid(range)) + break; + assert(range != POINTER_MAX); - if (s.max < 0) - s.max = m->required_operstate.max >= 0 ? m->required_operstate.max - : l->required_operstate.max; - - if (l->operational_state < s.min || l->operational_state > s.max) + if (!operational_state_is_in_range(l->operational_state, range)) return log_link_debug_errno(l, SYNTHETIC_ERRNO(EADDRNOTAVAIL), "Operational state '%s' is not in range ['%s':'%s']", link_operstate_to_string(l->operational_state), - link_operstate_to_string(s.min), link_operstate_to_string(s.max)); + link_operstate_to_string(range->min), link_operstate_to_string(range->max)); required_family = m->required_family > 0 ? m->required_family : l->required_family; needs_ipv4 = required_family & ADDRESS_FAMILY_IPV4; needs_ipv6 = required_family & ADDRESS_FAMILY_IPV6; - if (s.min < LINK_OPERSTATE_ROUTABLE) { + if (range->min < LINK_OPERSTATE_ROUTABLE) { if (needs_ipv4 && l->ipv4_address_state < LINK_ADDRESS_STATE_DEGRADED) return log_link_debug_errno(l, SYNTHETIC_ERRNO(EADDRNOTAVAIL), "No routable or link-local IPv4 address is configured."); @@ -155,7 +153,7 @@ bool manager_configured(Manager *m) { continue; } - r = manager_link_is_online(m, l, *range); + r = manager_link_is_online(m, l, range); if (r <= 0 && !m->any) return false; if (r > 0 && m->any) @@ -175,9 +173,7 @@ bool manager_configured(Manager *m) { continue; } - r = manager_link_is_online(m, l, - (LinkOperationalStateRange) { _LINK_OPERSTATE_INVALID, - _LINK_OPERSTATE_INVALID }); + r = manager_link_is_online(m, l, /* state_range = */ NULL); if (r < 0 && !m->any) /* Unlike the above loop, unmanaged interfaces are ignored here. */ return false; if (r > 0) { diff --git a/src/network/wait-online/wait-online.c b/src/network/wait-online/wait-online.c index 5328bba2d8..544c360edd 100644 --- a/src/network/wait-online/wait-online.c +++ b/src/network/wait-online/wait-online.c @@ -19,7 +19,7 @@ static bool arg_quiet = false; static usec_t arg_timeout = 120 * USEC_PER_SEC; static Hashmap *arg_interfaces = NULL; static char **arg_ignore = NULL; -static LinkOperationalStateRange arg_required_operstate = { _LINK_OPERSTATE_INVALID, _LINK_OPERSTATE_INVALID }; +static LinkOperationalStateRange arg_required_operstate = LINK_OPERSTATE_RANGE_INVALID; static AddressFamily arg_required_family = ADDRESS_FAMILY_NO; static bool arg_any = false; @@ -71,12 +71,11 @@ static int parse_interface_with_operstate_range(const char *str) { if (p) { r = parse_operational_state_range(p + 1, range); if (r < 0) - log_error_errno(r, "Invalid operational state range '%s'", p + 1); + return log_error_errno(r, "Invalid operational state range: %s", p + 1); ifname = strndup(optarg, p - optarg); } else { - range->min = _LINK_OPERSTATE_INVALID; - range->max = _LINK_OPERSTATE_INVALID; + *range = LINK_OPERSTATE_RANGE_INVALID; ifname = strdup(str); } if (!ifname) @@ -84,18 +83,19 @@ static int parse_interface_with_operstate_range(const char *str) { if (!ifname_valid(ifname)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Invalid interface name '%s'", ifname); + "Invalid interface name: %s", ifname); - r = hashmap_ensure_put(&arg_interfaces, &string_hash_ops, ifname, TAKE_PTR(range)); + r = hashmap_ensure_put(&arg_interfaces, &string_hash_ops, ifname, range); if (r == -ENOMEM) return log_oom(); if (r < 0) return log_error_errno(r, "Failed to store interface name: %m"); if (r == 0) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Interface name %s is already specified", ifname); + return log_error_errno(SYNTHETIC_ERRNO(EEXIST), + "Interface name %s is already specified.", ifname); TAKE_PTR(ifname); + TAKE_PTR(range); return 0; } @@ -154,17 +154,11 @@ static int parse_argv(int argc, char *argv[]) { break; - case 'o': { - LinkOperationalStateRange range; - - r = parse_operational_state_range(optarg, &range); + case 'o': + r = parse_operational_state_range(optarg, &arg_required_operstate); if (r < 0) return log_error_errno(r, "Invalid operational state range '%s'", optarg); - - arg_required_operstate = range; - break; - } case '4': arg_required_family |= ADDRESS_FAMILY_IPV4; From 3255bda698d2a02ab2f2825a1e652ac6f0871a89 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 17 Jan 2024 01:35:34 +0900 Subject: [PATCH 6/7] network: make 'carrier' as the default required operational state for CAN device As CAN devices do not support IP address, hence the state never goes to higher than 'carrier'. Prompted by https://github.com/linux-can/can-utils/issues/68#issuecomment-1327987724. --- man/systemd.network.xml | 11 +++++++++-- src/network/networkd-link.c | 5 +++++ src/network/networkd-network.c | 4 ++-- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/man/systemd.network.xml b/man/systemd.network.xml index 76f9f4d042..35c897af39 100644 --- a/man/systemd.network.xml +++ b/man/systemd.network.xml @@ -259,8 +259,9 @@ RequiredForOnline= - Takes a boolean or a minimum operational state and an optional maximum operational - state. Please see + Takes a boolean, a minimum operational state (e.g., carrier), or a range + of operational state separated with a colon (e.g., degraded:routable). + Please see networkctl1 for possible operational states. When yes, the network is deemed required when determining whether the system is online (including when running @@ -270,6 +271,12 @@ minimum and maximum operational state required for the network interface to be considered online. + When yes is specified for a CAN device, + systemd-networkd-wait-online deems that the interface is online when its + operational state becomes carrier. For an interface with other type, e.g. + ether, the interface is deened online when its online state is + degraded or routable. + Defaults to yes when ActivationPolicy= is not set, or set to up, always-up, or bound. Defaults to no when diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index d553a716fa..57ea9ecb75 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -77,6 +77,11 @@ void link_required_operstate_for_online(Link *link, LinkOperationalStateRange *r if (link->network && operational_state_range_is_valid(&link->network->required_operstate_for_online)) *ret = link->network->required_operstate_for_online; + else if (link->iftype == ARPHRD_CAN) + *ret = (const LinkOperationalStateRange) { + .min = LINK_OPERSTATE_CARRIER, + .max = LINK_OPERSTATE_CARRIER, + }; else *ret = LINK_OPERSTATE_RANGE_DEFAULT; } diff --git a/src/network/networkd-network.c b/src/network/networkd-network.c index 845a8130bd..08c7da5699 100644 --- a/src/network/networkd-network.c +++ b/src/network/networkd-network.c @@ -372,7 +372,7 @@ int network_load_one(Manager *manager, OrderedHashmap **networks, const char *fi .n_ref = 1, .required_for_online = -1, - .required_operstate_for_online = LINK_OPERSTATE_RANGE_DEFAULT, + .required_operstate_for_online = LINK_OPERSTATE_RANGE_INVALID, .activation_policy = _ACTIVATION_POLICY_INVALID, .group = -1, .arp = -1, @@ -1221,7 +1221,7 @@ int config_parse_required_for_online( if (isempty(rvalue)) { network->required_for_online = -1; - network->required_operstate_for_online = LINK_OPERSTATE_RANGE_DEFAULT; + network->required_operstate_for_online = LINK_OPERSTATE_RANGE_INVALID; return 0; } From 7155ad95324cde9c9c9d3309238a09d4df08dc35 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 17 Jan 2024 01:38:55 +0900 Subject: [PATCH 7/7] test-network: test the default required operational state for CAN devices --- test/test-network/systemd-networkd-tests.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py index 6d074e67ff..7bf2f50145 100755 --- a/test/test-network/systemd-networkd-tests.py +++ b/test/test-network/systemd-networkd-tests.py @@ -1772,6 +1772,8 @@ class NetworkdNetDevTests(unittest.TestCase, Utilities): start_networkd() self.wait_online(['vcan99:carrier', 'vcan98:carrier']) + # For can devices, 'carrier' is the default required operational state. + self.wait_online(['vcan99', 'vcan98']) # https://github.com/systemd/systemd/issues/30140 output = check_output('ip -d link show vcan99') @@ -1788,6 +1790,8 @@ class NetworkdNetDevTests(unittest.TestCase, Utilities): start_networkd() self.wait_online(['vxcan99:carrier', 'vxcan-peer:carrier']) + # For can devices, 'carrier' is the default required operational state. + self.wait_online(['vxcan99', 'vxcan-peer']) @expectedFailureIfModuleIsNotAvailable('wireguard') def test_wireguard(self):