From bb850fda0ed98988a48f81928b8dedfa7ae65b6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=8D=C3=B1igo=20Huguet?= Date: Fri, 14 Mar 2025 07:43:23 +0100 Subject: [PATCH 1/4] nmcli: connection: process port-type, type and controller first If the connection is a port we need to set the connection.port-type property. Usually this property is guessed by nmcli depending on the connection type or the chosen controller, so it doesn't need to be specified by the user. However, if it is explicitly set by the user we should not guess, but just use it. When we process arguments like "controller" or "type" we call custom functions like set_connection_controller that will guess the port-type if needed. By processing port-type first, it will be set in the connection by the time that these other properties are processed, so they won't try to guess. After port-type, process connection.type and connection.controller, as we are usually capable of deducing the port-type from them. Type needs to be processed first because some types like bond-slave or ovs-port have only one possible port-type value so we must not try to guess from the controller. Fixes: c5324ed285af ('nmcli: streamline connection addition') --- src/nmcli/connections.c | 195 +++++++++++++++++++++++++++++----------- src/nmcli/connections.h | 12 +-- src/nmcli/devices.c | 2 +- 3 files changed, 151 insertions(+), 58 deletions(-) diff --git a/src/nmcli/connections.c b/src/nmcli/connections.c index 6ce590d50e..ed7a7b62d9 100644 --- a/src/nmcli/connections.c +++ b/src/nmcli/connections.c @@ -5229,19 +5229,101 @@ get_value(const char **value, return TRUE; } -gboolean -nmc_process_connection_properties(NmCli *nmc, - NMConnection *connection, - int *argc, - const char *const **argv, - gboolean allow_setting_removal, - GError **error) +static int +_copy_connection_properties(const char ***dst, + const char *const *src, + gboolean invert_match, + const char *const *options_list_match) { + const char *option; + gboolean match; + int count = 0; + + while (*src) { + option = (**src == '+' || **src == '-') ? *src + 1 : *src; + match = _nm_g_strv_contains(options_list_match, option); + match = invert_match ? !match : match; + if (match) { + *((*dst)++) = src[0]; + *((*dst)++) = src[1]; + count += 2; + } + src++; + if (*src) /* Might be the NULL termination, already */ + src++; + } + + return count; +} + +gboolean +nmc_process_connection_properties(NmCli *nmc, + NMConnection *connection, + int argc, + const char *const *argv, + gboolean allow_setting_removal, + GError **error) +{ + gs_free const char **to_free = NULL; + + if (argc == 0) + return TRUE; + /* First check if we have a port-type, as this would mean we will not * have ip properties but possibly others, port-type specific. + * Then check connection.type and connection.controller, as port-type might + * be deduced from them. + * Don't reorder if we are doing CLI argument completion, as it might give + * unexpected results */ + if (!nmc->complete) { + const char **dst; + + dst = to_free = g_new(const char *, argc + 1); + + argc = _copy_connection_properties( + &dst, + argv, + FALSE, + NM_MAKE_STRV(NM_SETTING_CONNECTION_SETTING_NAME "." NM_SETTING_CONNECTION_PORT_TYPE, + "port-type", /* alias */ + NM_SETTING_CONNECTION_SETTING_NAME "." NM_SETTING_CONNECTION_SLAVE_TYPE, + "slave-type" /* alias */)); + argc += _copy_connection_properties(&dst, + argv, + FALSE, + NM_MAKE_STRV(NM_SETTING_CONNECTION_SETTING_NAME + "." NM_SETTING_CONNECTION_TYPE, + "type" /* alias */)); + argc += _copy_connection_properties( + &dst, + argv, + FALSE, + NM_MAKE_STRV(NM_SETTING_CONNECTION_SETTING_NAME "." NM_SETTING_CONNECTION_CONTROLLER, + "controller", /* alias */ + NM_SETTING_CONNECTION_SETTING_NAME "." NM_SETTING_CONNECTION_MASTER, + "master" /* alias */)); + argc += _copy_connection_properties( + &dst, + argv, + TRUE, + NM_MAKE_STRV(NM_SETTING_CONNECTION_SETTING_NAME "." NM_SETTING_CONNECTION_PORT_TYPE, + "port-type", + NM_SETTING_CONNECTION_SETTING_NAME "." NM_SETTING_CONNECTION_SLAVE_TYPE, + "slave-type", + NM_SETTING_CONNECTION_SETTING_NAME "." NM_SETTING_CONNECTION_TYPE, + "type", + NM_SETTING_CONNECTION_SETTING_NAME "." NM_SETTING_CONNECTION_CONTROLLER, + "controller", + NM_SETTING_CONNECTION_SETTING_NAME "." NM_SETTING_CONNECTION_MASTER, + "master")); + + *dst = NULL; /* NULL terminated as expected by get_value() */ + argv = to_free; + } + /* Go through arguments and set properties */ - do { + while (argc) { const NMMetaSettingValidPartItem *const *type_settings; const NMMetaSettingValidPartItem *const *port_settings; NMMetaAccessorModifier modifier; @@ -5260,19 +5342,10 @@ nmc_process_connection_properties(NmCli *nmc, ensure_settings(connection, port_settings); ensure_settings(connection, type_settings); - if (*argc <= 0) { - g_set_error_literal(error, - NMCLI_ERROR, - NMC_RESULT_ERROR_USER_INPUT, - _("Error: . argument is missing.")); - return FALSE; - } - nm_assert(argv); nm_assert(*argv); - nm_assert(**argv); - option_orig = **argv; + option_orig = *argv; switch (option_orig[0]) { case '+': @@ -5294,15 +5367,15 @@ nmc_process_connection_properties(NmCli *nmc, NMSetting *ss; const char *setting_name; - (*argc)--; - (*argv)++; + argc--; + argv++; - if (*argc == 1 && nmc->complete) { + if (argc == 1 && nmc->complete) { complete_existing_setting(nmc, connection, value); - return TRUE; + break; } - if (!*argc) { + if (!argc) { g_set_error_literal(error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, @@ -5310,9 +5383,9 @@ nmc_process_connection_properties(NmCli *nmc, return FALSE; } - setting_name = **argv; - (*argc)--; - (*argv)++; + setting_name = *argv; + argc--; + argv++; ss = is_setting_valid(connection, type_settings, port_settings, setting_name); if (!ss) { @@ -5342,7 +5415,7 @@ nmc_process_connection_properties(NmCli *nmc, /* This seems like a . (such as "connection.id" or "bond.mode"), * optionally prefixed with "+| or "-". */ - if (*argc == 1 && nmc->complete) + if (argc == 1 && nmc->complete) complete_property_name(nmc, connection, modifier, option_sett, option_prop); option_sett_expanded = @@ -5358,14 +5431,14 @@ nmc_process_connection_properties(NmCli *nmc, return FALSE; } - (*argc)--; - (*argv)++; - if (!get_value(&value, argc, argv, option_orig, error)) + argc--; + argv++; + if (!get_value(&value, &argc, &argv, option_orig, error)) return FALSE; - if (!*argc && nmc->complete) { + if (!argc && nmc->complete) { complete_property(nmc, option_sett, option_prop, value ?: "", connection); - return TRUE; + break; } if (!set_property(nmc->client, @@ -5447,7 +5520,7 @@ nmc_process_connection_properties(NmCli *nmc, } if (!chosen) { - if (*argc == 1 && nmc->complete) { + if (argc == 1 && nmc->complete) { if (allow_setting_removal && g_str_has_prefix("remove", option)) nmc_print("remove\n"); complete_property_name(nmc, connection, modifier, option, NULL); @@ -5460,21 +5533,20 @@ nmc_process_connection_properties(NmCli *nmc, return FALSE; } - if (*argc == 1 && nmc->complete) + if (argc == 1 && nmc->complete) complete_property_name(nmc, connection, modifier, option, NULL); - (*argc)--; - (*argv)++; - if (!get_value(&value, argc, argv, option_orig, error)) + argc--; + argv++; + if (!get_value(&value, &argc, &argv, option_orig, error)) return FALSE; - if (!*argc && nmc->complete) + if (!argc && nmc->complete) complete_option(nmc, chosen, value ?: "", connection); if (!set_option(nmc, connection, chosen, value, TRUE, error)) return FALSE; - - } while (*argc); + } return TRUE; } @@ -5911,6 +5983,7 @@ nmc_add_connection(NmCli *nmc, NMConnection *connection, gboolean temporary) static void do_connection_add(const NMCCommand *cmd, NmCli *nmc, int argc, const char *const *argv) { + gs_unref_ptrarray GPtrArray *props; gs_unref_object NMConnection *connection = NULL; NMSettingConnection *s_con; gs_free_error GError *error = NULL; @@ -5929,16 +6002,21 @@ do_connection_add(const NMCCommand *cmd, NmCli *nmc, int argc, const char *const s_con = (NMSettingConnection *) nm_setting_connection_new(); nm_connection_add_setting(connection, NM_SETTING(s_con)); -read_properties: - g_clear_error(&error); - /* Get the arguments from the command line if any */ - if (argc && !nmc_process_connection_properties(nmc, connection, &argc, &argv, FALSE, &error)) { - if (nm_streq0(*argv, "--") && !seen_dash_dash) { + props = g_ptr_array_new_full(sizeof(const char *) * (argc + 1), NULL); + + while (argc) { + if (nm_streq0(*argv, "--")) { /* This is for compatibility with older nmcli that required * options and properties to be separated with "--" */ - seen_dash_dash = TRUE; - next_arg(nmc, &argc, &argv, NULL); - goto read_properties; + if (seen_dash_dash) { + g_string_printf(nmc->return_text, + _("Error: argument '--' can only be passed once")); + nmc->return_value = NMC_RESULT_ERROR_USER_INPUT; + goto finish; + } else { + seen_dash_dash = TRUE; + next_arg(nmc, &argc, &argv, NULL); + } } else if (nm_streq0(*argv, "save")) { /* It would be better if "save" was a separate argument and not * mixed with properties, but there's not much we can do about it now. */ @@ -5951,16 +6029,31 @@ read_properties: nmc->return_value = NMC_RESULT_ERROR_USER_INPUT; goto finish; } - g_clear_error(&error); if (!nmc_string_to_bool(*argv, &save_bool, &error)) { g_string_printf(nmc->return_text, _("Error: 'save': %s."), error->message); nmc->return_value = NMC_RESULT_ERROR_USER_INPUT; goto finish; } next_arg(nmc, &argc, &argv, NULL); - goto read_properties; + } else { + g_ptr_array_add(props, (gpointer) *argv); + argc--; + argv++; + if (argc > 0) { + g_ptr_array_add(props, (gpointer) *argv); + argc--; + argv++; + } } + } + g_ptr_array_add(props, NULL); /* Must be NULL terminated */ + if (!nmc_process_connection_properties(nmc, + connection, + props->len - 1, + (const char *const *) props->pdata, + FALSE, + &error)) { g_string_assign(nmc->return_text, error->message); nmc->return_value = error->code; goto finish; @@ -9251,7 +9344,7 @@ do_connection_modify(const NMCCommand *cmd, NmCli *nmc, int argc, const char *co /* Don't insist on having argument if we're running in offline mode. */ if (!nmc->nmc_config.offline || argc > 0) { - if (!nmc_process_connection_properties(nmc, connection, &argc, &argv, TRUE, &error)) { + if (!nmc_process_connection_properties(nmc, connection, argc, argv, TRUE, &error)) { g_string_assign(nmc->return_text, error->message); nmc->return_value = error->code; return; diff --git a/src/nmcli/connections.h b/src/nmcli/connections.h index 48c981704d..a343ca256c 100644 --- a/src/nmcli/connections.h +++ b/src/nmcli/connections.h @@ -12,12 +12,12 @@ void nmc_monitor_connections(NmCli *nmc); const char *nmc_connection_check_deprecated(NMConnection *c); -gboolean nmc_process_connection_properties(NmCli *nmc, - NMConnection *connection, - int *argc, - const char *const **argv, - gboolean allow_remove_setting, - GError **error); +gboolean nmc_process_connection_properties(NmCli *nmc, + NMConnection *connection, + int argc, + const char *const *args, + gboolean allow_remove_setting, + GError **error); NMMetaColor nmc_active_connection_state_to_color(NMActiveConnection *ac); diff --git a/src/nmcli/devices.c b/src/nmcli/devices.c index b308e4406d..9a0181138b 100644 --- a/src/nmcli/devices.c +++ b/src/nmcli/devices.c @@ -2596,7 +2596,7 @@ modify_get_applied_cb(GObject *object, GAsyncResult *result, gpointer user_data) argc = info->argc; argv = (const char *const *) info->argv; - if (!nmc_process_connection_properties(info->nmc, connection, &argc, &argv, TRUE, &error)) { + if (!nmc_process_connection_properties(info->nmc, connection, argc, argv, TRUE, &error)) { g_string_assign(nmc->return_text, error->message); nmc->return_value = error->code; quit(); From 6a133d10a1e54c21fbacf7893f69c71fc6a41f48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=8D=C3=B1igo=20Huguet?= Date: Fri, 14 Mar 2025 08:18:46 +0100 Subject: [PATCH 2/4] nmcli: connection: don't overwrite port-type if explicitly set When processing the "type" property we deduce the port-type in some cases and set it. If the user has chosen a port-type we must not overwrite it. In any case, we should raise an error when validating the connection. Fixes: c5324ed285af ('nmcli: streamline connection addition') --- src/nmcli/connections.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/nmcli/connections.c b/src/nmcli/connections.c index ed7a7b62d9..d4c583f86f 100644 --- a/src/nmcli/connections.c +++ b/src/nmcli/connections.c @@ -4616,8 +4616,11 @@ set_connection_type(NmCli *nmc, gboolean allow_reset, GError **error) { - GError *local = NULL; - const char *port_type = NULL; + NMSettingConnection *s_con = nm_connection_get_setting_connection(con); + GError *local = NULL; + const char *port_type = NULL; + + nm_assert(s_con); value = check_valid_name_toplevel(value, &port_type, &local); if (!value) { @@ -4632,7 +4635,7 @@ set_connection_type(NmCli *nmc, return FALSE; } - if (port_type) { + if (!nm_setting_connection_get_port_type(s_con) && port_type) { if (!set_property(nmc->client, con, NM_SETTING_CONNECTION_SETTING_NAME, From 87a5d89f750d03a27402f5fd5470bed6723c5a98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=8D=C3=B1igo=20Huguet?= Date: Fri, 14 Mar 2025 12:11:29 +0100 Subject: [PATCH 3/4] nmcli: choose the right port-type for OVS Normally it is possible not to define port-type in nmcli and it deduces it from connection.type or connection.controller. Some types like 'bond-slave' have a single possible value for port-type. In other cases nmcli deduces the port-type by getting the controller's type, like 'bond'. For OVS connections, the second method of guessing by the controller's type was used. However, in OVS it is common to have different devices with the same name, causing nmcli to use "ovs-interface" as port-type if it matched by controller name. Fix if by deducing the port-type from the connection's type. An ovs-port connection must always have port-type=ovs-bridge, and an ovs-interface connection must always have port-type=ovs-port. Note that this is something that should be done in the daemon, not in the clients, but this is a small patch that makes it to work in nmcli, at least. Without this, the mechanism of guessing from the parent would act, leading to wrong results. Ideally, all this should be done in the daemon, but currently many checks in nmcli/libnm depends on having the port-type set, and it would be lot of work to change it. Fixes: c5324ed285af ('nmcli: streamline connection addition') --- src/nmcli/connections.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/nmcli/connections.c b/src/nmcli/connections.c index d4c583f86f..be89e992f5 100644 --- a/src/nmcli/connections.c +++ b/src/nmcli/connections.c @@ -3861,6 +3861,11 @@ check_valid_name_toplevel(const char *val, const char **port_type, GError **erro return NM_SETTING_WIRED_SETTING_NAME; } + if (nm_streq(str, "ovs-port")) + NM_SET_OUT(port_type, NM_SETTING_OVS_BRIDGE_SETTING_NAME); + else if (nm_streq(str, "ovs-interface")) + NM_SET_OUT(port_type, NM_SETTING_OVS_PORT_SETTING_NAME); + setting_info = nm_meta_setting_info_editor_find_by_name(str, TRUE); if (setting_info) return setting_info->general->setting_name; From 9f6562869b664f330aab36085457bda1487c8631 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=8D=C3=B1igo=20Huguet?= Date: Wed, 9 Apr 2025 16:06:43 +0200 Subject: [PATCH 4/4] nmcli: improve the warning message about no controller found When nmcli tries to match a controller it filters by its type. The controller's type must match with the port's port-type. If no controller matches, the printed warning was "doesn't refer to any existing profile". However, the profile might exist, but with wrong type. Improve the message so it makes that clear. Fixes: aa12bb353bca ('cli: discover slave type for a connection with a master') --- src/nmcli/connections.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/nmcli/connections.c b/src/nmcli/connections.c index be89e992f5..f53f2610d0 100644 --- a/src/nmcli/connections.c +++ b/src/nmcli/connections.c @@ -4005,8 +4005,10 @@ normalized_controller_for_port(const GPtrArray *connections, } if (!out_controller) { - nmc_printerr(_("Warning: controller='%s' doesn't refer to any existing profile.\n"), - controller); + nmc_printerr( + _("Warning: controller '%s' doesn't refer to any existing profile of type '%s'.\n"), + controller, + type); out_controller = controller; if (out_type) *out_type = type;