| Message ID | 20250520171649.39875-1-arukomoinikova@k2.cloud |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v2] lib: Added filter option in show command. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/cirrus-robot | success | cirrus build: passed |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 5/20/25 7:16 PM, Alexandra Rukomoinikova wrote: > Added the ability to filter the output of the show command > using the filter option. > > Signed-off-by: Alexandra Rukomoinikova <arukomoinikova@k2.cloud> > --- > v1 --> v2 : did as Ilya said. > --- > lib/db-ctl-base.c | 75 +++++++++++++++++++++++++++++++++++++++++++++- > tests/ovs-vsctl.at | 28 +++++++++++++++++ > 2 files changed, 102 insertions(+), 1 deletion(-) Hi, Alexandra. Thanks for the new version! nit: The 'lib:' in the patch name doesn't make a lot of sense as vast majority of OVS code is in lib. It should be 'db-ctl-base:' instead. Since this patch adds a new option, it needs to be documented in the man page (utilities/ovs-vsctl.8.in) and a NEWS entry should be added. Some more comments below. > > diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c > index 1f157e46c..45d2d51c0 100644 > --- a/lib/db-ctl-base.c > +++ b/lib/db-ctl-base.c > @@ -2243,19 +2243,92 @@ cmd_show_row(struct ctl_context *ctx, const struct ovsdb_idl_row *row, > sset_find_and_delete_assert(shown, show->table->name); > } > > +struct output_filter { > + char **filters; > + size_t n_filters; > +}; > + > +static void > +filter_row(struct ctl_context *ctx, > + struct output_filter *filter, > + size_t old_lenght) s/lenght/length/ here and in a few other places in the code. > +{ > + bool filter_match = false; > + > + for (size_t i = 0; i < filter->n_filters; i++) { > + if (strstr(&ctx->output.string[old_lenght], filter->filters[i])) { > + filter_match = true; break here. There is no need to continue the loop. > + } > + } > + > + if (!filter_match) { > + ds_truncate(&ctx->output, old_lenght); > + } > +} > + > +static struct output_filter * > +init_filters(struct ctl_context *ctx) > +{ > + char *filter_str = shash_find_data(&ctx->options, "--filter"); > + if (!filter_str || !*filter_str) { > + return NULL; > + } > + > + size_t count = 1; > + for (const char *p = filter_str; *p; p++) { > + if (*p == '|') { > + count++; > + } > + } > + > + struct output_filter *filter = xmalloc(sizeof(struct output_filter)); > + filter->filters = xmalloc(count * (sizeof(char *))); > + filter->n_filters = count; > + > + char *ptr = NULL; > + char *token = strtok_r(filter_str, "|", &ptr); > + size_t idx = 0; > + > + while (token && idx < count) { > + filter->filters[idx++] = xstrdup(token); > + token = strtok_r(NULL, "|", &ptr); > + } > + > + return filter; > +} If we use sset as a data structure for the filters, the code above can be mostly replaced with an sset_from_delimited_string() call. The sset is slightly less memory-efficient, but I don't think we care in this particular case. > + > +static void > +free_filters(struct output_filter *filter) > +{ > + if (!filter) { > + return; > + } > + for (size_t i = 0; i < filter->n_filters; i++) { > + free(filter->filters[i]); > + } > + free(filter->filters); > + free(filter); > +} > + > static void > cmd_show(struct ctl_context *ctx) > { > const struct ovsdb_idl_row *row; > + struct output_filter *filter = init_filters(ctx); > struct sset shown = SSET_INITIALIZER(&shown); > > for (row = ovsdb_idl_first_row(ctx->idl, cmd_show_tables[0].table); > row; row = ovsdb_idl_next_row(row)) { > + size_t lenght_before = ctx->output.length; > cmd_show_row(ctx, row, 0, &shown); > + if (filter) { > + filter_row(ctx, filter, lenght_before); > + } This only covers the top-level elements, but the idea was to perform the filtering recursively for every level. You should move the filtering into the cmd_show_row() itself, i.e. save the length at the top of cmd_show_row() and run the filtering at the bottom. This will ensure that we're filtering all the way through. For example, if we have a setup like this: $ ovs-vsctl add-br br1 $ ovs-vsctl add-br br0 $ ovs-vsctl add-port br0 p1 $ ovs-vsctl add-port br0 tunnel_port \ -- set Interface tunnel_port type=geneve \ options:remote_ip=flow options:key=123 $ ovs-vsctl add-port br0 tunnel_port2 \ -- set Interface tunnel_port2 type=geneve \ options:remote_ip=flow options:key=125 Then I can get a list of bridges only with: $ ovs-vsctl --filter="Bridge" show d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 Bridge br1 Bridge br0 Since the 'Bridge' match happens at the higher level, all the port and interface information is filtered out. Or a list of ports per bridge: $ ovs-vsctl --filter="Port" show d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 Bridge br1 Port br1 Bridge br0 Port p1 Port tunnel_port Port tunnel_port2 Port br0 Here only the interface information is filtered out since once we match on the 'Port' string, all the higher-level nodes will match as well. Or if I want to only show geneve interfaces: $ ovs-vsctl --filter="geneve" show d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 Bridge br0 Port tunnel_port Interface tunnel_port type: geneve options: {key="123", remote_ip=flow} Port tunnel_port2 Interface tunnel_port2 type: geneve options: {key="125", remote_ip=flow} Or if I want to show all tunnels with the tunnel key 123: $ ovs-vsctl --filter='key="123"' show d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 Bridge br0 Port tunnel_port Interface tunnel_port type: geneve options: {key="123", remote_ip=flow} Or I want the previous one, but also names of all the ports per bridge: $ ovs-vsctl --filter='key="123"|Port' show d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 Bridge br1 Port br1 Bridge br0 Port p1 Port tunnel_port Interface tunnel_port type: geneve options: {key="123", remote_ip=flow} Port tunnel_port2 Port br0 What do you think? > } > > ovs_assert(sset_is_empty(&shown)); > sset_destroy(&shown); > + free_filters(filter); > } > > > @@ -2548,7 +2621,7 @@ ctl_init__(const struct ovsdb_idl_class *idl_class_, > cmd_show_tables = cmd_show_tables_; > if (cmd_show_tables) { > static const struct ctl_command_syntax show = > - {"show", 0, 0, "", pre_cmd_show, cmd_show, NULL, "", RO}; > + {"show", 0, 0, "", pre_cmd_show, cmd_show, NULL, "--filter=", RO}; > ctl_register_command(&show); > } > } > diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at > index e488e292d..010889789 100644 > --- a/tests/ovs-vsctl.at > +++ b/tests/ovs-vsctl.at > @@ -1831,3 +1831,31 @@ AT_CHECK([ovs-vsctl --no-wait --bare --columns _uuid,name list bridge tst1], [0] > > OVS_VSCTL_CLEANUP > AT_CLEANUP > + > +AT_SETUP([ovs-vsctl filter option usage]) nit: I'd name it 'ovs-vsctl show filtered' or something like that. > +AT_KEYWORDS([ovs-vsctl filter option]) nit :I'd replace the 'option' with 'show'. 'option' is a not very specific keyword to use. > + > +OVS_VSWITCHD_START([dnl > + add-port br0 p1 -- set Interface p1 type=internal ofport_request=1 -- \ > + add-port br0 p2 -- set Interface p2 type=internal ofport_request=2 -- \ > + add-port br0 p3 -- set Interface p3 type=internal ofport_request=3 > +]) > + > +AT_CHECK([ovs-vsctl --filter='p1' show | grep Port | sort], [0], [dnl > + Port br0 > + Port p1 > + Port p2 > + Port p3 > +]) > + > +AT_CHECK([ovs-vsctl --filter='test' show | grep Port | sort], [0], []) > + > +AT_CHECK([ovs-vsctl --filter='Bridge' show | grep Port | sort], [0], [dnl > + Port br0 > + Port p1 > + Port p2 > + Port p3 > +]) We should print the whole thing here. It will not change from run to run, because database structures are sorted and our top-level table only has one row. The only thing that will change is the datapath uuid at the very top. You may just cut out the first line with tail -n +2 or feed the output into the uuidfilt. Maybe add a bit more variety to the tests. Different interface types. > + > +OVS_VSCTL_CLEANUP > +AT_CLEANUP > \ No newline at end of file Add the new line here! Best regards, Ilya Maximets.
On 5/29/25 6:02 PM, Ilya Maximets wrote: > On 5/20/25 7:16 PM, Alexandra Rukomoinikova wrote: >> Added the ability to filter the output of the show command >> using the filter option. >> >> Signed-off-by: Alexandra Rukomoinikova <arukomoinikova@k2.cloud> >> --- >> v1 --> v2 : did as Ilya said. >> --- >> lib/db-ctl-base.c | 75 +++++++++++++++++++++++++++++++++++++++++++++- >> tests/ovs-vsctl.at | 28 +++++++++++++++++ >> 2 files changed, 102 insertions(+), 1 deletion(-) > > Hi, Alexandra. Thanks for the new version! > > nit: The 'lib:' in the patch name doesn't make a lot of sense as vast > majority of OVS code is in lib. It should be 'db-ctl-base:' instead. > > Since this patch adds a new option, it needs to be documented in the > man page (utilities/ovs-vsctl.8.in) and a NEWS entry should be added. > > Some more comments below. > >> >> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c >> index 1f157e46c..45d2d51c0 100644 >> --- a/lib/db-ctl-base.c >> +++ b/lib/db-ctl-base.c >> @@ -2243,19 +2243,92 @@ cmd_show_row(struct ctl_context *ctx, const struct ovsdb_idl_row *row, >> sset_find_and_delete_assert(shown, show->table->name); >> } >> >> +struct output_filter { >> + char **filters; >> + size_t n_filters; >> +}; >> + >> +static void >> +filter_row(struct ctl_context *ctx, >> + struct output_filter *filter, >> + size_t old_lenght) > > s/lenght/length/ > > here and in a few other places in the code. > >> +{ >> + bool filter_match = false; >> + >> + for (size_t i = 0; i < filter->n_filters; i++) { >> + if (strstr(&ctx->output.string[old_lenght], filter->filters[i])) { >> + filter_match = true; > > break here. There is no need to continue the loop. > >> + } >> + } >> + >> + if (!filter_match) { >> + ds_truncate(&ctx->output, old_lenght); >> + } >> +} >> + >> +static struct output_filter * >> +init_filters(struct ctl_context *ctx) >> +{ >> + char *filter_str = shash_find_data(&ctx->options, "--filter"); >> + if (!filter_str || !*filter_str) { >> + return NULL; >> + } >> + >> + size_t count = 1; >> + for (const char *p = filter_str; *p; p++) { >> + if (*p == '|') { >> + count++; >> + } >> + } >> + >> + struct output_filter *filter = xmalloc(sizeof(struct output_filter)); >> + filter->filters = xmalloc(count * (sizeof(char *))); >> + filter->n_filters = count; >> + >> + char *ptr = NULL; >> + char *token = strtok_r(filter_str, "|", &ptr); >> + size_t idx = 0; >> + >> + while (token && idx < count) { >> + filter->filters[idx++] = xstrdup(token); >> + token = strtok_r(NULL, "|", &ptr); >> + } >> + >> + return filter; >> +} > > If we use sset as a data structure for the filters, the code above can > be mostly replaced with an sset_from_delimited_string() call. > The sset is slightly less memory-efficient, but I don't think we care > in this particular case. > >> + >> +static void >> +free_filters(struct output_filter *filter) >> +{ >> + if (!filter) { >> + return; >> + } >> + for (size_t i = 0; i < filter->n_filters; i++) { >> + free(filter->filters[i]); >> + } >> + free(filter->filters); >> + free(filter); >> +} >> + >> static void >> cmd_show(struct ctl_context *ctx) >> { >> const struct ovsdb_idl_row *row; >> + struct output_filter *filter = init_filters(ctx); >> struct sset shown = SSET_INITIALIZER(&shown); >> >> for (row = ovsdb_idl_first_row(ctx->idl, cmd_show_tables[0].table); >> row; row = ovsdb_idl_next_row(row)) { >> + size_t lenght_before = ctx->output.length; >> cmd_show_row(ctx, row, 0, &shown); >> + if (filter) { >> + filter_row(ctx, filter, lenght_before); >> + } > > This only covers the top-level elements, but the idea was to perform the > filtering recursively for every level. You should move the filtering into > the cmd_show_row() itself, i.e. save the length at the top of cmd_show_row() > and run the filtering at the bottom. This will ensure that we're filtering > all the way through. > > For example, if we have a setup like this: > > $ ovs-vsctl add-br br1 > $ ovs-vsctl add-br br0 > $ ovs-vsctl add-port br0 p1 > $ ovs-vsctl add-port br0 tunnel_port \ > -- set Interface tunnel_port type=geneve \ > options:remote_ip=flow options:key=123 > $ ovs-vsctl add-port br0 tunnel_port2 \ > -- set Interface tunnel_port2 type=geneve \ > options:remote_ip=flow options:key=125 > > Then I can get a list of bridges only with: > > $ ovs-vsctl --filter="Bridge" show > d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 > Bridge br1 > Bridge br0 > > Since the 'Bridge' match happens at the higher level, all the port > and interface information is filtered out. > > Or a list of ports per bridge: > > $ ovs-vsctl --filter="Port" show > d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 > Bridge br1 > Port br1 > Bridge br0 > Port p1 > Port tunnel_port > Port tunnel_port2 > Port br0 > > Here only the interface information is filtered out since once we match > on the 'Port' string, all the higher-level nodes will match as well. > > Or if I want to only show geneve interfaces: > > $ ovs-vsctl --filter="geneve" show > d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 > Bridge br0 > Port tunnel_port > Interface tunnel_port > type: geneve > options: {key="123", remote_ip=flow} > Port tunnel_port2 > Interface tunnel_port2 > type: geneve > options: {key="125", remote_ip=flow} > > > Or if I want to show all tunnels with the tunnel key 123: > > $ ovs-vsctl --filter='key="123"' show > d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 > Bridge br0 > Port tunnel_port > Interface tunnel_port > type: geneve > options: {key="123", remote_ip=flow} > > > Or I want the previous one, but also names of all the ports per bridge: > > $ ovs-vsctl --filter='key="123"|Port' show > d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 > Bridge br1 > Port br1 > Bridge br0 > Port p1 > Port tunnel_port > Interface tunnel_port > type: geneve > options: {key="123", remote_ip=flow} > Port tunnel_port2 > Port br0 > > > What do you think? > >> } >> >> ovs_assert(sset_is_empty(&shown)); >> sset_destroy(&shown); >> + free_filters(filter); >> } >> >> >> @@ -2548,7 +2621,7 @@ ctl_init__(const struct ovsdb_idl_class *idl_class_, >> cmd_show_tables = cmd_show_tables_; >> if (cmd_show_tables) { >> static const struct ctl_command_syntax show = >> - {"show", 0, 0, "", pre_cmd_show, cmd_show, NULL, "", RO}; >> + {"show", 0, 0, "", pre_cmd_show, cmd_show, NULL, "--filter=", RO}; >> ctl_register_command(&show); >> } >> } >> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at >> index e488e292d..010889789 100644 >> --- a/tests/ovs-vsctl.at >> +++ b/tests/ovs-vsctl.at >> @@ -1831,3 +1831,31 @@ AT_CHECK([ovs-vsctl --no-wait --bare --columns _uuid,name list bridge tst1], [0] >> >> OVS_VSCTL_CLEANUP >> AT_CLEANUP >> + >> +AT_SETUP([ovs-vsctl filter option usage]) > > nit: I'd name it 'ovs-vsctl show filtered' or something like that. > >> +AT_KEYWORDS([ovs-vsctl filter option]) > > nit :I'd replace the 'option' with 'show'. 'option' is a not very specific > keyword to use. > >> + >> +OVS_VSWITCHD_START([dnl >> + add-port br0 p1 -- set Interface p1 type=internal ofport_request=1 -- \ >> + add-port br0 p2 -- set Interface p2 type=internal ofport_request=2 -- \ >> + add-port br0 p3 -- set Interface p3 type=internal ofport_request=3 >> +]) >> + >> +AT_CHECK([ovs-vsctl --filter='p1' show | grep Port | sort], [0], [dnl >> + Port br0 >> + Port p1 >> + Port p2 >> + Port p3 >> +]) >> + >> +AT_CHECK([ovs-vsctl --filter='test' show | grep Port | sort], [0], []) >> + >> +AT_CHECK([ovs-vsctl --filter='Bridge' show | grep Port | sort], [0], [dnl >> + Port br0 >> + Port p1 >> + Port p2 >> + Port p3 >> +]) > > We should print the whole thing here. It will not change from run to run, > because database structures are sorted and our top-level table only has one > row. The only thing that will change is the datapath uuid at the very top. > You may just cut out the first line with tail -n +2 or feed the output into > the uuidfilt. Ignore that part, we do not control the UUIDs of referenced rows, so the output order will change between runs, unfortunately. > > Maybe add a bit more variety to the tests. Different interface types. > >> + >> +OVS_VSCTL_CLEANUP >> +AT_CLEANUP >> \ No newline at end of file > > Add the new line here! > > Best regards, Ilya Maximets. > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Ilya, hi! thanks for the comments, I fixed the storage of filters on the sset structure, but I have a question below On 29.05.2025 19:10, Ilya Maximets wrote: > On 5/29/25 6:02 PM, Ilya Maximets wrote: >> On 5/20/25 7:16 PM, Alexandra Rukomoinikova wrote: >>> Added the ability to filter the output of the show command >>> using the filter option. >>> >>> Signed-off-by: Alexandra Rukomoinikova <arukomoinikova@k2.cloud> >>> --- >>> v1 --> v2 : did as Ilya said. >>> --- >>> lib/db-ctl-base.c | 75 +++++++++++++++++++++++++++++++++++++++++++++- >>> tests/ovs-vsctl.at | 28 +++++++++++++++++ >>> 2 files changed, 102 insertions(+), 1 deletion(-) >> Hi, Alexandra. Thanks for the new version! >> >> nit: The 'lib:' in the patch name doesn't make a lot of sense as vast >> majority of OVS code is in lib. It should be 'db-ctl-base:' instead. >> >> Since this patch adds a new option, it needs to be documented in the >> man page (utilities/ovs-vsctl.8.in) and a NEWS entry should be added. >> >> Some more comments below. >> >>> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c >>> index 1f157e46c..45d2d51c0 100644 >>> --- a/lib/db-ctl-base.c >>> +++ b/lib/db-ctl-base.c >>> @@ -2243,19 +2243,92 @@ cmd_show_row(struct ctl_context *ctx, const struct ovsdb_idl_row *row, >>> sset_find_and_delete_assert(shown, show->table->name); >>> } >>> >>> +struct output_filter { >>> + char **filters; >>> + size_t n_filters; >>> +}; >>> + >>> +static void >>> +filter_row(struct ctl_context *ctx, >>> + struct output_filter *filter, >>> + size_t old_lenght) >> s/lenght/length/ >> >> here and in a few other places in the code. >> >>> +{ >>> + bool filter_match = false; >>> + >>> + for (size_t i = 0; i < filter->n_filters; i++) { >>> + if (strstr(&ctx->output.string[old_lenght], filter->filters[i])) { >>> + filter_match = true; >> break here. There is no need to continue the loop. >> >>> + } >>> + } >>> + >>> + if (!filter_match) { >>> + ds_truncate(&ctx->output, old_lenght); >>> + } >>> +} >>> + >>> +static struct output_filter * >>> +init_filters(struct ctl_context *ctx) >>> +{ >>> + char *filter_str = shash_find_data(&ctx->options, "--filter"); >>> + if (!filter_str || !*filter_str) { >>> + return NULL; >>> + } >>> + >>> + size_t count = 1; >>> + for (const char *p = filter_str; *p; p++) { >>> + if (*p == '|') { >>> + count++; >>> + } >>> + } >>> + >>> + struct output_filter *filter = xmalloc(sizeof(struct output_filter)); >>> + filter->filters = xmalloc(count * (sizeof(char *))); >>> + filter->n_filters = count; >>> + >>> + char *ptr = NULL; >>> + char *token = strtok_r(filter_str, "|", &ptr); >>> + size_t idx = 0; >>> + >>> + while (token && idx < count) { >>> + filter->filters[idx++] = xstrdup(token); >>> + token = strtok_r(NULL, "|", &ptr); >>> + } >>> + >>> + return filter; >>> +} >> If we use sset as a data structure for the filters, the code above can >> be mostly replaced with an sset_from_delimited_string() call. >> The sset is slightly less memory-efficient, but I don't think we care >> in this particular case. >> >>> + >>> +static void >>> +free_filters(struct output_filter *filter) >>> +{ >>> + if (!filter) { >>> + return; >>> + } >>> + for (size_t i = 0; i < filter->n_filters; i++) { >>> + free(filter->filters[i]); >>> + } >>> + free(filter->filters); >>> + free(filter); >>> +} >>> + >>> static void >>> cmd_show(struct ctl_context *ctx) >>> { >>> const struct ovsdb_idl_row *row; >>> + struct output_filter *filter = init_filters(ctx); >>> struct sset shown = SSET_INITIALIZER(&shown); >>> >>> for (row = ovsdb_idl_first_row(ctx->idl, cmd_show_tables[0].table); >>> row; row = ovsdb_idl_next_row(row)) { >>> + size_t lenght_before = ctx->output.length; >>> cmd_show_row(ctx, row, 0, &shown); >>> + if (filter) { >>> + filter_row(ctx, filter, lenght_before); >>> + } >> This only covers the top-level elements, but the idea was to perform the >> filtering recursively for every level. You should move the filtering into >> the cmd_show_row() itself, i.e. save the length at the top of cmd_show_row() >> and run the filtering at the bottom. This will ensure that we're filtering >> all the way through. >> >> For example, if we have a setup like this: >> >> $ ovs-vsctl add-br br1 >> $ ovs-vsctl add-br br0 >> $ ovs-vsctl add-port br0 p1 >> $ ovs-vsctl add-port br0 tunnel_port \ >> -- set Interface tunnel_port type=geneve \ >> options:remote_ip=flow options:key=123 >> $ ovs-vsctl add-port br0 tunnel_port2 \ >> -- set Interface tunnel_port2 type=geneve \ >> options:remote_ip=flow options:key=125 >> >> Then I can get a list of bridges only with: >> >> $ ovs-vsctl --filter="Bridge" show >> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 >> Bridge br1 >> Bridge br0 >> >> Since the 'Bridge' match happens at the higher level, all the port >> and interface information is filtered out. >> >> Or a list of ports per bridge: >> >> $ ovs-vsctl --filter="Port" show >> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 >> Bridge br1 >> Port br1 >> Bridge br0 >> Port p1 >> Port tunnel_port >> Port tunnel_port2 >> Port br0 >> >> Here only the interface information is filtered out since once we match >> on the 'Port' string, all the higher-level nodes will match as well. >> >> Or if I want to only show geneve interfaces: >> >> $ ovs-vsctl --filter="geneve" show >> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 >> Bridge br0 >> Port tunnel_port >> Interface tunnel_port >> type: geneve >> options: {key="123", remote_ip=flow} >> Port tunnel_port2 >> Interface tunnel_port2 >> type: geneve >> options: {key="125", remote_ip=flow} >> >> >> Or if I want to show all tunnels with the tunnel key 123: >> >> $ ovs-vsctl --filter='key="123"' show >> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 >> Bridge br0 >> Port tunnel_port >> Interface tunnel_port >> type: geneve >> options: {key="123", remote_ip=flow} >> >> >> Or I want the previous one, but also names of all the ports per bridge: >> >> $ ovs-vsctl --filter='key="123"|Port' show >> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 >> Bridge br1 >> Port br1 >> Bridge br0 >> Port p1 >> Port tunnel_port >> Interface tunnel_port >> type: geneve >> options: {key="123", remote_ip=flow} >> Port tunnel_port2 >> Port br0 >> >> >> What do you think? my idea was in many ways for the sbctl command to be able to filter by chassis, it seems that in your approach this will not be available, and if I filter by chassis name, I will only see its chassis name and will not see all the information about enсap-ips and port bindings. >>> } >>> >>> ovs_assert(sset_is_empty(&shown)); >>> sset_destroy(&shown); >>> + free_filters(filter); >>> } >>> >>> >>> @@ -2548,7 +2621,7 @@ ctl_init__(const struct ovsdb_idl_class *idl_class_, >>> cmd_show_tables = cmd_show_tables_; >>> if (cmd_show_tables) { >>> static const struct ctl_command_syntax show = >>> - {"show", 0, 0, "", pre_cmd_show, cmd_show, NULL, "", RO}; >>> + {"show", 0, 0, "", pre_cmd_show, cmd_show, NULL, "--filter=", RO}; >>> ctl_register_command(&show); >>> } >>> } >>> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at >>> index e488e292d..010889789 100644 >>> --- a/tests/ovs-vsctl.at >>> +++ b/tests/ovs-vsctl.at >>> @@ -1831,3 +1831,31 @@ AT_CHECK([ovs-vsctl --no-wait --bare --columns _uuid,name list bridge tst1], [0] >>> >>> OVS_VSCTL_CLEANUP >>> AT_CLEANUP >>> + >>> +AT_SETUP([ovs-vsctl filter option usage]) >> nit: I'd name it 'ovs-vsctl show filtered' or something like that. >> >>> +AT_KEYWORDS([ovs-vsctl filter option]) >> nit :I'd replace the 'option' with 'show'. 'option' is a not very specific >> keyword to use. >> >>> + >>> +OVS_VSWITCHD_START([dnl >>> + add-port br0 p1 -- set Interface p1 type=internal ofport_request=1 -- \ >>> + add-port br0 p2 -- set Interface p2 type=internal ofport_request=2 -- \ >>> + add-port br0 p3 -- set Interface p3 type=internal ofport_request=3 >>> +]) >>> + >>> +AT_CHECK([ovs-vsctl --filter='p1' show | grep Port | sort], [0], [dnl >>> + Port br0 >>> + Port p1 >>> + Port p2 >>> + Port p3 >>> +]) >>> + >>> +AT_CHECK([ovs-vsctl --filter='test' show | grep Port | sort], [0], []) >>> + >>> +AT_CHECK([ovs-vsctl --filter='Bridge' show | grep Port | sort], [0], [dnl >>> + Port br0 >>> + Port p1 >>> + Port p2 >>> + Port p3 >>> +]) >> We should print the whole thing here. It will not change from run to run, >> because database structures are sorted and our top-level table only has one >> row. The only thing that will change is the datapath uuid at the very top. >> You may just cut out the first line with tail -n +2 or feed the output into >> the uuidfilt. > Ignore that part, we do not control the UUIDs of referenced rows, so the output > order will change between runs, unfortunately. > >> Maybe add a bit more variety to the tests. Different interface types. >> >>> + >>> +OVS_VSCTL_CLEANUP >>> +AT_CLEANUP >>> \ No newline at end of file >> Add the new line here! >> >> Best regards, Ilya Maximets. >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>
On 5/29/25 7:39 PM, Rukomoinikova Aleksandra wrote: > Ilya, hi! thanks for the comments, I fixed the storage of filters on the > sset structure, but I have a question below > > On 29.05.2025 19:10, Ilya Maximets wrote: >> On 5/29/25 6:02 PM, Ilya Maximets wrote: >>> On 5/20/25 7:16 PM, Alexandra Rukomoinikova wrote: >>>> Added the ability to filter the output of the show command >>>> using the filter option. >>>> >>>> Signed-off-by: Alexandra Rukomoinikova <arukomoinikova@k2.cloud> >>>> --- >>>> v1 --> v2 : did as Ilya said. >>>> --- >>>> lib/db-ctl-base.c | 75 +++++++++++++++++++++++++++++++++++++++++++++- >>>> tests/ovs-vsctl.at | 28 +++++++++++++++++ >>>> 2 files changed, 102 insertions(+), 1 deletion(-) >>> Hi, Alexandra. Thanks for the new version! >>> >>> nit: The 'lib:' in the patch name doesn't make a lot of sense as vast >>> majority of OVS code is in lib. It should be 'db-ctl-base:' instead. >>> >>> Since this patch adds a new option, it needs to be documented in the >>> man page (utilities/ovs-vsctl.8.in) and a NEWS entry should be added. >>> >>> Some more comments below. >>> >>>> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c >>>> index 1f157e46c..45d2d51c0 100644 >>>> --- a/lib/db-ctl-base.c >>>> +++ b/lib/db-ctl-base.c >>>> @@ -2243,19 +2243,92 @@ cmd_show_row(struct ctl_context *ctx, const struct ovsdb_idl_row *row, >>>> sset_find_and_delete_assert(shown, show->table->name); >>>> } >>>> >>>> +struct output_filter { >>>> + char **filters; >>>> + size_t n_filters; >>>> +}; >>>> + >>>> +static void >>>> +filter_row(struct ctl_context *ctx, >>>> + struct output_filter *filter, >>>> + size_t old_lenght) >>> s/lenght/length/ >>> >>> here and in a few other places in the code. >>> >>>> +{ >>>> + bool filter_match = false; >>>> + >>>> + for (size_t i = 0; i < filter->n_filters; i++) { >>>> + if (strstr(&ctx->output.string[old_lenght], filter->filters[i])) { >>>> + filter_match = true; >>> break here. There is no need to continue the loop. >>> >>>> + } >>>> + } >>>> + >>>> + if (!filter_match) { >>>> + ds_truncate(&ctx->output, old_lenght); >>>> + } >>>> +} >>>> + >>>> +static struct output_filter * >>>> +init_filters(struct ctl_context *ctx) >>>> +{ >>>> + char *filter_str = shash_find_data(&ctx->options, "--filter"); >>>> + if (!filter_str || !*filter_str) { >>>> + return NULL; >>>> + } >>>> + >>>> + size_t count = 1; >>>> + for (const char *p = filter_str; *p; p++) { >>>> + if (*p == '|') { >>>> + count++; >>>> + } >>>> + } >>>> + >>>> + struct output_filter *filter = xmalloc(sizeof(struct output_filter)); >>>> + filter->filters = xmalloc(count * (sizeof(char *))); >>>> + filter->n_filters = count; >>>> + >>>> + char *ptr = NULL; >>>> + char *token = strtok_r(filter_str, "|", &ptr); >>>> + size_t idx = 0; >>>> + >>>> + while (token && idx < count) { >>>> + filter->filters[idx++] = xstrdup(token); >>>> + token = strtok_r(NULL, "|", &ptr); >>>> + } >>>> + >>>> + return filter; >>>> +} >>> If we use sset as a data structure for the filters, the code above can >>> be mostly replaced with an sset_from_delimited_string() call. >>> The sset is slightly less memory-efficient, but I don't think we care >>> in this particular case. >>> >>>> + >>>> +static void >>>> +free_filters(struct output_filter *filter) >>>> +{ >>>> + if (!filter) { >>>> + return; >>>> + } >>>> + for (size_t i = 0; i < filter->n_filters; i++) { >>>> + free(filter->filters[i]); >>>> + } >>>> + free(filter->filters); >>>> + free(filter); >>>> +} >>>> + >>>> static void >>>> cmd_show(struct ctl_context *ctx) >>>> { >>>> const struct ovsdb_idl_row *row; >>>> + struct output_filter *filter = init_filters(ctx); >>>> struct sset shown = SSET_INITIALIZER(&shown); >>>> >>>> for (row = ovsdb_idl_first_row(ctx->idl, cmd_show_tables[0].table); >>>> row; row = ovsdb_idl_next_row(row)) { >>>> + size_t lenght_before = ctx->output.length; >>>> cmd_show_row(ctx, row, 0, &shown); >>>> + if (filter) { >>>> + filter_row(ctx, filter, lenght_before); >>>> + } >>> This only covers the top-level elements, but the idea was to perform the >>> filtering recursively for every level. You should move the filtering into >>> the cmd_show_row() itself, i.e. save the length at the top of cmd_show_row() >>> and run the filtering at the bottom. This will ensure that we're filtering >>> all the way through. >>> >>> For example, if we have a setup like this: >>> >>> $ ovs-vsctl add-br br1 >>> $ ovs-vsctl add-br br0 >>> $ ovs-vsctl add-port br0 p1 >>> $ ovs-vsctl add-port br0 tunnel_port \ >>> -- set Interface tunnel_port type=geneve \ >>> options:remote_ip=flow options:key=123 >>> $ ovs-vsctl add-port br0 tunnel_port2 \ >>> -- set Interface tunnel_port2 type=geneve \ >>> options:remote_ip=flow options:key=125 >>> >>> Then I can get a list of bridges only with: >>> >>> $ ovs-vsctl --filter="Bridge" show >>> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 >>> Bridge br1 >>> Bridge br0 >>> >>> Since the 'Bridge' match happens at the higher level, all the port >>> and interface information is filtered out. >>> >>> Or a list of ports per bridge: >>> >>> $ ovs-vsctl --filter="Port" show >>> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 >>> Bridge br1 >>> Port br1 >>> Bridge br0 >>> Port p1 >>> Port tunnel_port >>> Port tunnel_port2 >>> Port br0 >>> >>> Here only the interface information is filtered out since once we match >>> on the 'Port' string, all the higher-level nodes will match as well. >>> >>> Or if I want to only show geneve interfaces: >>> >>> $ ovs-vsctl --filter="geneve" show >>> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 >>> Bridge br0 >>> Port tunnel_port >>> Interface tunnel_port >>> type: geneve >>> options: {key="123", remote_ip=flow} >>> Port tunnel_port2 >>> Interface tunnel_port2 >>> type: geneve >>> options: {key="125", remote_ip=flow} >>> >>> >>> Or if I want to show all tunnels with the tunnel key 123: >>> >>> $ ovs-vsctl --filter='key="123"' show >>> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 >>> Bridge br0 >>> Port tunnel_port >>> Interface tunnel_port >>> type: geneve >>> options: {key="123", remote_ip=flow} >>> >>> >>> Or I want the previous one, but also names of all the ports per bridge: >>> >>> $ ovs-vsctl --filter='key="123"|Port' show >>> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 >>> Bridge br1 >>> Port br1 >>> Bridge br0 >>> Port p1 >>> Port tunnel_port >>> Interface tunnel_port >>> type: geneve >>> options: {key="123", remote_ip=flow} >>> Port tunnel_port2 >>> Port br0 >>> >>> >>> What do you think? > > my idea was in many ways for the sbctl command to be able to filter by > chassis, it seems that in your approach this will not be available, and > if I filter by chassis name, I will only see its chassis name and will > not see all the information about enсap-ips and port bindings. Hmm, good point. The recursive logic works nicely for the OVS schema, but not so much for the OVN_Southbound. So, we probably want is an option to apply filters at the level of specific tables. I spent some time thinking what would be a good syntax for this that would not limit the filtering options too much, and I came up with something like this: --filter=TABLE(FILTER[|FILTER...]),... So, for filtering on chassis names, but keeping all the encap and the port binding informations, it will look like: ovn-sbctl --filter="chassis(ch0|ch1)" show This will mean that we only apply this filter in cmd_show_row() when the current show->table is "chassis". If we'll want to only show specific port bindings in the found chassis, then we'll be able to do something like this: ovn-sbctl --filter="chassis(ch0|ch1),port-binding(sw0-p)" show This will still print all the encap-ips for all the found chassis, but will only print port bindings that contain 'sw0-p' substring. And we should still allow the simple case without specifying the table that should do the filtering on all levels, e.g. ovs-vsctl --filter='Port' show That will still only print port names inside all the bridges. And something like this: ovs-vsctl --filter='bridge(key=123)' show would print every port and interface of a bridge if at least one of the interfaces in it is a tunnel interface with a key=123. So, the solution would look very similar, but parsing will be a bit more involved... First we'll need to split by a comma, then parse each chunk by finding the first parenthesis and treating everything inside as a list of filters for the table. As a data structure to store all that, may use an shash with sset as a value, I guess. If the name of a current table is not in the shash, then use the filter that has no table specified. What do you think? >>>> } >>>> >>>> ovs_assert(sset_is_empty(&shown)); >>>> sset_destroy(&shown); >>>> + free_filters(filter); >>>> } >>>> >>>> >>>> @@ -2548,7 +2621,7 @@ ctl_init__(const struct ovsdb_idl_class *idl_class_, >>>> cmd_show_tables = cmd_show_tables_; >>>> if (cmd_show_tables) { >>>> static const struct ctl_command_syntax show = >>>> - {"show", 0, 0, "", pre_cmd_show, cmd_show, NULL, "", RO}; >>>> + {"show", 0, 0, "", pre_cmd_show, cmd_show, NULL, "--filter=", RO}; >>>> ctl_register_command(&show); >>>> } >>>> } >>>> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at >>>> index e488e292d..010889789 100644 >>>> --- a/tests/ovs-vsctl.at >>>> +++ b/tests/ovs-vsctl.at >>>> @@ -1831,3 +1831,31 @@ AT_CHECK([ovs-vsctl --no-wait --bare --columns _uuid,name list bridge tst1], [0] >>>> >>>> OVS_VSCTL_CLEANUP >>>> AT_CLEANUP >>>> + >>>> +AT_SETUP([ovs-vsctl filter option usage]) >>> nit: I'd name it 'ovs-vsctl show filtered' or something like that. >>> >>>> +AT_KEYWORDS([ovs-vsctl filter option]) >>> nit :I'd replace the 'option' with 'show'. 'option' is a not very specific >>> keyword to use. >>> >>>> + >>>> +OVS_VSWITCHD_START([dnl >>>> + add-port br0 p1 -- set Interface p1 type=internal ofport_request=1 -- \ >>>> + add-port br0 p2 -- set Interface p2 type=internal ofport_request=2 -- \ >>>> + add-port br0 p3 -- set Interface p3 type=internal ofport_request=3 >>>> +]) >>>> + >>>> +AT_CHECK([ovs-vsctl --filter='p1' show | grep Port | sort], [0], [dnl >>>> + Port br0 >>>> + Port p1 >>>> + Port p2 >>>> + Port p3 >>>> +]) >>>> + >>>> +AT_CHECK([ovs-vsctl --filter='test' show | grep Port | sort], [0], []) >>>> + >>>> +AT_CHECK([ovs-vsctl --filter='Bridge' show | grep Port | sort], [0], [dnl >>>> + Port br0 >>>> + Port p1 >>>> + Port p2 >>>> + Port p3 >>>> +]) >>> We should print the whole thing here. It will not change from run to run, >>> because database structures are sorted and our top-level table only has one >>> row. The only thing that will change is the datapath uuid at the very top. >>> You may just cut out the first line with tail -n +2 or feed the output into >>> the uuidfilt. >> Ignore that part, we do not control the UUIDs of referenced rows, so the output >> order will change between runs, unfortunately. >> >>> Maybe add a bit more variety to the tests. Different interface types. >>> >>>> + >>>> +OVS_VSCTL_CLEANUP >>>> +AT_CLEANUP >>>> \ No newline at end of file >>> Add the new line here! >>> >>> Best regards, Ilya Maximets. >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >
On 29.05.2025 22:21, Ilya Maximets wrote: > On 5/29/25 7:39 PM, Rukomoinikova Aleksandra wrote: >> Ilya, hi! thanks for the comments, I fixed the storage of filters on the >> sset structure, but I have a question below >> >> On 29.05.2025 19:10, Ilya Maximets wrote: >>> On 5/29/25 6:02 PM, Ilya Maximets wrote: >>>> On 5/20/25 7:16 PM, Alexandra Rukomoinikova wrote: >>>>> Added the ability to filter the output of the show command >>>>> using the filter option. >>>>> >>>>> Signed-off-by: Alexandra Rukomoinikova <arukomoinikova@k2.cloud> >>>>> --- >>>>> v1 --> v2 : did as Ilya said. >>>>> --- >>>>> lib/db-ctl-base.c | 75 +++++++++++++++++++++++++++++++++++++++++++++- >>>>> tests/ovs-vsctl.at | 28 +++++++++++++++++ >>>>> 2 files changed, 102 insertions(+), 1 deletion(-) >>>> Hi, Alexandra. Thanks for the new version! >>>> >>>> nit: The 'lib:' in the patch name doesn't make a lot of sense as vast >>>> majority of OVS code is in lib. It should be 'db-ctl-base:' instead. >>>> >>>> Since this patch adds a new option, it needs to be documented in the >>>> man page (utilities/ovs-vsctl.8.in) and a NEWS entry should be added. >>>> >>>> Some more comments below. >>>> >>>>> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c >>>>> index 1f157e46c..45d2d51c0 100644 >>>>> --- a/lib/db-ctl-base.c >>>>> +++ b/lib/db-ctl-base.c >>>>> @@ -2243,19 +2243,92 @@ cmd_show_row(struct ctl_context *ctx, const struct ovsdb_idl_row *row, >>>>> sset_find_and_delete_assert(shown, show->table->name); >>>>> } >>>>> >>>>> +struct output_filter { >>>>> + char **filters; >>>>> + size_t n_filters; >>>>> +}; >>>>> + >>>>> +static void >>>>> +filter_row(struct ctl_context *ctx, >>>>> + struct output_filter *filter, >>>>> + size_t old_lenght) >>>> s/lenght/length/ >>>> >>>> here and in a few other places in the code. >>>> >>>>> +{ >>>>> + bool filter_match = false; >>>>> + >>>>> + for (size_t i = 0; i < filter->n_filters; i++) { >>>>> + if (strstr(&ctx->output.string[old_lenght], filter->filters[i])) { >>>>> + filter_match = true; >>>> break here. There is no need to continue the loop. >>>> >>>>> + } >>>>> + } >>>>> + >>>>> + if (!filter_match) { >>>>> + ds_truncate(&ctx->output, old_lenght); >>>>> + } >>>>> +} >>>>> + >>>>> +static struct output_filter * >>>>> +init_filters(struct ctl_context *ctx) >>>>> +{ >>>>> + char *filter_str = shash_find_data(&ctx->options, "--filter"); >>>>> + if (!filter_str || !*filter_str) { >>>>> + return NULL; >>>>> + } >>>>> + >>>>> + size_t count = 1; >>>>> + for (const char *p = filter_str; *p; p++) { >>>>> + if (*p == '|') { >>>>> + count++; >>>>> + } >>>>> + } >>>>> + >>>>> + struct output_filter *filter = xmalloc(sizeof(struct output_filter)); >>>>> + filter->filters = xmalloc(count * (sizeof(char *))); >>>>> + filter->n_filters = count; >>>>> + >>>>> + char *ptr = NULL; >>>>> + char *token = strtok_r(filter_str, "|", &ptr); >>>>> + size_t idx = 0; >>>>> + >>>>> + while (token && idx < count) { >>>>> + filter->filters[idx++] = xstrdup(token); >>>>> + token = strtok_r(NULL, "|", &ptr); >>>>> + } >>>>> + >>>>> + return filter; >>>>> +} >>>> If we use sset as a data structure for the filters, the code above can >>>> be mostly replaced with an sset_from_delimited_string() call. >>>> The sset is slightly less memory-efficient, but I don't think we care >>>> in this particular case. >>>> >>>>> + >>>>> +static void >>>>> +free_filters(struct output_filter *filter) >>>>> +{ >>>>> + if (!filter) { >>>>> + return; >>>>> + } >>>>> + for (size_t i = 0; i < filter->n_filters; i++) { >>>>> + free(filter->filters[i]); >>>>> + } >>>>> + free(filter->filters); >>>>> + free(filter); >>>>> +} >>>>> + >>>>> static void >>>>> cmd_show(struct ctl_context *ctx) >>>>> { >>>>> const struct ovsdb_idl_row *row; >>>>> + struct output_filter *filter = init_filters(ctx); >>>>> struct sset shown = SSET_INITIALIZER(&shown); >>>>> >>>>> for (row = ovsdb_idl_first_row(ctx->idl, cmd_show_tables[0].table); >>>>> row; row = ovsdb_idl_next_row(row)) { >>>>> + size_t lenght_before = ctx->output.length; >>>>> cmd_show_row(ctx, row, 0, &shown); >>>>> + if (filter) { >>>>> + filter_row(ctx, filter, lenght_before); >>>>> + } >>>> This only covers the top-level elements, but the idea was to perform the >>>> filtering recursively for every level. You should move the filtering into >>>> the cmd_show_row() itself, i.e. save the length at the top of cmd_show_row() >>>> and run the filtering at the bottom. This will ensure that we're filtering >>>> all the way through. >>>> >>>> For example, if we have a setup like this: >>>> >>>> $ ovs-vsctl add-br br1 >>>> $ ovs-vsctl add-br br0 >>>> $ ovs-vsctl add-port br0 p1 >>>> $ ovs-vsctl add-port br0 tunnel_port \ >>>> -- set Interface tunnel_port type=geneve \ >>>> options:remote_ip=flow options:key=123 >>>> $ ovs-vsctl add-port br0 tunnel_port2 \ >>>> -- set Interface tunnel_port2 type=geneve \ >>>> options:remote_ip=flow options:key=125 >>>> >>>> Then I can get a list of bridges only with: >>>> >>>> $ ovs-vsctl --filter="Bridge" show >>>> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 >>>> Bridge br1 >>>> Bridge br0 >>>> >>>> Since the 'Bridge' match happens at the higher level, all the port >>>> and interface information is filtered out. >>>> >>>> Or a list of ports per bridge: >>>> >>>> $ ovs-vsctl --filter="Port" show >>>> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 >>>> Bridge br1 >>>> Port br1 >>>> Bridge br0 >>>> Port p1 >>>> Port tunnel_port >>>> Port tunnel_port2 >>>> Port br0 >>>> >>>> Here only the interface information is filtered out since once we match >>>> on the 'Port' string, all the higher-level nodes will match as well. >>>> >>>> Or if I want to only show geneve interfaces: >>>> >>>> $ ovs-vsctl --filter="geneve" show >>>> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 >>>> Bridge br0 >>>> Port tunnel_port >>>> Interface tunnel_port >>>> type: geneve >>>> options: {key="123", remote_ip=flow} >>>> Port tunnel_port2 >>>> Interface tunnel_port2 >>>> type: geneve >>>> options: {key="125", remote_ip=flow} >>>> >>>> >>>> Or if I want to show all tunnels with the tunnel key 123: >>>> >>>> $ ovs-vsctl --filter='key="123"' show >>>> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 >>>> Bridge br0 >>>> Port tunnel_port >>>> Interface tunnel_port >>>> type: geneve >>>> options: {key="123", remote_ip=flow} >>>> >>>> >>>> Or I want the previous one, but also names of all the ports per bridge: >>>> >>>> $ ovs-vsctl --filter='key="123"|Port' show >>>> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 >>>> Bridge br1 >>>> Port br1 >>>> Bridge br0 >>>> Port p1 >>>> Port tunnel_port >>>> Interface tunnel_port >>>> type: geneve >>>> options: {key="123", remote_ip=flow} >>>> Port tunnel_port2 >>>> Port br0 >>>> >>>> >>>> What do you think? >> my idea was in many ways for the sbctl command to be able to filter by >> chassis, it seems that in your approach this will not be available, and >> if I filter by chassis name, I will only see its chassis name and will >> not see all the information about enсap-ips and port bindings. > Hmm, good point. The recursive logic works nicely for the OVS schema, > but not so much for the OVN_Southbound. So, we probably want is an > option to apply filters at the level of specific tables. I spent some > time thinking what would be a good syntax for this that would not limit > the filtering options too much, and I came up with something like this: > > --filter=TABLE(FILTER[|FILTER...]),... > > So, for filtering on chassis names, but keeping all the encap and the > port binding informations, it will look like: > > ovn-sbctl --filter="chassis(ch0|ch1)" show > > This will mean that we only apply this filter in cmd_show_row() when > the current show->table is "chassis". > > If we'll want to only show specific port bindings in the found chassis, > then we'll be able to do something like this: > > ovn-sbctl --filter="chassis(ch0|ch1),port-binding(sw0-p)" show > > This will still print all the encap-ips for all the found chassis, but > will only print port bindings that contain 'sw0-p' substring. > > And we should still allow the simple case without specifying the table > that should do the filtering on all levels, e.g. > > ovs-vsctl --filter='Port' show > > That will still only print port names inside all the bridges. > And something like this: > > ovs-vsctl --filter='bridge(key=123)' show > > would print every port and interface of a bridge if at least one of the > interfaces in it is a tunnel interface with a key=123. > > So, the solution would look very similar, but parsing will be a bit more > involved... First we'll need to split by a comma, then parse each chunk > by finding the first parenthesis and treating everything inside as a > list of filters for the table. As a data structure to store all that, > may use an shash with sset as a value, I guess. If the name of a current > table is not in the shash, then use the filter that has no table specified. > > What do you think? I think this approach is not very user-friendly, I would honestly get tired of writing this in the show command=) maybe I'll make 2 flags? just a filter that will implement your approach, and the second filter-all (or filter-table, but i like filter-all more), when we can filter the table completely ? >>>>> } >>>>> >>>>> ovs_assert(sset_is_empty(&shown)); >>>>> sset_destroy(&shown); >>>>> + free_filters(filter); >>>>> } >>>>> >>>>> >>>>> @@ -2548,7 +2621,7 @@ ctl_init__(const struct ovsdb_idl_class *idl_class_, >>>>> cmd_show_tables = cmd_show_tables_; >>>>> if (cmd_show_tables) { >>>>> static const struct ctl_command_syntax show = >>>>> - {"show", 0, 0, "", pre_cmd_show, cmd_show, NULL, "", RO}; >>>>> + {"show", 0, 0, "", pre_cmd_show, cmd_show, NULL, "--filter=", RO}; >>>>> ctl_register_command(&show); >>>>> } >>>>> } >>>>> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at >>>>> index e488e292d..010889789 100644 >>>>> --- a/tests/ovs-vsctl.at >>>>> +++ b/tests/ovs-vsctl.at >>>>> @@ -1831,3 +1831,31 @@ AT_CHECK([ovs-vsctl --no-wait --bare --columns _uuid,name list bridge tst1], [0] >>>>> >>>>> OVS_VSCTL_CLEANUP >>>>> AT_CLEANUP >>>>> + >>>>> +AT_SETUP([ovs-vsctl filter option usage]) >>>> nit: I'd name it 'ovs-vsctl show filtered' or something like that. >>>> >>>>> +AT_KEYWORDS([ovs-vsctl filter option]) >>>> nit :I'd replace the 'option' with 'show'. 'option' is a not very specific >>>> keyword to use. >>>> >>>>> + >>>>> +OVS_VSWITCHD_START([dnl >>>>> + add-port br0 p1 -- set Interface p1 type=internal ofport_request=1 -- \ >>>>> + add-port br0 p2 -- set Interface p2 type=internal ofport_request=2 -- \ >>>>> + add-port br0 p3 -- set Interface p3 type=internal ofport_request=3 >>>>> +]) >>>>> + >>>>> +AT_CHECK([ovs-vsctl --filter='p1' show | grep Port | sort], [0], [dnl >>>>> + Port br0 >>>>> + Port p1 >>>>> + Port p2 >>>>> + Port p3 >>>>> +]) >>>>> + >>>>> +AT_CHECK([ovs-vsctl --filter='test' show | grep Port | sort], [0], []) >>>>> + >>>>> +AT_CHECK([ovs-vsctl --filter='Bridge' show | grep Port | sort], [0], [dnl >>>>> + Port br0 >>>>> + Port p1 >>>>> + Port p2 >>>>> + Port p3 >>>>> +]) >>>> We should print the whole thing here. It will not change from run to run, >>>> because database structures are sorted and our top-level table only has one >>>> row. The only thing that will change is the datapath uuid at the very top. >>>> You may just cut out the first line with tail -n +2 or feed the output into >>>> the uuidfilt. >>> Ignore that part, we do not control the UUIDs of referenced rows, so the output >>> order will change between runs, unfortunately. >>> >>>> Maybe add a bit more variety to the tests. Different interface types. >>>> >>>>> + >>>>> +OVS_VSCTL_CLEANUP >>>>> +AT_CLEANUP >>>>> \ No newline at end of file >>>> Add the new line here! >>>> >>>> Best regards, Ilya Maximets. >>>> _______________________________________________ >>>> dev mailing list >>>> dev@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>>
On 5/29/25 10:03 PM, Rukomoinikova Aleksandra wrote: > On 29.05.2025 22:21, Ilya Maximets wrote: >> On 5/29/25 7:39 PM, Rukomoinikova Aleksandra wrote: >>> Ilya, hi! thanks for the comments, I fixed the storage of filters on the >>> sset structure, but I have a question below >>> >>> On 29.05.2025 19:10, Ilya Maximets wrote: >>>> On 5/29/25 6:02 PM, Ilya Maximets wrote: >>>>> On 5/20/25 7:16 PM, Alexandra Rukomoinikova wrote: >>>>>> Added the ability to filter the output of the show command >>>>>> using the filter option. >>>>>> >>>>>> Signed-off-by: Alexandra Rukomoinikova <arukomoinikova@k2.cloud> >>>>>> --- >>>>>> v1 --> v2 : did as Ilya said. >>>>>> --- >>>>>> lib/db-ctl-base.c | 75 +++++++++++++++++++++++++++++++++++++++++++++- >>>>>> tests/ovs-vsctl.at | 28 +++++++++++++++++ >>>>>> 2 files changed, 102 insertions(+), 1 deletion(-) >>>>> Hi, Alexandra. Thanks for the new version! >>>>> >>>>> nit: The 'lib:' in the patch name doesn't make a lot of sense as vast >>>>> majority of OVS code is in lib. It should be 'db-ctl-base:' instead. >>>>> >>>>> Since this patch adds a new option, it needs to be documented in the >>>>> man page (utilities/ovs-vsctl.8.in) and a NEWS entry should be added. >>>>> >>>>> Some more comments below. >>>>> >>>>>> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c >>>>>> index 1f157e46c..45d2d51c0 100644 >>>>>> --- a/lib/db-ctl-base.c >>>>>> +++ b/lib/db-ctl-base.c >>>>>> @@ -2243,19 +2243,92 @@ cmd_show_row(struct ctl_context *ctx, const struct ovsdb_idl_row *row, >>>>>> sset_find_and_delete_assert(shown, show->table->name); >>>>>> } >>>>>> >>>>>> +struct output_filter { >>>>>> + char **filters; >>>>>> + size_t n_filters; >>>>>> +}; >>>>>> + >>>>>> +static void >>>>>> +filter_row(struct ctl_context *ctx, >>>>>> + struct output_filter *filter, >>>>>> + size_t old_lenght) >>>>> s/lenght/length/ >>>>> >>>>> here and in a few other places in the code. >>>>> >>>>>> +{ >>>>>> + bool filter_match = false; >>>>>> + >>>>>> + for (size_t i = 0; i < filter->n_filters; i++) { >>>>>> + if (strstr(&ctx->output.string[old_lenght], filter->filters[i])) { >>>>>> + filter_match = true; >>>>> break here. There is no need to continue the loop. >>>>> >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + if (!filter_match) { >>>>>> + ds_truncate(&ctx->output, old_lenght); >>>>>> + } >>>>>> +} >>>>>> + >>>>>> +static struct output_filter * >>>>>> +init_filters(struct ctl_context *ctx) >>>>>> +{ >>>>>> + char *filter_str = shash_find_data(&ctx->options, "--filter"); >>>>>> + if (!filter_str || !*filter_str) { >>>>>> + return NULL; >>>>>> + } >>>>>> + >>>>>> + size_t count = 1; >>>>>> + for (const char *p = filter_str; *p; p++) { >>>>>> + if (*p == '|') { >>>>>> + count++; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + struct output_filter *filter = xmalloc(sizeof(struct output_filter)); >>>>>> + filter->filters = xmalloc(count * (sizeof(char *))); >>>>>> + filter->n_filters = count; >>>>>> + >>>>>> + char *ptr = NULL; >>>>>> + char *token = strtok_r(filter_str, "|", &ptr); >>>>>> + size_t idx = 0; >>>>>> + >>>>>> + while (token && idx < count) { >>>>>> + filter->filters[idx++] = xstrdup(token); >>>>>> + token = strtok_r(NULL, "|", &ptr); >>>>>> + } >>>>>> + >>>>>> + return filter; >>>>>> +} >>>>> If we use sset as a data structure for the filters, the code above can >>>>> be mostly replaced with an sset_from_delimited_string() call. >>>>> The sset is slightly less memory-efficient, but I don't think we care >>>>> in this particular case. >>>>> >>>>>> + >>>>>> +static void >>>>>> +free_filters(struct output_filter *filter) >>>>>> +{ >>>>>> + if (!filter) { >>>>>> + return; >>>>>> + } >>>>>> + for (size_t i = 0; i < filter->n_filters; i++) { >>>>>> + free(filter->filters[i]); >>>>>> + } >>>>>> + free(filter->filters); >>>>>> + free(filter); >>>>>> +} >>>>>> + >>>>>> static void >>>>>> cmd_show(struct ctl_context *ctx) >>>>>> { >>>>>> const struct ovsdb_idl_row *row; >>>>>> + struct output_filter *filter = init_filters(ctx); >>>>>> struct sset shown = SSET_INITIALIZER(&shown); >>>>>> >>>>>> for (row = ovsdb_idl_first_row(ctx->idl, cmd_show_tables[0].table); >>>>>> row; row = ovsdb_idl_next_row(row)) { >>>>>> + size_t lenght_before = ctx->output.length; >>>>>> cmd_show_row(ctx, row, 0, &shown); >>>>>> + if (filter) { >>>>>> + filter_row(ctx, filter, lenght_before); >>>>>> + } >>>>> This only covers the top-level elements, but the idea was to perform the >>>>> filtering recursively for every level. You should move the filtering into >>>>> the cmd_show_row() itself, i.e. save the length at the top of cmd_show_row() >>>>> and run the filtering at the bottom. This will ensure that we're filtering >>>>> all the way through. >>>>> >>>>> For example, if we have a setup like this: >>>>> >>>>> $ ovs-vsctl add-br br1 >>>>> $ ovs-vsctl add-br br0 >>>>> $ ovs-vsctl add-port br0 p1 >>>>> $ ovs-vsctl add-port br0 tunnel_port \ >>>>> -- set Interface tunnel_port type=geneve \ >>>>> options:remote_ip=flow options:key=123 >>>>> $ ovs-vsctl add-port br0 tunnel_port2 \ >>>>> -- set Interface tunnel_port2 type=geneve \ >>>>> options:remote_ip=flow options:key=125 >>>>> >>>>> Then I can get a list of bridges only with: >>>>> >>>>> $ ovs-vsctl --filter="Bridge" show >>>>> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 >>>>> Bridge br1 >>>>> Bridge br0 >>>>> >>>>> Since the 'Bridge' match happens at the higher level, all the port >>>>> and interface information is filtered out. >>>>> >>>>> Or a list of ports per bridge: >>>>> >>>>> $ ovs-vsctl --filter="Port" show >>>>> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 >>>>> Bridge br1 >>>>> Port br1 >>>>> Bridge br0 >>>>> Port p1 >>>>> Port tunnel_port >>>>> Port tunnel_port2 >>>>> Port br0 >>>>> >>>>> Here only the interface information is filtered out since once we match >>>>> on the 'Port' string, all the higher-level nodes will match as well. >>>>> >>>>> Or if I want to only show geneve interfaces: >>>>> >>>>> $ ovs-vsctl --filter="geneve" show >>>>> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 >>>>> Bridge br0 >>>>> Port tunnel_port >>>>> Interface tunnel_port >>>>> type: geneve >>>>> options: {key="123", remote_ip=flow} >>>>> Port tunnel_port2 >>>>> Interface tunnel_port2 >>>>> type: geneve >>>>> options: {key="125", remote_ip=flow} >>>>> >>>>> >>>>> Or if I want to show all tunnels with the tunnel key 123: >>>>> >>>>> $ ovs-vsctl --filter='key="123"' show >>>>> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 >>>>> Bridge br0 >>>>> Port tunnel_port >>>>> Interface tunnel_port >>>>> type: geneve >>>>> options: {key="123", remote_ip=flow} >>>>> >>>>> >>>>> Or I want the previous one, but also names of all the ports per bridge: >>>>> >>>>> $ ovs-vsctl --filter='key="123"|Port' show >>>>> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0 >>>>> Bridge br1 >>>>> Port br1 >>>>> Bridge br0 >>>>> Port p1 >>>>> Port tunnel_port >>>>> Interface tunnel_port >>>>> type: geneve >>>>> options: {key="123", remote_ip=flow} >>>>> Port tunnel_port2 >>>>> Port br0 >>>>> >>>>> >>>>> What do you think? >>> my idea was in many ways for the sbctl command to be able to filter by >>> chassis, it seems that in your approach this will not be available, and >>> if I filter by chassis name, I will only see its chassis name and will >>> not see all the information about enсap-ips and port bindings. >> Hmm, good point. The recursive logic works nicely for the OVS schema, >> but not so much for the OVN_Southbound. So, we probably want is an >> option to apply filters at the level of specific tables. I spent some >> time thinking what would be a good syntax for this that would not limit >> the filtering options too much, and I came up with something like this: >> >> --filter=TABLE(FILTER[|FILTER...]),... >> >> So, for filtering on chassis names, but keeping all the encap and the >> port binding informations, it will look like: >> >> ovn-sbctl --filter="chassis(ch0|ch1)" show >> >> This will mean that we only apply this filter in cmd_show_row() when >> the current show->table is "chassis". >> >> If we'll want to only show specific port bindings in the found chassis, >> then we'll be able to do something like this: >> >> ovn-sbctl --filter="chassis(ch0|ch1),port-binding(sw0-p)" show >> >> This will still print all the encap-ips for all the found chassis, but >> will only print port bindings that contain 'sw0-p' substring. >> >> And we should still allow the simple case without specifying the table >> that should do the filtering on all levels, e.g. >> >> ovs-vsctl --filter='Port' show >> >> That will still only print port names inside all the bridges. >> And something like this: >> >> ovs-vsctl --filter='bridge(key=123)' show >> >> would print every port and interface of a bridge if at least one of the >> interfaces in it is a tunnel interface with a key=123. >> >> So, the solution would look very similar, but parsing will be a bit more >> involved... First we'll need to split by a comma, then parse each chunk >> by finding the first parenthesis and treating everything inside as a >> list of filters for the table. As a data structure to store all that, >> may use an shash with sset as a value, I guess. If the name of a current >> table is not in the shash, then use the filter that has no table specified. >> >> What do you think? > > I think this approach is not very user-friendly, I would honestly get > tired of writing this in the show command=) Fair. :) Also, OVN doesn't make things easy by having a lot of tables with the same prefixes like 'logical-' or 'chassis', defeating the partial match mechanism. > maybe I'll make 2 flags? > just a filter that will implement your approach, and the second > filter-all (or filter-table, but i like filter-all more), when we can > filter the table completely ? A counteroffer: We have just one option, but if the table is not specified, then the filtering happens only at the top level as in your current implementation. But if the table is specified, then the filtering happens at the level of that particular table. i.e. --filter='ch0' in ovn-sbctl will be the same as --filter='chassis(ch0)', but I would still be able to use ovs-vsctl with --filter='interface(geneve)' to filter out only the geneve interfaces or use --filter='port(Port),int(key=123)' to print all the port names, but only print tunnel interfaces with the key=123 in them. Does that sound reasonable? >>>>>> } >>>>>> >>>>>> ovs_assert(sset_is_empty(&shown)); >>>>>> sset_destroy(&shown); >>>>>> + free_filters(filter); >>>>>> } >>>>>> >>>>>> >>>>>> @@ -2548,7 +2621,7 @@ ctl_init__(const struct ovsdb_idl_class *idl_class_, >>>>>> cmd_show_tables = cmd_show_tables_; >>>>>> if (cmd_show_tables) { >>>>>> static const struct ctl_command_syntax show = >>>>>> - {"show", 0, 0, "", pre_cmd_show, cmd_show, NULL, "", RO}; >>>>>> + {"show", 0, 0, "", pre_cmd_show, cmd_show, NULL, "--filter=", RO}; >>>>>> ctl_register_command(&show); >>>>>> } >>>>>> } >>>>>> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at >>>>>> index e488e292d..010889789 100644 >>>>>> --- a/tests/ovs-vsctl.at >>>>>> +++ b/tests/ovs-vsctl.at >>>>>> @@ -1831,3 +1831,31 @@ AT_CHECK([ovs-vsctl --no-wait --bare --columns _uuid,name list bridge tst1], [0] >>>>>> >>>>>> OVS_VSCTL_CLEANUP >>>>>> AT_CLEANUP >>>>>> + >>>>>> +AT_SETUP([ovs-vsctl filter option usage]) >>>>> nit: I'd name it 'ovs-vsctl show filtered' or something like that. >>>>> >>>>>> +AT_KEYWORDS([ovs-vsctl filter option]) >>>>> nit :I'd replace the 'option' with 'show'. 'option' is a not very specific >>>>> keyword to use. >>>>> >>>>>> + >>>>>> +OVS_VSWITCHD_START([dnl >>>>>> + add-port br0 p1 -- set Interface p1 type=internal ofport_request=1 -- \ >>>>>> + add-port br0 p2 -- set Interface p2 type=internal ofport_request=2 -- \ >>>>>> + add-port br0 p3 -- set Interface p3 type=internal ofport_request=3 >>>>>> +]) >>>>>> + >>>>>> +AT_CHECK([ovs-vsctl --filter='p1' show | grep Port | sort], [0], [dnl >>>>>> + Port br0 >>>>>> + Port p1 >>>>>> + Port p2 >>>>>> + Port p3 >>>>>> +]) >>>>>> + >>>>>> +AT_CHECK([ovs-vsctl --filter='test' show | grep Port | sort], [0], []) >>>>>> + >>>>>> +AT_CHECK([ovs-vsctl --filter='Bridge' show | grep Port | sort], [0], [dnl >>>>>> + Port br0 >>>>>> + Port p1 >>>>>> + Port p2 >>>>>> + Port p3 >>>>>> +]) >>>>> We should print the whole thing here. It will not change from run to run, >>>>> because database structures are sorted and our top-level table only has one >>>>> row. The only thing that will change is the datapath uuid at the very top. >>>>> You may just cut out the first line with tail -n +2 or feed the output into >>>>> the uuidfilt. >>>> Ignore that part, we do not control the UUIDs of referenced rows, so the output >>>> order will change between runs, unfortunately. >>>> >>>>> Maybe add a bit more variety to the tests. Different interface types. >>>>> >>>>>> + >>>>>> +OVS_VSCTL_CLEANUP >>>>>> +AT_CLEANUP >>>>>> \ No newline at end of file >>>>> Add the new line here! >>>>> >>>>> Best regards, Ilya Maximets. >>>>> _______________________________________________ >>>>> dev mailing list >>>>> dev@openvswitch.org >>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>>> >
On 29.05.2025 23:48, Ilya Maximets wrote: > A counteroffer: We have just one option, but if the table is not specified, then > the filtering happens only at the top level as in your current implementation. > But if the table is specified, then the filtering happens at the level of that > particular table. i.e. --filter='ch0' in ovn-sbctl will be the same as > --filter='chassis(ch0)', but I would still be able to use ovs-vsctl with > --filter='interface(geneve)' to filter out only the geneve interfaces or use > --filter='port(Port),int(key=123)' to print all the port names, but only print > tunnel interfaces with the key=123 in them. Yeah, okay, that sounds reasonable, thanks. I will send a new version.
On 5/29/25 10:56 PM, Rukomoinikova Aleksandra wrote: > On 29.05.2025 23:48, Ilya Maximets wrote: >> A counteroffer: We have just one option, but if the table is not specified, then >> the filtering happens only at the top level as in your current implementation. >> But if the table is specified, then the filtering happens at the level of that >> particular table. i.e. --filter='ch0' in ovn-sbctl will be the same as >> --filter='chassis(ch0)', but I would still be able to use ovs-vsctl with >> --filter='interface(geneve)' to filter out only the geneve interfaces or use >> --filter='port(Port),int(key=123)' to print all the port names, but only print >> tunnel interfaces with the key=123 in them. I suppose in this schema the 'interface(key=123)' and 'port(Port),int(key=123)' will actually give the same result. Because we'll be filtering interfaces out, but not the ports that hold them. In order to filter out ports that do not have matching interfaces one will need to use 'port(Interface),int(key=123)'. But that's still makes logical sense, so should be fine. We may need to add some examples to the docs. > > Yeah, okay, that sounds reasonable, thanks. I will send a new version. Thanks! Best regards, Ilya Maximets.
On 30.05.2025 00:18, Ilya Maximets wrote: On 5/29/25 10:56 PM, Rukomoinikova Aleksandra wrote: On 29.05.2025 23:48, Ilya Maximets wrote: A counteroffer: We have just one option, but if the table is not specified, then the filtering happens only at the top level as in your current implementation. But if the table is specified, then the filtering happens at the level of that particular table. i.e. --filter='ch0' in ovn-sbctl will be the same as --filter='chassis(ch0)', but I would still be able to use ovs-vsctl with --filter='interface(geneve)' to filter out only the geneve interfaces or use --filter='port(Port),int(key=123)' to print all the port names, but only print tunnel interfaces with the key=123 in them. I suppose in this schema the 'interface(key=123)' and 'port(Port),int(key=123)' will actually give the same result. Because we'll be filtering interfaces out, but not the ports that hold them. In order to filter out ports that do not have matching interfaces one will need to use 'port(Interface),int(key=123)'. But that's still makes logical sense, so should be fine. We may need to add some examples to the docs. Yeah, okay, that sounds reasonable, thanks. I will send a new version. Thanks! Best regards, Ilya Maximets. Ilya, h! I've been implementing your idea, and it seems to me that the implementation does not justify the functionality: it seems that grep will still be more convenient if we want to filter the output of a single table (for example: --filter=interface(geneve|vxlan - I mean it). It seems unjustified to take into account all the options for writing tables in ovn/ovs (case, dash, and so on) for this option. I would prefer to make this functionality a separate option. and in my implementation, I will correct your comments in the original version and implement the output of one row, As in my original idea, what do you think ? -- regards, Alexandra.
Ilya, h! I've been implementing your idea, and it seems to me that the implementation does not justify the functionality: it seems that grep will still be more convenient if we want to filter the output of a single table (for example: --filter=interface(geneve|vxlan - I mean it). It seems unjustified to take into account all the options for writing tables in ovn/ovs (case, dash, and so on) for this option. I would prefer to make this functionality a separate option. and in my implementation, I will correct your comments in the original version and implement the output of one row, As in my original idea, what do you think ? On 30.05.2025 00:18, Ilya Maximets wrote: > On 5/29/25 10:56 PM, Rukomoinikova Aleksandra wrote: >> On 29.05.2025 23:48, Ilya Maximets wrote: >>> A counteroffer: We have just one option, but if the table is not specified, then >>> the filtering happens only at the top level as in your current implementation. >>> But if the table is specified, then the filtering happens at the level of that >>> particular table. i.e. --filter='ch0' in ovn-sbctl will be the same as >>> --filter='chassis(ch0)', but I would still be able to use ovs-vsctl with >>> --filter='interface(geneve)' to filter out only the geneve interfaces or use >>> --filter='port(Port),int(key=123)' to print all the port names, but only print >>> tunnel interfaces with the key=123 in them. > I suppose in this schema the 'interface(key=123)' and 'port(Port),int(key=123)' > will actually give the same result. Because we'll be filtering interfaces out, > but not the ports that hold them. In order to filter out ports that do not have > matching interfaces one will need to use 'port(Interface),int(key=123)'. > But that's still makes logical sense, so should be fine. We may need to add some > examples to the docs. > >> Yeah, okay, that sounds reasonable, thanks. I will send a new version. > Thanks! > > Best regards, Ilya Maximets.
On 6/2/25 12:15 PM, Rukomoinikova Aleksandra wrote: > Ilya, h! I've been implementing your idea, and it seems to me that the > implementation does not justify the functionality: it seems that grep > will still be more convenient if we want to filter the output of a > single table (for example: --filter=interface(geneve|vxlan - I mean it). Grep is not more convenient, because if you want to check other fields of the interface, then you need to add -A/-B to the grep, and then if you also want to add something else in the match of the same grep command, the output becomes messy. > It seems unjustified to take into account all the options for writing > tables in ovn/ovs (case, dash, and so on) for this option. You don't need to manually check for case and dashes. Just call the get_table() and it will give you the matching table pointer taking into account the case, the dashes and the partial matches. Then you may put the actual table->name as a key into hash map. This way it should be possible to easily lookup filters for the show->table->name. > I would prefer to make this functionality a separate option. and in my > implementation, I will correct your comments in the original version and > implement the output of one row, As in my original idea, what do you think ? I'd still prefer it to be the same option. > > > On 30.05.2025 00:18, Ilya Maximets wrote: >> On 5/29/25 10:56 PM, Rukomoinikova Aleksandra wrote: >>> On 29.05.2025 23:48, Ilya Maximets wrote: >>>> A counteroffer: We have just one option, but if the table is not specified, then >>>> the filtering happens only at the top level as in your current implementation. >>>> But if the table is specified, then the filtering happens at the level of that >>>> particular table. i.e. --filter='ch0' in ovn-sbctl will be the same as >>>> --filter='chassis(ch0)', but I would still be able to use ovs-vsctl with >>>> --filter='interface(geneve)' to filter out only the geneve interfaces or use >>>> --filter='port(Port),int(key=123)' to print all the port names, but only print >>>> tunnel interfaces with the key=123 in them. >> I suppose in this schema the 'interface(key=123)' and 'port(Port),int(key=123)' >> will actually give the same result. Because we'll be filtering interfaces out, >> but not the ports that hold them. In order to filter out ports that do not have >> matching interfaces one will need to use 'port(Interface),int(key=123)'. >> But that's still makes logical sense, so should be fine. We may need to add some >> examples to the docs. >> >>> Yeah, okay, that sounds reasonable, thanks. I will send a new version. >> Thanks! >> >> Best regards, Ilya Maximets. > >
diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c index 1f157e46c..45d2d51c0 100644 --- a/lib/db-ctl-base.c +++ b/lib/db-ctl-base.c @@ -2243,19 +2243,92 @@ cmd_show_row(struct ctl_context *ctx, const struct ovsdb_idl_row *row, sset_find_and_delete_assert(shown, show->table->name); } +struct output_filter { + char **filters; + size_t n_filters; +}; + +static void +filter_row(struct ctl_context *ctx, + struct output_filter *filter, + size_t old_lenght) +{ + bool filter_match = false; + + for (size_t i = 0; i < filter->n_filters; i++) { + if (strstr(&ctx->output.string[old_lenght], filter->filters[i])) { + filter_match = true; + } + } + + if (!filter_match) { + ds_truncate(&ctx->output, old_lenght); + } +} + +static struct output_filter * +init_filters(struct ctl_context *ctx) +{ + char *filter_str = shash_find_data(&ctx->options, "--filter"); + if (!filter_str || !*filter_str) { + return NULL; + } + + size_t count = 1; + for (const char *p = filter_str; *p; p++) { + if (*p == '|') { + count++; + } + } + + struct output_filter *filter = xmalloc(sizeof(struct output_filter)); + filter->filters = xmalloc(count * (sizeof(char *))); + filter->n_filters = count; + + char *ptr = NULL; + char *token = strtok_r(filter_str, "|", &ptr); + size_t idx = 0; + + while (token && idx < count) { + filter->filters[idx++] = xstrdup(token); + token = strtok_r(NULL, "|", &ptr); + } + + return filter; +} + +static void +free_filters(struct output_filter *filter) +{ + if (!filter) { + return; + } + for (size_t i = 0; i < filter->n_filters; i++) { + free(filter->filters[i]); + } + free(filter->filters); + free(filter); +} + static void cmd_show(struct ctl_context *ctx) { const struct ovsdb_idl_row *row; + struct output_filter *filter = init_filters(ctx); struct sset shown = SSET_INITIALIZER(&shown); for (row = ovsdb_idl_first_row(ctx->idl, cmd_show_tables[0].table); row; row = ovsdb_idl_next_row(row)) { + size_t lenght_before = ctx->output.length; cmd_show_row(ctx, row, 0, &shown); + if (filter) { + filter_row(ctx, filter, lenght_before); + } } ovs_assert(sset_is_empty(&shown)); sset_destroy(&shown); + free_filters(filter); } @@ -2548,7 +2621,7 @@ ctl_init__(const struct ovsdb_idl_class *idl_class_, cmd_show_tables = cmd_show_tables_; if (cmd_show_tables) { static const struct ctl_command_syntax show = - {"show", 0, 0, "", pre_cmd_show, cmd_show, NULL, "", RO}; + {"show", 0, 0, "", pre_cmd_show, cmd_show, NULL, "--filter=", RO}; ctl_register_command(&show); } } diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at index e488e292d..010889789 100644 --- a/tests/ovs-vsctl.at +++ b/tests/ovs-vsctl.at @@ -1831,3 +1831,31 @@ AT_CHECK([ovs-vsctl --no-wait --bare --columns _uuid,name list bridge tst1], [0] OVS_VSCTL_CLEANUP AT_CLEANUP + +AT_SETUP([ovs-vsctl filter option usage]) +AT_KEYWORDS([ovs-vsctl filter option]) + +OVS_VSWITCHD_START([dnl + add-port br0 p1 -- set Interface p1 type=internal ofport_request=1 -- \ + add-port br0 p2 -- set Interface p2 type=internal ofport_request=2 -- \ + add-port br0 p3 -- set Interface p3 type=internal ofport_request=3 +]) + +AT_CHECK([ovs-vsctl --filter='p1' show | grep Port | sort], [0], [dnl + Port br0 + Port p1 + Port p2 + Port p3 +]) + +AT_CHECK([ovs-vsctl --filter='test' show | grep Port | sort], [0], []) + +AT_CHECK([ovs-vsctl --filter='Bridge' show | grep Port | sort], [0], [dnl + Port br0 + Port p1 + Port p2 + Port p3 +]) + +OVS_VSCTL_CLEANUP +AT_CLEANUP \ No newline at end of file
Added the ability to filter the output of the show command using the filter option. Signed-off-by: Alexandra Rukomoinikova <arukomoinikova@k2.cloud> --- v1 --> v2 : did as Ilya said. --- lib/db-ctl-base.c | 75 +++++++++++++++++++++++++++++++++++++++++++++- tests/ovs-vsctl.at | 28 +++++++++++++++++ 2 files changed, 102 insertions(+), 1 deletion(-)