On Tue, Nov 12, 2024 at 6:38 PM Alexandra Rukomoinikova
<[email protected]> wrote:

> Added ability to output multiple individual chassis by their
> name, hostname and encap-ip.
>
> Signed-off-by: Alexandra Rukomoinikova <[email protected]>
> Tested-by: Ivan Burnin <[email protected]>
> ---
> v2: - removed all copy from ovs function
>     - removed hash for hostnames
> ---
>

Hi Alexandra,

thank you for the followup and sorry for the delay, I have a few minor
comments that could be addressed during merge if maintainers agree.


>  tests/ovn-sbctl.at    |   9 ++++
>  utilities/ovn-sbctl.c | 109 +++++++++++++++++++++++++++++++++---------
>  2 files changed, 96 insertions(+), 22 deletions(-)
>
> diff --git a/tests/ovn-sbctl.at b/tests/ovn-sbctl.at
> index 19ac55c80..9743dacb8 100644
> --- a/tests/ovn-sbctl.at
> +++ b/tests/ovn-sbctl.at
> @@ -164,6 +164,15 @@ Chassis ch0
>      Port_Binding vif0
>  ])
>
> +AT_CHECK([ovn-sbctl set Chassis ch0 hostname="ch0_host"])
> +AT_CHECK([ovn-sbctl chassis-add ch1 geneve 1.2.3.6])
> +AT_CHECK([ovn-sbctl chassis-add ch2 geneve 1.2.3.7])
> +AT_CHECK([ovn-sbctl show ch0_host ch1 1.2.3.7 | grep Chassis], [0], [dnl
> +Chassis ch0
> +Chassis ch1
> +Chassis ch2
> +])
> +
>  uuid=$(ovn-sbctl --columns=_uuid list Chassis ch0 | cut -d ':' -f2 | tr
> -d ' ')
>  AT_CHECK_UNQUOTED([ovn-sbctl --columns=logical_port,mac,chassis list
> Port_Binding], [0], [dnl
>  logical_port        : vif0
> diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
> index f1f8c2b42..e2ef37fa3 100644
> --- a/utilities/ovn-sbctl.c
> +++ b/utilities/ovn-sbctl.c
> @@ -91,7 +91,7 @@ sbctl_usage(void)
>  usage: %s [OPTIONS] COMMAND [ARG...]\n\
>  \n\
>  General commands:\n\
> -  show                        print overview of database contents\n\
> +  show [CHASSIS | HOSTNAME | ENCAP-IP ...] print overview of database
> contents\n\
>

As pointed out by checkpatch this line is longer than 79 characters.


>  \n\
>  Chassis commands:\n\
>    chassis-add CHASSIS ENCAP-TYPE ENCAP-IP  create a new chassis named\n\
> @@ -355,12 +355,14 @@ static void
>  pre_get_info(struct ctl_context *ctx)
>  {
>      ovsdb_idl_add_column(ctx->idl, &sbrec_chassis_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &sbrec_chassis_col_hostname);
>      ovsdb_idl_add_column(ctx->idl, &sbrec_chassis_col_encaps);
>
>      ovsdb_idl_add_column(ctx->idl, &sbrec_chassis_private_col_name);
>
>      ovsdb_idl_add_column(ctx->idl, &sbrec_encap_col_type);
>      ovsdb_idl_add_column(ctx->idl, &sbrec_encap_col_ip);
> +    ovsdb_idl_add_column(ctx->idl, &sbrec_encap_col_options);
>
>      ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_logical_port);
>      ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_tunnel_key);
> @@ -404,26 +406,6 @@ pre_get_info(struct ctl_context *ctx)
>      ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_protocol);
>  }
>
> -static struct cmd_show_table cmd_show_tables[] = {
> -    {&sbrec_table_chassis,
> -     &sbrec_chassis_col_name,
> -     {&sbrec_chassis_col_hostname,
> -      &sbrec_chassis_col_encaps,
> -      NULL},
> -     {&sbrec_table_port_binding,
> -      &sbrec_port_binding_col_logical_port,
> -      &sbrec_port_binding_col_chassis}},
> -
> -    {&sbrec_table_encap,
> -     &sbrec_encap_col_type,
> -     {&sbrec_encap_col_ip,
> -      &sbrec_encap_col_options,
> -      NULL},
> -     {NULL, NULL, NULL}},
> -
> -    {NULL, NULL, {NULL, NULL, NULL}, {NULL, NULL, NULL}},
> -};
> -
>  static void
>  sbctl_init(struct ctl_context *ctx OVS_UNUSED)
>  {
> @@ -564,6 +546,86 @@ cmd_lsp_unbind(struct ctl_context *ctx)
>      }
>  }
>
> +static void
> +show_encap(struct ctl_context *ctx,
> +           const struct sbrec_encap *encap)
> +{
> +    ds_put_format(&ctx->output, "    Encap %s\n", encap->type);
> +    ds_put_format(&ctx->output, "        ip: \"%s\"\n", encap->ip);
> +    ds_put_format(&ctx->output, "        options: {csum=\"%s\"}\n",
> +                  smap_get_def(&encap->options, "csum", " "));
> +};
> +
> +static void
> +show_chassis(struct ctl_context *ctx,
> +             const struct sbrec_chassis *chassis)
> +{
> +    const struct sbrec_port_binding *pb;
> +
> +    ds_put_format(&ctx->output, "Chassis %s\n", chassis->name);
> +
> +    if (chassis->hostname && strlen(chassis->hostname)) {
>

No need for strlen, simple "chassis->hostname[0]" is sufficient.


> +        ds_put_format(&ctx->output, "    hostname: %s\n",
> chassis->hostname);
> +    }
> +
> +    for (size_t i = 0; i < chassis->n_encaps; i++) {
> +        show_encap(ctx, chassis->encaps[i]);
> +    }
> +
> +    SBREC_PORT_BINDING_FOR_EACH (pb, ctx->idl) {
> +        if (pb->chassis == chassis) {
> +            ds_put_format(&ctx->output, "    Port_Binding %s\n",
> +                          pb->logical_port);
> +        }
> +    }
> +}
> +
> +static const struct sbrec_chassis *
> +sbctl_find_chassis(struct ctl_context *ctx, const char *name)
> +{
> +    const struct sbctl_chassis *chassis = NULL;
> +    const struct sbrec_chassis *chassis_rec = NULL;
> +
> +    /* Find chassis by his name. */
> +    chassis = find_chassis(ctx, name, false);
> +    if (chassis) {
> +        return chassis->ch_cfg;
> +    }
> +
> +    /* Find chassis by his hostname or encap ip. */
> +    SBREC_CHASSIS_FOR_EACH (chassis_rec, ctx->idl) {
> +        if (chassis_rec->hostname && !strcmp(chassis_rec->hostname,
> name)) {
> +            return chassis_rec;
> +        }
> +        for (size_t i = 0; i < chassis_rec->n_encaps; i++) {
> +            if (!strcmp(chassis_rec->encaps[i]->ip, name)) {
> +                return chassis_rec;
> +            }
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +static void
> +cmd_show(struct ctl_context *ctx)
> +{
> +    const struct sbrec_chassis *chassis;
> +
> +    if (ctx->argc > 1) {
> +        for (int i = 1; i <= ctx->argc - 1; i++) {
> +            chassis = sbctl_find_chassis(ctx, ctx->argv[i]);
> +            if (chassis) {
> +                show_chassis(ctx, chassis);
> +            }
> +        }
> +    } else {
> +        SBREC_CHASSIS_FOR_EACH (chassis, ctx->idl) {
> +            show_chassis(ctx, chassis);
> +        }
> +    }
> +}
> +
>  enum {
>      PL_INGRESS,
>      PL_EGRESS,
> @@ -1554,6 +1616,9 @@ static const struct ctl_table_class
> tables[SBREC_N_TABLES] = {
>  static const struct ctl_command_syntax sbctl_commands[] = {
>      { "init", 0, 0, "", NULL, sbctl_init, NULL, "", RW },
>
> +    { "show", 0, INT_MAX, "[CHASSIS | HOSTNAME | ENCAP-IP ...]",
> +     pre_get_info, cmd_show, NULL,  "", RO },
> +
>      /* Chassis commands. */
>      {"chassis-add", 3, 3, "CHASSIS ENCAP-TYPE ENCAP-IP", pre_get_info,
>       cmd_chassis_add, NULL, "--may-exist", RW},
> @@ -1610,7 +1675,7 @@ main(int argc, char *argv[])
>
>          .idl_class = &sbrec_idl_class,
>          .tables = tables,
> -        .cmd_show_table = cmd_show_tables,
> +        .cmd_show_table = NULL,
>          .commands = sbctl_commands,
>
>          .usage = sbctl_usage,
> --
> 2.34.1
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
With those addressed:

Acked-by: Ales Musil <[email protected]>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to