This is an RFC about making the ovs-vsctl more strict with argument checking for the set command in other_config option. This RFC is intentionally limited in scope as it is written to be a proof on concept and doesn't cover all the other KEY:VALUE pairs for other_config.
This RFC is intended to open up the discussion about making the set command more strict with its cli arguments. Ideally we would be strict on all known values for other_config. A lot of those are a choice between strings, or boolean, or a range of numbers. For those we could add an error when the user enters something invalid, which is what this patch is trying to show that we can do. ===== PATCH ===== The set command from the ovs-vsctl utility allows users to set values on the ovs database using KEY:VALUE pairs. The problem is that the ovs-vsctl utility will accept any arbitrary string for KEYS that have strict requirements like a choice or integer range. The only way to determine if there was an error in the user's cli arguments is to look at the logs. This patch makes it so that the ovs-vsctl utility errors on invalid KEY:VALUE pairs allowing users to fix their cli arguments. Here are a couple of examples of before and after applying this patch Before. No errors and added to db ================================= sh# ovs-vsctl set Open_vSwitch . other_config:tc-policy=skip-sw sh# ovs-vsctl set Open_vSwitch . other_config:hw-offload=treu sh# ovs-vsctl set Open_vSwitch . other_config:tx-flush-interval=1242143213 After. Errors, not added to db ============================== sh# ovs-vsctl set Open_vSwitch . other_config:tc-policy=skip-sw ovs-vsctl: Could not find choice "skip-sw". Your choices are skip_sw, skip_hw, none, sh# ovs-vsctl set Open_vSwitch . other_config:tx-flush-interval=1242143213 ovs-vsctl: The value 1242143213 is not in the range [0, 1000000] required for tx-flush-interval sh# ovs-vsctl set Open_vSwitch . other_config:hw-offload=treu ovs-vsctl: Could not find choice "treu". Your choices are true, false, 0, 1, Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1990130 Signed-off-by: Michael Santana <[email protected]> --- lib/db-ctl-base.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c index 134496ef3..b35ec99c7 100644 --- a/lib/db-ctl-base.c +++ b/lib/db-ctl-base.c @@ -535,6 +535,21 @@ missing_operator_error(const char *arg, const char **allowed_operators, return ds_steal_cstr(&s); } +static char * OVS_WARN_UNUSED_RESULT +generate_choices_str(char *choices[], int choice_size, char *valuep) +{ + struct ds s; + ds_init(&s); + + ds_put_format(&s, "Could not find choice \"%s\". ", valuep); + ds_put_format(&s, "Your choices are "); + for (int choice = 0; choice < choice_size; choice++) { + ds_put_format(&s, "%s, ", choices[choice]); + } + + return ds_steal_cstr(&s); +} + /* Breaks 'arg' apart into a number of fields in the following order: * * - The name of a column in 'table', stored into '*columnp'. The column @@ -561,8 +576,20 @@ parse_column_key_value(const char *arg, const char **allowed_operators, size_t n_allowed, char **valuep) { + char *tc_policy_choices[] = {"skip_sw", "skip_hw", "none"}; + char *hw_offload_choices[] = {"true", "false", "0", "1"}; + int hw_size = ARRAY_SIZE(hw_offload_choices); + int tc_size = ARRAY_SIZE(tc_policy_choices); + + const char *TX_FLUSH = "tx-flush-interval"; + const char *HW_OFFLOAD = "hw-offload"; + const char *TC_POLICY = "tc-policy"; + int mintx_flush_interval_max = 1000000; + int mintx_flush_interval_min = 0; + const char *p = arg; char *column_name; + char *choicep; char *error; ovs_assert(!(operatorp && !valuep)); @@ -635,6 +662,46 @@ parse_column_key_value(const char *arg, goto error; } } + + bool invalid = false; + if (!strncmp(*keyp, TC_POLICY, strlen(TC_POLICY))) { + invalid = true; + for (int choice = 0; choice < tc_size; choice++) { + choicep = tc_policy_choices[choice]; + if (!strncmp(*valuep, choicep, strlen(choicep))) { + invalid = false; + } + } + if (invalid) { + error = xasprintf("%s", generate_choices_str(tc_policy_choices, + tc_size, *valuep)); + goto error; + } + } else if (!strncmp(*keyp, HW_OFFLOAD, strlen(HW_OFFLOAD))) { + invalid = true; + for (int choice = 0; choice < hw_size; choice++) { + choicep = hw_offload_choices[choice]; + if (!strncmp(*valuep, choicep, strlen(choicep))) { + invalid = false; + } + } + if (invalid) { + error = xasprintf("%s", generate_choices_str(hw_offload_choices, + hw_size, *valuep)); + goto error; + } + } else if (!strncmp(*keyp, TX_FLUSH, strlen(TX_FLUSH))) { + long int_value = atol(*valuep); + if (int_value < mintx_flush_interval_min || + int_value > mintx_flush_interval_max) { + error = xasprintf("The value %ld is not in the range [%d, %d] " + "required for %s", int_value, + mintx_flush_interval_min, + mintx_flush_interval_max, *keyp); + goto error; + } + } + return NULL; error: -- 2.33.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
