| Message ID | 20230111184621.73694-1-msantana@redhat.com |
|---|---|
| State | RFC |
| Headers | show |
| Series | [ovs-dev,RFC] ovs-vsctl: Be more strict in cli arguments | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
On Wed, Jan 11, 2023 at 1:46 PM Michael Santana <msantana@redhat.com> wrote: > > 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. > I think this is a good idea, in fact, I believe someone was asking about this on IRC today. However, I think there could be a better approach to implementation. One option would be to modify ovsdb-idlc, types.py, ovsdb-data.c, and the ovsschema files to add a new type of validation. This is where a lot of the other parameters are already validated, things like ranges, lengths, enumerations, and even UUID formats are already supported. You could add a new special type of constraint for the nested values like in other_config. The advantage of this is that we could extend it and validate things like Port options as well, and adding new constraints would be as easy as modifying the ovsschema file. An additional benefit of having this validation live in ovsschema is we could generate or validate a variety of documentation from it as well in the future. WDYT? M > > ===== > 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 <msantana@redhat.com> > > --- > 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 >
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:
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 <msantana@redhat.com> --- lib/db-ctl-base.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+)