Hi, Alexandra. Thanks for the update!
This version mostly works as expected. See some comments below.
Note: I'll be creating a new branch for OVS 3.6 somewhere at the
end of Wednesday, so it would be great if you can send v6 on Tuesday
or in the first half of Wednesday. Otherwise it may slip to the
next release cycle.
Best regards, Ilya Maximets.
On 7/11/25 11:24 AM, Alexandra Rukomoinikova wrote:
> The --filter option allows filtering output in two ways:
> 1. Basic filtering (comma-separated list, e.g., 'filter1,filter2').
> 2. Recursive filtering by table (e.g., 'interface(geneve)').
The commit message may need some updating according to the docs change.
>
> Signed-off-by: Alexandra Rukomoinikova <arukomoinikova@k2.cloud>
> ---
> v4 --> v5: fixed comments.
> ---
> NEWS | 2 +
> lib/db-ctl-base.c | 135 +++++++++++++++++++++++++++++++++++++--
> tests/ovs-vsctl.at | 48 ++++++++++++++
> utilities/ovs-vsctl.8.in | 7 ++
> 4 files changed, 188 insertions(+), 4 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index d7231fabc..250ac204e 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -15,6 +15,8 @@ Post-v3.5.0
> core file size.
> - ovs-appctl:
> * Added JSON output support to the 'ovs/route/show' command.
> + - ovs-vsctl:
nit: indentation is a bit off here.
> + * Added '--filter' option to the 'show' command.
> - SSL/TLS:
> * Support for deprecated TLSv1 and TLSv1.1 protocols on OpenFlow and
> database connections is now removed.
> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
> index 5b741b1d4..9537fd115 100644
> --- a/lib/db-ctl-base.c
> +++ b/lib/db-ctl-base.c
> @@ -2141,14 +2141,42 @@ cmd_show_weak_ref(struct ctl_context *ctx, const
> struct cmd_show_table *show,
> }
> }
>
> +static void
> +table_filter(struct ctl_context *ctx,
> + const struct cmd_show_table *show,
> + const struct shash *filters, size_t base_length)
> +{
> + if (shash_is_empty(filters)) {
> + return;
> + }
> +
> + const char *table_name =
> + show && show->table ? show->table->name : NULL;
nit: we may add the !show || !show->table to the 'if' above and avoid
checking here. We will also not need to check for table_name below
in this case.
> + struct sset *table_filter =
> + table_name ? shash_find_data(filters, table_name) : NULL;
> +
> + if (table_filter) {
> + const char *output = &ctx->output.string[base_length];
> + const char *value;
> +
> + SSET_FOR_EACH (value, table_filter) {
> + if (strcasestr(output, value)) {
> + return;
> + }
> + }
> + ds_truncate(&ctx->output, base_length);
> + }
> +}
> +
> /* 'shown' records the tables that has been displayed by the current
> * command to avoid duplicated prints.
> */
> static void
> cmd_show_row(struct ctl_context *ctx, const struct ovsdb_idl_row *row,
> - int level, struct sset *shown)
> + int level, struct sset *shown, struct shash *filters)
> {
> const struct cmd_show_table *show = cmd_show_find_table_by_row(row);
> + size_t start_pos = ctx->output.length;
> size_t i;
>
> ds_put_char_multiple(&ctx->output, ' ', level * 4);
> @@ -2192,7 +2220,7 @@ cmd_show_row(struct ctl_context *ctx, const struct
> ovsdb_idl_row *row,
> ref_show->table,
>
> &datum->keys[j].uuid);
> if (ref_row) {
> - cmd_show_row(ctx, ref_row, level + 1, shown);
> + cmd_show_row(ctx, ref_row, level + 1, shown,
> filters);
> }
> }
> continue;
> @@ -2247,6 +2275,92 @@ cmd_show_row(struct ctl_context *ctx, const struct
> ovsdb_idl_row *row,
> }
> cmd_show_weak_ref(ctx, show, row, level);
> sset_find_and_delete_assert(shown, show->table->name);
> +
> + table_filter(ctx, show, filters, start_pos);
> +}
> +
> +static char * OVS_WARN_UNUSED_RESULT
> +init_filters(const char *filter_str,
> + const struct ovsdb_idl_table_class *top_table,
> + struct shash *filters)
> +{
> + struct sset initialized_filters;
> + sset_init(&initialized_filters);
> + sset_from_delimited_string(&initialized_filters, filter_str, ",");
nit: Either add an empty line here, or move the sset* function calls after
the declaration block.
> + char *error = NULL;
> + const char *item;
> +
> + SSET_FOR_EACH (item, &initialized_filters) {
> + const struct ovsdb_idl_table_class *table = top_table;
> + const char *ptr = strchr(item, '(');
> + size_t len = strlen(item);
> +
> + char *values = NULL;
> + char *table_name = NULL;
> + struct sset parsed_values;
nit: Declarations should go in the reverse x-mass tree order, i.e. the
opposite order in this case.
> +
> + if (ptr && item[len - 1] == ')') {
> + table_name = xmemdup0(item, ptr - item);
> + error = get_table(table_name, &table);
> + if (error) {
> + free(table_name);
> + goto cleanup;
> + }
> +
> + values = xmemdup0(ptr + 1, len - (ptr - item + 2));
> + sset_from_delimited_string(&parsed_values, values, "|");
> +
> + struct sset *value_set = shash_find_data(filters, table_name);
This lookup is done using the user-provided 'table_name', but later we're
calling shash_add() with the table->name as a key. We should use the
table->name in both cases, otherwise --filter='int(qwe),interface(abc)'
will create two separate filters with the same key (shahs allows duplicates)
and we'll end up filtering on a random one instead of both.
> + if (!value_set) {
> + value_set = xzalloc(sizeof *value_set);
> + sset_init(value_set);
> + sset_clone(value_set, &parsed_values);
> + shash_add(filters, table->name, value_set);
> + } else {
> + const char *v;
> + SSET_FOR_EACH (v, &parsed_values) {
> + sset_add(value_set, v);
> + }
> + }
> + } else if (!ptr) {
> + values = xstrdup(item);
> + table_name = xstrdup(top_table->name);
This value is not used anywhere, there is no need to clone it.
Also, 'table' and the 'top_table' are the same in this branch of the 'if'
statement, so we can just use 'table' here and below.
> + sset_from_delimited_string(&parsed_values, values, "|");
> +
> + struct sset *value_set =
> + shash_find_data(filters, top_table->name);
> + if (!value_set) {
> + value_set = xzalloc(sizeof *value_set);
> + sset_init(value_set);
> + sset_add(value_set, values);
The 'values' is a '|'-separated string. We parsed it into 'parsed_values',
but not using the parsed result anywhere for some reason. This means that
we can't use something like --filter="qwe|abc" at the top level, which is
maybe fine, since we can still use --filter="qwe,abc", but it still looks
like an unnecessary restriction.
> + shash_add(filters, top_table->name, value_set);
> + } else {
> + sset_add(value_set, values);
Same here, we should be using the 'parsed_values'.
Once you do that, you'll see that both branches of the top 'if' statement
will contain exactly the same code:
sset_from_delimited_string(&parsed_values, values, "|");
struct sset *value_set = shash_find_data(filters, table->name);
if (!value_set) {
value_set = xzalloc(sizeof *value_set);
sset_init(value_set);
sset_clone(value_set, &parsed_values);
shash_add(filters, table->name, value_set);
} else {
const char *v;
SSET_FOR_EACH (v, &parsed_values) {
sset_add(value_set, v);
}
}
And so, it can be moved out of the 'if' statement.
> + }
> + } else {
> + error = xasprintf("Malformed filter: missing closing ')'");
> + goto cleanup;
> + }
> + sset_destroy(&parsed_values);
> + free(table_name);
> + free(values);
> + }
> +
> +cleanup:
> + sset_destroy(&initialized_filters);
> + return error;
> +}
> +
> +static void
> +destroy_filters(struct shash *filters)
> +{
> + struct shash_node *node;
> + SHASH_FOR_EACH (node, filters) {
> + struct sset *value_set = node->data;
> + sset_destroy(value_set);
> + free(value_set);
> + }
> + shash_destroy(filters);
> }
>
> static void
> @@ -2254,12 +2368,25 @@ cmd_show(struct ctl_context *ctx)
> {
> const struct ovsdb_idl_row *row;
> struct sset shown = SSET_INITIALIZER(&shown);
> + struct shash filters = SHASH_INITIALIZER(&filters);
> +
> + char *filter_str = shash_find_data(&ctx->options, "--filter");
> + if (filter_str && *filter_str) {
> + char *error = init_filters(filter_str,
> + cmd_show_tables[0].table, &filters);
> + if (error) {
> + destroy_filters(&filters);
> + ctx->error = error;
> + return;
> + }
> + }
>
> for (row = ovsdb_idl_first_row(ctx->idl, cmd_show_tables[0].table);
> row; row = ovsdb_idl_next_row(row)) {
> - cmd_show_row(ctx, row, 0, &shown);
> + cmd_show_row(ctx, row, 0, &shown, &filters);
> }
>
> + destroy_filters(&filters);
> ovs_assert(sset_is_empty(&shown));
> sset_destroy(&shown);
> }
> @@ -2554,7 +2681,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..03ab804cf 100644
> --- a/tests/ovs-vsctl.at
> +++ b/tests/ovs-vsctl.at
> @@ -1831,3 +1831,51 @@ 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 tunnel_port \
> + -- set Interface tunnel_port type=geneve \
> + options:remote_ip=1.2.3.4 options:key=123 -- \
> + add-port br0 tunnel_port2 \
> + -- set Interface tunnel_port2 type=geneve \
> + options:remote_ip=1.2.3.5 options:key=125
> +])
> +
> +AT_CHECK([ovs-vsctl --filter='geneve,interface(1.2.3.5)' show | grep Port |
> sort ], [0], [dnl
> + Port br0
> + Port p1
> + Port tunnel_port
> + Port tunnel_port2
> +])
> +
> +AT_CHECK([ovs-vsctl --filter='geneve' show | grep Port | sort ], [0], [dnl
> + Port br0
> + Port p1
> + Port tunnel_port
> + Port tunnel_port2
> +])
> +
> +AT_CHECK([ovs-vsctl --filter='interface(geneve)' show | grep Port | sort ],
> [0], [dnl
> + Port br0
> + Port p1
> + Port tunnel_port
> + Port tunnel_port2
> +])
I'd suggest changing the grep in the above 3 cases to grep -E
'(Port|Interface)'.
This way we'll see the actual difference between these commands, since all the
differences are on the interface level.
Might also be good to have a couple of cases that use '|' syntax.
> +
> +AT_CHECK([ovs-vsctl --filter='interface(p1)' show | grep "Interface p1"],
> [0], [dnl
May be better to shorten this grep to just "Interface" instead of "Interface
p1".
> + Interface p1
> +])
> +
> +AT_CHECK([ovs-vsctl --filter=$'interface\x28p1' show 2>&1 | uuidfilt], [0],
> [dnl
> +ovs-vsctl: Malformed filter: missing closing ')'
> +])
> +
> +AT_CHECK([ovs-vsctl --filter='interface1(p1)' show 2>&1 | uuidfilt], [0],
> [dnl
> +ovs-vsctl: unknown table "interface1"
> +])
Instead of passing this through the unnecessary uuidfilt in the above two cases,
we should just check for the command failure, e.g.:
AT_CHECK([ovs-vsctl --filter='interface1(p1)' show], [1], [stdout], [dnl
ovs-vsctl: unknown table "interface1"
])
> +
> +AT_CLEANUP
> diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> index 575b7c0bf..cedec37c2 100644
> --- a/utilities/ovs-vsctl.8.in
> +++ b/utilities/ovs-vsctl.8.in
> @@ -153,6 +153,13 @@ initialize the database without executing any other
> command.
> .IP "\fBshow\fR"
This line should be removed.
> Prints a brief overview of the database contents.
This text should be integrated into the description below. There should not be
two separate descriptions for the 'show' command.
> .
> +.IP "[\fB\-\-filter=\fIfilter\-rule\fR[\fB,\fIfilter\-rule\fR]...]
> \fBshow\fR"
> +Used for output filtering, the syntax is as follows: each filter rule has
"If the \fB\-\-filter\fR is specified, then it is used for filtering the output.
The syntax of the \fIfilter\-rule\fR is ..."
> +the form \fBtable\-name(filter)\fR.
The actual form is table-name(filter[|filter...]).
> If a table name is specified, then output
> +filtering is performed at the level of the specified table.
I'd add ", i.e. the row in this table is shown only if the printed version of
it,
including all the rows referenced from this one, contains one of the 'filter's
as
a sub-string."
> If the table
> +is not specified, then output filtering is performed at the at the top level
> +of the output table.
This isn't really useful for ovs-vsctl as the top level table (Open_vSwitch
table)
always contains just one row. So, if you filter this way, it is only possible
to
get the full output or none. So, I'd say this last sentence can be just
removed.
> +.
> .IP "\fBemer\-reset\fR"
> Reset the configuration into a clean state. It deconfigures OpenFlow
> controllers, OVSDB servers, and SSL/TLS, and deletes port mirroring,
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
