[ovs-dev] [PATCH] db-ctl-base: Allow print rows that weak reference to table in 'cmd_show_table'.

Justin Pettit jpettit at nicira.com
Thu Aug 13 06:58:12 UTC 2015


I should have added that if you use the attached patch, you'll need to update the ovn-sbctl unit test you were nice enough to include in this patch.

--Justin


> On Aug 12, 2015, at 11:52 PM, Justin Pettit <jpettit at nicira.com> wrote:
> 
> 
>> On Aug 12, 2015, at 9:49 PM, Alex Wang <alexw at nicira.com> wrote:
>> 
>> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
>> index 3028c09..cbafff1 100644
>> --- a/lib/db-ctl-base.c
>> +++ b/lib/db-ctl-base.c
>> @@ -1620,6 +1620,12 @@ pre_cmd_show(struct ctl_context *ctx)
>>                ovsdb_idl_add_column(ctx->idl, column);
>>            }
>>        }
>> +        if (show->wref_table.name_column) {
>> +            ovsdb_idl_add_column(ctx->idl, show->wref_table.name_column);
>> +        }
>> +        if (show->wref_table.wref_column) {
>> +            ovsdb_idl_add_column(ctx->idl, show->wref_table.wref_column);
>> +        }
> 
> Do you need to do a ovsdb_idl_add_table() on 'wref_table.table'?
> 
>> +/*  Prints table entries that weak reference the 'cur_row'. */
>> +static void
>> +cmd_show_weak_ref(struct ctl_context *ctx, const struct cmd_show_table *show,
>> +                  const struct ovsdb_idl_row *cur_row, int level)
>> +{
>> +    const struct ovsdb_idl_row *row_wref;
>> +    const struct ovsdb_idl_table_class *table = show->wref_table.table;
>> +    const struct ovsdb_idl_column *name_column
>> +        = show->wref_table.name_column;
>> +    const struct ovsdb_idl_column *wref_column
>> +        = show->wref_table.wref_column;
>> +
>> +    if (!table || !name_column || !wref_column) {
>> +        return;
>> +    }
>> +
>> +    ds_put_char_multiple(&ctx->output, ' ', (level + 1) * 4);
>> +    ds_put_format(&ctx->output, "%s", table->name);
>> +    ds_put_char(&ctx->output, '\n');
>> +
>> +    for (row_wref = ovsdb_idl_first_row(ctx->idl, table); row_wref;
>> +         row_wref = ovsdb_idl_next_row(row_wref)) {
>> +        const struct ovsdb_datum *wref_datum
>> +            = ovsdb_idl_read(row_wref, wref_column);
>> +        /* If weak reference refers to the 'cur_row', prints it. */
>> +        if (wref_datum->n
>> +            && uuid_equals(&cur_row->uuid, &wref_datum->keys[0].uuid)) {
>> +            const struct ovsdb_datum *name_datum
>> +                = ovsdb_idl_read(row_wref, name_column);
>> +
>> +            ds_put_char_multiple(&ctx->output, ' ', (level + 2) * 4);
>> +            ds_put_format(&ctx->output, "%s: ", name_column->name);
>> +            ovsdb_datum_to_string(name_datum, &name_column->type, &ctx->output);
>> +            ds_put_char(&ctx->output, '\n');
>> +        }
>> +    }
>> +}
> 
> I think this causes the "ovn-controller-vtep" test case to fail due to the text printing before the "for" loop; that gets printed regardless of whether there are any matches or not.  I originally wrote a patch to just not print that preamble if there were no matches.  However, I think the attached patch might be more consistent with commands like "ovs-vsctl show" which print the root name for a row on the same line as the table name.  It also passes the unit tests.
> 
> For example, here's the output of the original patch:
> 
> -=-=-=-=-=-=-=-=-=-=-
> Chassis "56b18105-5706-46ef-80c4-ff20979ab068"
>    Encap geneve
>        ip: "127.0.0.1"
>    Port_Binding
>        logical_port: "sw0-port0"
>        logical_port: "sw0-port1"
> -=-=-=-=-=-=-=-=-=-=-
> 
> Here it is with the attached patch:
> 
> -=-=-=-=-=-=-=-=-=-=-
> Chassis "56b18105-5706-46ef-80c4-ff20979ab068"
>    Encap geneve
>        ip: "127.0.0.1"
>    Port_Binding "sw0-port0"
>    Port_Binding "sw0-port1"
> -=-=-=-=-=-=-=-=-=-=-
> 
> Does that seem reasonable?  If we later want to add other columns from Port_Binding, they can then be placed under each entry.
> 
>> @@ -157,11 +168,16 @@ struct ctl_command *ctl_parse_commands(int argc, char *argv[],
>> *
>> * - 'columns[]' allows user to specify the print of additional columns
>> *   in 'table'.
>> + *
>> + * - if 'wref_table' is filled, print the 'wref_table.table' rows that refer
>> + *   to the 'table'.
> 
> What do you think of this phrasing instead?  I think it's a bit more descriptive.
> 
> * - if 'wref_table' is populated, print 'wref_table.name_column' for
> *   each row in table 'wref_table.table' that has a reference to 'table'
> *   in 'wref_table.wref_column'.  Every field must be populated.
> 
> Thanks a lot for doing this on short notice!
> 
> Acked-by: Justin Pettit <jpettit at nicira.com>
> 
> --Justin
> 
> 
> -=-=-=-=-=-=-=-=-=-=-=-=-
> 
> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
> index cbafff1..142428e 100644
> --- a/lib/db-ctl-base.c
> +++ b/lib/db-ctl-base.c
> @@ -1671,10 +1671,6 @@ cmd_show_weak_ref(struct ctl_context *ctx, const struct c
>         return;
>     }
> 
> -    ds_put_char_multiple(&ctx->output, ' ', (level + 1) * 4);
> -    ds_put_format(&ctx->output, "%s", table->name);
> -    ds_put_char(&ctx->output, '\n');
> -
>     for (row_wref = ovsdb_idl_first_row(ctx->idl, table); row_wref;
>          row_wref = ovsdb_idl_next_row(row_wref)) {
>         const struct ovsdb_datum *wref_datum
> @@ -1685,8 +1681,8 @@ cmd_show_weak_ref(struct ctl_context *ctx, const struct cm
>             const struct ovsdb_datum *name_datum
>                 = ovsdb_idl_read(row_wref, name_column);
> 
> -            ds_put_char_multiple(&ctx->output, ' ', (level + 2) * 4);
> -            ds_put_format(&ctx->output, "%s: ", name_column->name);
> +            ds_put_char_multiple(&ctx->output, ' ', (level + 1) * 4);
> +            ds_put_format(&ctx->output, "%s ", table->name);
>             ovsdb_datum_to_string(name_datum, &name_column->type, &ctx->output)
>             ds_put_char(&ctx->output, '\n');
>         }
> 
> 




More information about the dev mailing list