From 851cdffd1bfe7a1f2d3dfc0d343003aab6b44620 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 16 Sep 2023 12:06:19 +0200 Subject: [PATCH] network: use a common helper to parse bounded ranges This compresses repetetive code and makes it easier to add new options in networkd. The formatting of error messages becomes uniform. The error message always specifies the rvalue literally, instead of using a "descriptive name". This makes the message much easier to handle for the user. I opted to add just one parser, and wrap it with inline functions to proxy the type. This is less verbose than copying functions for each type separately, and the compiler should be able to get rid of the inline wrapper almost entirely. asserts are reordered to use the same order as the parameter list. This makes the code easier to read. No functional change intended, apart from the difference in error message formatting. --- src/network/netdev/bond.c | 47 +++----------- src/network/netdev/bridge.c | 53 ++++------------ src/network/netdev/geneve.c | 51 ++++------------ src/network/netdev/l2tp-tunnel.c | 59 ++++++------------ src/network/netdev/macvlan.c | 25 ++------ src/network/netdev/tunnel.c | 102 ++++++++++--------------------- src/shared/conf-parser.c | 38 ++++++++++++ src/shared/conf-parser.h | 94 ++++++++++++++++++++++++++++ 8 files changed, 220 insertions(+), 249 deletions(-) diff --git a/src/network/netdev/bond.c b/src/network/netdev/bond.c index 601bff0a9c..704a4c6b4c 100644 --- a/src/network/netdev/bond.c +++ b/src/network/netdev/bond.c @@ -303,32 +303,18 @@ int config_parse_ad_actor_sys_prio( const char *rvalue, void *data, void *userdata) { - Bond *b = userdata; - uint16_t v; - int r; assert(filename); assert(lvalue); assert(rvalue); assert(data); - r = safe_atou16(rvalue, &v); - if (r < 0) { - log_syntax(unit, LOG_WARNING, filename, line, r, - "Failed to parse actor system priority '%s', ignoring: %m", rvalue); - return 0; - } + Bond *b = ASSERT_PTR(userdata); - if (v == 0) { - log_syntax(unit, LOG_WARNING, filename, line, 0, - "Failed to parse actor system priority '%s'. Range is [1,65535], ignoring.", - rvalue); - return 0; - } - - b->ad_actor_sys_prio = v; - - return 0; + return config_parse_uint16_bounded( + unit, filename, line, section, section_line, lvalue, rvalue, + 1, UINT16_MAX, true, + &b->ad_actor_sys_prio); } int config_parse_ad_user_port_key( @@ -342,31 +328,18 @@ int config_parse_ad_user_port_key( const char *rvalue, void *data, void *userdata) { - Bond *b = userdata; - uint16_t v; - int r; assert(filename); assert(lvalue); assert(rvalue); assert(data); - r = safe_atou16(rvalue, &v); - if (r < 0) { - log_syntax(unit, LOG_WARNING, filename, line, r, - "Failed to parse user port key '%s', ignoring: %m", rvalue); - return 0; - } + Bond *b = ASSERT_PTR(userdata); - if (v > 1023) { - log_syntax(unit, LOG_WARNING, filename, line, 0, - "Failed to parse user port key '%s'. Range is [0…1023], ignoring.", rvalue); - return 0; - } - - b->ad_user_port_key = v; - - return 0; + return config_parse_uint16_bounded( + unit, filename, line, section, section_line, lvalue, rvalue, + 0, 1023, /* ignoring= */ true, + &b->ad_user_port_key); } int config_parse_ad_actor_system( diff --git a/src/network/netdev/bridge.c b/src/network/netdev/bridge.c index b65c3b49fc..3783139d15 100644 --- a/src/network/netdev/bridge.c +++ b/src/network/netdev/bridge.c @@ -189,36 +189,22 @@ int config_parse_bridge_igmp_version( void *data, void *userdata) { - Bridge *b = userdata; - uint8_t u; - int r; - assert(filename); assert(lvalue); assert(rvalue); assert(data); + Bridge *b = ASSERT_PTR(userdata); + if (isempty(rvalue)) { b->igmp_version = 0; /* 0 means unset. */ return 0; } - r = safe_atou8(rvalue, &u); - if (r < 0) { - log_syntax(unit, LOG_WARNING, filename, line, r, - "Failed to parse bridge's multicast IGMP version number '%s', ignoring assignment: %m", - rvalue); - return 0; - } - if (!IN_SET(u, 2, 3)) { - log_syntax(unit, LOG_WARNING, filename, line, 0, - "Invalid bridge's multicast IGMP version number '%s', ignoring assignment.", rvalue); - return 0; - } - - b->igmp_version = u; - - return 0; + return config_parse_uint8_bounded( + unit, filename, line, section, section_line, lvalue, rvalue, + 2, 3, true, + &b->igmp_version); } int config_parse_bridge_port_priority( @@ -233,33 +219,16 @@ int config_parse_bridge_port_priority( void *data, void *userdata) { - uint16_t i; - int r; - assert(filename); assert(lvalue); assert(rvalue); - assert(data); - /* This is used in networkd-network-gperf.gperf. */ + uint16_t *prio = ASSERT_PTR(data); - r = safe_atou16(rvalue, &i); - if (r < 0) { - log_syntax(unit, LOG_WARNING, filename, line, r, - "Failed to parse bridge port priority, ignoring: %s", rvalue); - return 0; - } - - if (i > LINK_BRIDGE_PORT_PRIORITY_MAX) { - log_syntax(unit, LOG_WARNING, filename, line, 0, - "Bridge port priority is larger than maximum %u, ignoring: %s", - LINK_BRIDGE_PORT_PRIORITY_MAX, rvalue); - return 0; - } - - *((uint16_t *)data) = i; - - return 0; + return config_parse_uint16_bounded( + unit, filename, line, section, section_line, lvalue, rvalue, + 0, LINK_BRIDGE_PORT_PRIORITY_MAX, true, + prio); } static void bridge_init(NetDev *n) { diff --git a/src/network/netdev/geneve.c b/src/network/netdev/geneve.c index 7d5a8679d1..83c73aff07 100644 --- a/src/network/netdev/geneve.c +++ b/src/network/netdev/geneve.c @@ -116,29 +116,17 @@ int config_parse_geneve_vni( void *data, void *userdata) { - Geneve *v = userdata; - uint32_t f; - int r; - assert(filename); assert(lvalue); assert(rvalue); assert(data); - r = safe_atou32(rvalue, &f); - if (r < 0) { - log_syntax(unit, LOG_WARNING, filename, line, r, "Failed to parse Geneve VNI '%s'.", rvalue); - return 0; - } + Geneve *v = ASSERT_PTR(userdata); - if (f > GENEVE_VID_MAX){ - log_syntax(unit, LOG_WARNING, filename, line, 0, "Geneve VNI out is of range '%s'.", rvalue); - return 0; - } - - v->id = f; - - return 0; + return config_parse_uint32_bounded( + unit, filename, line, section, section_line, lvalue, rvalue, + 0, GENEVE_VID_MAX, true, + &v->id); } int config_parse_geneve_address( @@ -230,35 +218,22 @@ int config_parse_geneve_ttl( void *data, void *userdata) { - Geneve *v = userdata; - unsigned f; - int r; - assert(filename); assert(lvalue); assert(rvalue); assert(data); - if (streq(rvalue, "inherit")) + Geneve *v = ASSERT_PTR(userdata); + + if (streq(rvalue, "inherit")) { v->inherit = true; - else { - r = safe_atou(rvalue, &f); - if (r < 0) { - log_syntax(unit, LOG_WARNING, filename, line, r, - "Failed to parse Geneve TTL '%s', ignoring assignment: %m", rvalue); - return 0; - } - - if (f > 255) { - log_syntax(unit, LOG_WARNING, filename, line, 0, - "Invalid Geneve TTL '%s'. TTL must be <= 255. Ignoring assignment.", rvalue); - return 0; - } - - v->ttl = f; + return 0; } - return 0; + return config_parse_uint8_bounded( + unit, filename, line, section, section_line, lvalue, rvalue, + 0, UINT8_MAX, true, + &v->ttl); } static int netdev_geneve_verify(NetDev *netdev, const char *filename) { diff --git a/src/network/netdev/l2tp-tunnel.c b/src/network/netdev/l2tp-tunnel.c index 547de2418c..17d5391cd3 100644 --- a/src/network/netdev/l2tp-tunnel.c +++ b/src/network/netdev/l2tp-tunnel.c @@ -626,30 +626,16 @@ int config_parse_l2tp_tunnel_id( void *data, void *userdata) { - uint32_t *id = data, k; - int r; - assert(filename); assert(lvalue); assert(rvalue); - assert(data); - r = safe_atou32(rvalue, &k); - if (r < 0) { - log_syntax(unit, LOG_WARNING, filename, line, r, - "Failed to parse L2TP tunnel id. Ignoring assignment: %s", rvalue); - return 0; - } + uint32_t *id = ASSERT_PTR(data); - if (k == 0) { - log_syntax(unit, LOG_WARNING, filename, line, 0, - "Invalid L2TP tunnel id. Ignoring assignment: %s", rvalue); - return 0; - } - - *id = k; - - return 0; + return config_parse_uint32_bounded( + unit, filename, line, section, section_line, lvalue, rvalue, + 1, UINT32_MAX, true, + id); } int config_parse_l2tp_session_id( @@ -664,40 +650,29 @@ int config_parse_l2tp_session_id( void *data, void *userdata) { - _cleanup_(l2tp_session_free_or_set_invalidp) L2tpSession *session = NULL; - L2tpTunnel *t = userdata; - uint32_t k; - int r; - assert(filename); assert(section); assert(lvalue); assert(rvalue); assert(data); + L2tpTunnel *t = ASSERT_PTR(userdata); + _cleanup_(l2tp_session_free_or_set_invalidp) L2tpSession *session = NULL; + int r; + r = l2tp_session_new_static(t, filename, section_line, &session); if (r < 0) return log_oom(); - r = safe_atou32(rvalue, &k); - if (r < 0) { - log_syntax(unit, LOG_WARNING, filename, line, r, - "Failed to parse L2TP session id. Ignoring assignment: %s", rvalue); - return 0; - } + uint32_t *id = streq(lvalue, "SessionId") ? &session->session_id : &session->peer_session_id; - if (k == 0) { - log_syntax(unit, LOG_WARNING, filename, line, 0, - "Invalid L2TP session id. Ignoring assignment: %s", rvalue); - return 0; - } - - if (streq(lvalue, "SessionId")) - session->session_id = k; - else - session->peer_session_id = k; - - session = NULL; + r = config_parse_uint32_bounded( + unit, filename, line, section, section_line, lvalue, rvalue, + 1, UINT32_MAX, true, + id); + if (r <= 0) + return r; + TAKE_PTR(session); return 0; } diff --git a/src/network/netdev/macvlan.c b/src/network/netdev/macvlan.c index 1114bb0cb1..6083cd2b44 100644 --- a/src/network/netdev/macvlan.c +++ b/src/network/netdev/macvlan.c @@ -84,36 +84,23 @@ int config_parse_macvlan_broadcast_queue_size( void *data, void *userdata) { - MacVlan *m = ASSERT_PTR(userdata); - uint32_t v; - int r; - assert(filename); assert(section); assert(lvalue); assert(rvalue); assert(data); + MacVlan *m = ASSERT_PTR(userdata); + if (isempty(rvalue)) { m->bc_queue_length = UINT32_MAX; return 0; } - r = safe_atou32(rvalue, &v); - if (r < 0) { - log_syntax(unit, LOG_WARNING, filename, line, r, - "Failed to parse BroadcastMulticastQueueLength=%s, ignoring assignment: %m", rvalue); - return 0; - } - - if (v == UINT32_MAX) { - log_syntax(unit, LOG_WARNING, filename, line, 0, - "Invalid BroadcastMulticastQueueLength=%s, ignoring assignment: %m", rvalue); - return 0; - } - - m->bc_queue_length = v; - return 0; + return config_parse_uint32_bounded( + unit, filename, line, section, section_line, lvalue, rvalue, + 0, UINT32_MAX - 1, true, + &m->bc_queue_length); } static void macvlan_done(NetDev *n) { diff --git a/src/network/netdev/tunnel.c b/src/network/netdev/tunnel.c index 881be3c405..ced04c77ec 100644 --- a/src/network/netdev/tunnel.c +++ b/src/network/netdev/tunnel.c @@ -972,33 +972,26 @@ int config_parse_encap_limit( void *data, void *userdata) { - Tunnel *t = ASSERT_PTR(userdata); - int k, r; - assert(filename); assert(rvalue); + Tunnel *t = ASSERT_PTR(userdata); + int r; + if (streq(rvalue, "none")) { - t->flags |= IP6_TNL_F_IGN_ENCAP_LIMIT; t->encap_limit = 0; + t->flags |= IP6_TNL_F_IGN_ENCAP_LIMIT; return 0; } - r = safe_atoi(rvalue, &k); - if (r < 0) { - log_syntax(unit, LOG_WARNING, filename, line, r, - "Failed to parse Tunnel Encapsulation Limit option, ignoring assignment: %s", rvalue); - return 0; - } - - if (k > 255 || k < 0) { - log_syntax(unit, LOG_WARNING, filename, line, 0, - "Invalid Tunnel Encapsulation value, ignoring assignment: %d", k); - return 0; - } - - t->encap_limit = k; + r = config_parse_uint8_bounded( + unit, filename, line, section, section_line, lvalue, rvalue, + 0, UINT8_MAX, true, + &t->encap_limit); + if (r <= 0) + return r; t->flags &= ~IP6_TNL_F_IGN_ENCAP_LIMIT; + return 0; } @@ -1051,32 +1044,21 @@ int config_parse_erspan_version( void *data, void *userdata) { - uint8_t n, *v = ASSERT_PTR(data); - int r; - assert(filename); assert(lvalue); assert(rvalue); + uint8_t *v = ASSERT_PTR(data); + if (isempty(rvalue)) { *v = 1; /* defaults to 1 */ return 0; } - r = safe_atou8(rvalue, &n); - if (r < 0) { - log_syntax(unit, LOG_WARNING, filename, line, r, - "Failed to parse erspan version \"%s\", ignoring: %m", rvalue); - return 0; - } - if (!IN_SET(n, 0, 1, 2)) { - log_syntax(unit, LOG_WARNING, filename, line, 0, - "Invalid erspan version \"%s\", which must be 0, 1 or 2, ignoring.", rvalue); - return 0; - } - - *v = n; - return 0; + return config_parse_uint8_bounded( + unit, filename, line, section, section_line, lvalue, rvalue, + 0, 2, true, + v); } int config_parse_erspan_index( @@ -1091,32 +1073,21 @@ int config_parse_erspan_index( void *data, void *userdata) { - uint32_t n, *v = ASSERT_PTR(data); - int r; - assert(filename); assert(lvalue); assert(rvalue); + uint32_t *v = ASSERT_PTR(data); + if (isempty(rvalue)) { *v = 0; /* defaults to 0 */ return 0; } - r = safe_atou32(rvalue, &n); - if (r < 0) { - log_syntax(unit, LOG_WARNING, filename, line, r, - "Failed to parse erspan index \"%s\", ignoring: %m", rvalue); - return 0; - } - if (n >= 0x100000) { - log_syntax(unit, LOG_WARNING, filename, line, 0, - "Invalid erspan index \"%s\", which must be less than 0x100000, ignoring.", rvalue); - return 0; - } - - *v = n; - return 0; + return config_parse_uint32_bounded( + unit, filename, line, section, section_line, lvalue, rvalue, + 0, 0x100000 - 1, true, + v); } int config_parse_erspan_direction( @@ -1131,12 +1102,12 @@ int config_parse_erspan_direction( void *data, void *userdata) { - uint8_t *v = ASSERT_PTR(data); - assert(filename); assert(lvalue); assert(rvalue); + uint8_t *v = ASSERT_PTR(data); + if (isempty(rvalue) || streq(rvalue, "ingress")) *v = 0; /* defaults to ingress */ else if (streq(rvalue, "egress")) @@ -1160,32 +1131,21 @@ int config_parse_erspan_hwid( void *data, void *userdata) { - uint16_t n, *v = ASSERT_PTR(data); - int r; - assert(filename); assert(lvalue); assert(rvalue); + uint16_t *v = ASSERT_PTR(data); + if (isempty(rvalue)) { *v = 0; /* defaults to 0 */ return 0; } - r = safe_atou16(rvalue, &n); - if (r < 0) { - log_syntax(unit, LOG_WARNING, filename, line, r, - "Failed to parse erspan hwid \"%s\", ignoring: %m", rvalue); - return 0; - } - if (n >= 64) { - log_syntax(unit, LOG_WARNING, filename, line, 0, - "Invalid erspan index \"%s\", which must be less than 64, ignoring.", rvalue); - return 0; - } - - *v = n; - return 0; + return config_parse_uint16_bounded( + unit, filename, line, section, section_line, lvalue, rvalue, + 0, 63, true, + v); } static void netdev_tunnel_init(NetDev *netdev) { diff --git a/src/shared/conf-parser.c b/src/shared/conf-parser.c index af5aac7b29..a60588ad9e 100644 --- a/src/shared/conf-parser.c +++ b/src/shared/conf-parser.c @@ -1908,6 +1908,44 @@ int config_parse_in_addr_non_null( return 0; } +int config_parse_unsigned_bounded( + const char *unit, + const char *filename, + unsigned line, + const char *section, + unsigned section_line, + const char *name, + const char *value, + unsigned min, + unsigned max, + bool ignoring, + unsigned *ret) { + + int r; + + assert(filename); + assert(name); + assert(value); + assert(ret); + + r = safe_atou_bounded(value, min, max, ret); + if (r == -ERANGE) + log_syntax(unit, LOG_WARNING, filename, line, r, + "Invalid '%s=%s', allowed range is %u..%u%s.", + name, value, min, max, ignoring ? ", ignoring" : ""); + else if (r < 0) + log_syntax(unit, LOG_WARNING, filename, line, r, + "Failed to parse '%s=%s'%s: %m", + name, value, ignoring ? ", ignoring" : ""); + + if (r >= 0) + return 1; /* Return 1 if something was set */ + else if (ignoring) + return 0; + else + return r; +} + DEFINE_CONFIG_PARSE(config_parse_percent, parse_percent, "Failed to parse percent value"); DEFINE_CONFIG_PARSE(config_parse_permyriad, parse_permyriad, "Failed to parse permyriad value"); DEFINE_CONFIG_PARSE_PTR(config_parse_sec_fix_0, parse_sec_fix_0, usec_t, "Failed to parse time value"); diff --git a/src/shared/conf-parser.h b/src/shared/conf-parser.h index 140883969c..f1b03a8d15 100644 --- a/src/shared/conf-parser.h +++ b/src/shared/conf-parser.h @@ -385,3 +385,97 @@ typedef enum ConfigParseStringFlags { \ return free_and_replace(*enums, xs); \ } + +int config_parse_unsigned_bounded( + const char *unit, + const char *filename, + unsigned line, + const char *section, + unsigned section_line, + const char *name, + const char *value, + unsigned min, + unsigned max, + bool ignoring, + unsigned *ret); + +static inline int config_parse_uint32_bounded( + const char *unit, + const char *filename, + unsigned line, + const char *section, + unsigned section_line, + const char *name, + const char *value, + uint32_t min, + uint32_t max, + bool ignoring, + uint32_t *ret) { + + unsigned t; + int r; + + r = config_parse_unsigned_bounded( + unit, filename, line, section, section_line, name, value, + min, max, ignoring, + &t); + if (r <= 0) + return r; + assert(t <= UINT32_MAX); + *ret = t; + return 1; +} + +static inline int config_parse_uint16_bounded( + const char *unit, + const char *filename, + unsigned line, + const char *section, + unsigned section_line, + const char *name, + const char *value, + uint16_t min, + uint16_t max, + bool ignoring, + uint16_t *ret) { + + unsigned t; + int r; + + r = config_parse_unsigned_bounded( + unit, filename, line, section, section_line, name, value, + min, max, ignoring, + &t); + if (r <= 0) + return r; + assert(t <= UINT16_MAX); + *ret = t; + return 1; +} + +static inline int config_parse_uint8_bounded( + const char *unit, + const char *filename, + unsigned line, + const char *section, + unsigned section_line, + const char *name, + const char *value, + uint8_t min, + uint8_t max, + bool ignoring, + uint8_t *ret) { + + unsigned t; + int r; + + r = config_parse_unsigned_bounded( + unit, filename, line, section, section_line, name, value, + min, max, ignoring, + &t); + if (r <= 0) + return r; + assert(t <= UINT8_MAX); + *ret = t; + return 1; +}