Adopted all suggestions and fixed unittest,
Applied to master~
Thanks,
Alex Wang,
On Thu, Aug 13, 2015 at 12:17 AM, Alex Wang <al...@nicira.com> wrote:
> Hey Justin,
>
> Thx for the responsive review~
>
> On Wed, Aug 12, 2015 at 11:58 PM, Justin Pettit <jpet...@nicira.com>
> wrote:
>
>> 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 <jpet...@nicira.com> wrote:
>> >
>> >
>> >> On Aug 12, 2015, at 9:49 PM, Alex Wang <al...@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'?
>> >
>>
>
> Yeah, I should do that,
>
>
>> >> +/* 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.
>> >
>>
>
> Sorry forget to run all 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.
>>
>
> Yeah, totally makes sense,
>
> >
>> >> @@ -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.
>> >
>>
>
> This is more comprehensive, thx, will adopt it,
>
>
>
>> > Thanks a lot for doing this on short notice!
>> >
>> > Acked-by: Justin Pettit <jpet...@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');
>> > }
>> >
>> >
>>
>>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
