On 6/18/25 12:08 PM, Mairtin O'Loingsigh via dev wrote:
> Add support for using ovn-nbctl to list ports in a port group
>
> pg-get-ports: Get logical switch ports of a port group
>
> Reported-at: https://issues.redhat.com/browse/FDP-1463
> Signed-off-by: Mairtin O'Loingsigh <moloi...@redhat.com>
> ---
Hi Mairtin,
Thanks for the patch! To me it looks correct from a functional
perspective. I left a bunch of minor comments below though.
> tests/ovn-nbctl.at | 10 +++++++-
> utilities/ovn-nbctl.c | 56 +++++++++++++++++++++++++++++++++++++++++++
Nit: we probably need to add a NEWS entry mentioning this new command.
Please also update the ovn-nbctl man page (ovn-nbctl.8.xml) and document
this new get command.
> 2 files changed, 65 insertions(+), 1 deletion(-)
>
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index 447a0666f..b2f0b7df8 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -2798,15 +2798,23 @@ pg1
> AT_CHECK_UNQUOTED([ovn-nbctl --bare --columns=ports list port_group pg1],
> [0], [dnl
> $SW1P1
> ])
> +AT_CHECK([ovn-nbctl pg-get-ports pg1],[0], [dnl
> +sw1-p1
> +])
>
> AT_CHECK([ovn-nbctl pg-set-ports pg1 sw1-p2], [0], [ignore])
> AT_CHECK_UNQUOTED([ovn-nbctl --bare --columns=ports list port_group pg1],
> [0], [dnl
> $SW1P2
> ])
> +AT_CHECK([ovn-nbctl pg-get-ports pg1],[0], [dnl
> +sw1-p2
> +])
>
> AT_CHECK([ovn-nbctl pg-del pg1], [0], [ignore])
> AT_CHECK([ovn-nbctl list port_group], [0], [])
> -])
> +AT_CHECK([ovn-nbctl pg-get-ports pg1], [1], [],
> + [ovn-nbctl: pg1: port group name not found
> +])])
Thanks for adding tests! But, for completeness, can we also add a test
that configures port groups with more than one port in them? That'd
also test the sorting part.
> dnl ---------------------------------------------------------------------
>
> OVN_NBCTL_TEST([ovn_nbctl_fwd_groups], [fwd groups], [
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 33e789523..b3c9b89fb 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -483,6 +483,7 @@ Port group commands:\n\
> pg-add PG [PORTS] Create port group PG with optional PORTS\n\
> pg-set-ports PG PORTS Set PORTS on port group PG\n\
> pg-del PG Delete port group PG\n\
> + pg-get-ports PG List port group PG\n\
Maybe "Get PORTS on port group PG" is slightly more accurate?
> HA chassis group commands:\n\
> ha-chassis-group-add GRP Create an HA chassis group GRP\n\
> ha-chassis-group-del GRP Delete the HA chassis group GRP\n\
> @@ -7489,6 +7490,59 @@ cmd_pg_del(struct ctl_context *ctx)
> nbrec_port_group_delete(pg);
> }
>
> +static void
> +cmd_pre_pg_get_ports(struct ctl_context *ctx)
Nit: This is identical to cmd_pre_pg_set_ports() and given that I think
we always want to "get" back exactly the same information we can "set"
it might make sense to just rename it to cmd_pre_pg_set_get_ports() and
avoid duplicating the code.
> +{
> + ovsdb_idl_add_column(ctx->idl, &nbrec_logical_switch_port_col_name);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_port_group_col_name);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_port_group_col_ports);
> +}
> +
> +static int
> +port_name_cmp(const void *s1_, const void *s2_)
> +{
> + const char *s1 = *(char **) s1_, *s2 = *(char **) s2_;
I think I'd rewrite this as:
const char *s1 = *(const char **) s1_;
const char *s2 = *(const char **) s2_;
> + return strcmp(s1, s2);
> +}
> +
> +static void
> +cmd_pg_get_ports(struct ctl_context *ctx)
> +{
> + const struct nbrec_port_group *pg;
> + struct nbrec_logical_switch_port **ports;
> + size_t i;
> + char **port_names;
> + char *error;
> +
> + error = pg_by_name_or_uuid(ctx, ctx->argv[1], true, &pg);
In this case I think we'd prefer to define the variable here directly
(as close to where it's used), i.e.:
char *error = pg_by_name_or_uuid(...);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> +
> + ports = pg->ports;
> + if (pg->n_ports == 0) {
> + return;
> + }
> +
> + port_names = xmalloc(sizeof(char *) * pg->n_ports);
Per our coding guidelines this should be "sizeof *port_names":
https://github.com/ovn-org/ovn/blob/main/Documentation/internals/contributing/coding-style.rst#expressions
> + for (i = 0; i < pg->n_ports; i++) {
> + port_names[i] = ports[i]->name;
I'd just use pg->ports here instead of having the new local 'ports'.
> + }
> +
> + qsort(port_names, pg->n_ports, sizeof(char *), port_name_cmp);
Same here, "sizeof *port_names".
> + ds_put_format(&ctx->output, "%s", port_names[0]);
> + for (i = 1; i < pg->n_ports; i++) {
> + ds_put_format(&ctx->output, " %s", port_names[i]);
> + }
> +
'i' is always > 0 here, isn't it?
Which means we can just always limit the scope of the 'i' variable to
the loop it's used in. That is we can do this in both loops above:
for (size_t i = ...)
> + if (i > 0) {
> + ds_put_cstr(&ctx->output, "\n");
> + }
> +
> + free(port_names);
> +}
> +
> static const struct nbrec_ha_chassis_group*
> ha_chassis_group_by_name_or_uuid(struct ctl_context *ctx, const char *id,
> bool must_exist)
> @@ -8420,6 +8474,8 @@ static const struct ctl_command_syntax nbctl_commands[]
> = {
> {"pg-set-ports", 2, INT_MAX, "", cmd_pre_pg_set_ports, cmd_pg_set_ports,
> NULL, "", RW },
> {"pg-del", 1, 1, "", cmd_pre_pg_del, cmd_pg_del, NULL, "", RW },
> + {"pg-get-ports", 1, 1, "", cmd_pre_pg_get_ports, cmd_pg_get_ports,
"ovn-nbctl pg-get-ports" expects exactly one argument. The third field
of the 'struct ctl_command_syntax' structure is:
/* Names that roughly describe the arguments that the command
* uses. These should be similar to the names displayed in the
* man page or in the help output. */
const char *arguments;
We should probably change this to:
{ "pg-get-ports", 1, 1, "", "PORT_GROUP", ...
> + NULL, "", RO },
>
> /* HA chassis group commands. */
> {"ha-chassis-group-add", 1, 1, "[CHASSIS GROUP]",
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
