On Wed, Jul 10, 2024 at 12:13:37AM GMT, Ilya Maximets wrote:
> 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.
>
You're right. I forgot. Thanks.
> > 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.
>
Thanks for the suggestion. It does seem to yield cleaner code.
> >
> > + 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]] .
Ack.
>
> > ofproto_unixctl_dpif_set_dp_features, NULL);
> > }
> >
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
