diff mbox series

[ovs-dev,v2] lib: Added filter option in show command.

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

Checks

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

Commit Message

Rukomoinikova Aleksandra May 20, 2025, 5:16 p.m. UTC
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(-)

Comments

Ilya Maximets May 29, 2025, 4:02 p.m. UTC | #1
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.
Ilya Maximets May 29, 2025, 4:10 p.m. UTC | #2
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
>
Rukomoinikova Aleksandra May 29, 2025, 5:39 p.m. UTC | #3
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
>>
Ilya Maximets May 29, 2025, 7:21 p.m. UTC | #4
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
>>>
>
Rukomoinikova Aleksandra May 29, 2025, 8:03 p.m. UTC | #5
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
>>>>
Ilya Maximets May 29, 2025, 8:48 p.m. UTC | #6
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
>>>>>
>
Rukomoinikova Aleksandra May 29, 2025, 8:56 p.m. UTC | #7
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.
Ilya Maximets May 29, 2025, 9:18 p.m. UTC | #8
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.
Rukomoinikova Aleksandra June 2, 2025, 9:56 a.m. UTC | #9
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.
Rukomoinikova Aleksandra June 2, 2025, 10:15 a.m. UTC | #10
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.
Ilya Maximets June 2, 2025, 10:47 a.m. UTC | #11
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 mbox series

Patch

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