On 7/7/24 22:08, Adrian Moreno wrote:
> Datapath features can be set with dpif/set-dp-features unixctl command.
> This command is not docummented and therefore not supported in
> production but only useful for unit tests.
>
> A limitation was put in place originally to avoid enabling features at
> runtime that were disabled at boot time to avoid breaking the datapath
> in unexpected ways.
>
> But, considering users should not use this command and it should only be
> used for testing, we can assume whoever runs it knows what they are
> doing. Therefore, the limitation should be bypass-able.
>
> This patch adds a "--force" flag to the unixctl command to allow
> bypassing the mentioned limitation.
>
> Signed-off-by: Adrian Moreno <amore...@redhat.com>
> ---
> ofproto/ofproto-dpif.c | 37 +++++++++++++++++++++++++++----------
> 1 file changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index fcd7cd753..4712d10a8 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -6432,7 +6432,8 @@ display_support_field(const char *name,
> static bool
> dpif_set_support(struct dpif_backer_support *rt_support,
> struct dpif_backer_support *bt_support,
> - const char *name, const char *value, struct ds *ds)
> + const char *name, const char *value, bool force,
> + struct ds *ds)
> {
> struct shash all_fields = SHASH_INITIALIZER(&all_fields);
> struct dpif_support_field *field;
> @@ -6484,8 +6485,8 @@ dpif_set_support(struct dpif_backer_support *rt_support,
>
> if (field->type == DPIF_SUPPORT_FIELD_bool) {
> if (!strcasecmp(value, "true")) {
> - if (*(bool *)field->bt_ptr) {
> - *(bool *)field->rt_ptr = true;
> + if (*(bool *) field->bt_ptr || force) {
> + *(bool *) field->rt_ptr = true;
We discussed having a scary warning message for the force option.
I don't see it here.
> changed = true;
> } else {
> ds_put_cstr(ds, "Can not enable features not supported by
> the datapth");
> @@ -6720,21 +6721,36 @@ ofproto_unixctl_dpif_set_dp_features(struct
> unixctl_conn *conn,
> void *aux OVS_UNUSED)
> {
> struct ds ds = DS_EMPTY_INITIALIZER;
> - const char *br = argv[1];
> + struct ofproto_dpif *ofproto;
> + bool changed, force = false;
> const char *name, *value;
> - struct ofproto_dpif *ofproto = ofproto_dpif_lookup_by_name(br);
> - bool changed;
> + const char *br = NULL;
> +
> + if (!strcmp(argv[1], "--force")) {
> + force = true;
> + if (argc > 2) {
> + br = argv[2];
> + name = argc > 3 ? argv[3] : NULL;
> + value = argc > 4 ? argv[4] : NULL;
> + } else {
> + unixctl_command_reply_error(conn, "bridge not specified");
> + return;
> + }
> + } else {
> + br = argv[1];
> + name = argc > 2 ? argv[2] : NULL;
> + value = argc > 3 ? argv[3] : NULL;
> + }
It feels like this can be cleaner if we just eat the --force,
argv++, argc-- and continue the processing instead of doing
the same thing with diferent indexes in different branches.
>
> + ofproto = ofproto_dpif_lookup_by_name(br);
> if (!ofproto) {
> unixctl_command_reply_error(conn, "no such bridge");
> return;
> }
>
> - name = argc > 2 ? argv[2] : NULL;
> - value = argc > 3 ? argv[3] : NULL;
> changed = dpif_set_support(&ofproto->backer->rt_support,
> &ofproto->backer->bt_support,
> - name, value, &ds);
> + name, value, force, &ds);
> if (changed) {
> xlate_set_support(ofproto, &ofproto->backer->rt_support);
> udpif_flush(ofproto->backer->udpif);
> @@ -6777,7 +6793,8 @@ ofproto_unixctl_init(void)
> unixctl_command_register("dpif/dump-flows",
> "[-m] [--names | --no-names] bridge", 1,
> INT_MAX,
> ofproto_unixctl_dpif_dump_flows, NULL);
> - unixctl_command_register("dpif/set-dp-features", "bridge", 1, 3 ,
> + unixctl_command_register("dpif/set-dp-features",
> + "[--force] bridge [feature] [value]", 1, 4 ,
There is an extra space after '4'. It was there before, but it's
a good time to remove it.
Also, [feature [value]] .
> ofproto_unixctl_dpif_set_dp_features, NULL);
> }
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
