| Message ID | 20240707200905.2719071-2-amorenoz@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Eelco Chaudron |
| Headers | show |
| Series | Introduce local sampling with NXAST_SAMPLE action. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
On 4 Jul 2024, at 9:52, 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 documented > 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. Small nit below, the rest looks good to me. If this is the only change in the next series you can add my ACK. Cheers, Eelco > Signed-off-by: Adrian Moreno <amorenoz@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; > 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; Don't think we have to initialize br here. > + > + 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; > + } > > + 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 , > ofproto_unixctl_dpif_set_dp_features, NULL); > } > > -- > 2.45.2
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 <amorenoz@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); > } >
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 <amorenoz@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); > > } > > >
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; 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; + } + 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 , ofproto_unixctl_dpif_set_dp_features, NULL); }
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 <amorenoz@redhat.com> --- ofproto/ofproto-dpif.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-)