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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
