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
