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