| Message ID | 1445489131-21483-1-git-send-email-azhou@nicira.com |
|---|---|
| State | Changes Requested |
| Headers | show |
I have also pushed the patches to my github account: https://github.com/azhou-nicira/ovs-review/tree/monitor2 On Wed, Oct 21, 2015 at 9:45 PM, Andy Zhou <azhou@nicira.com> wrote: > This function will have multiple callers in later patches. > > Signed-off-by: Andy Zhou <azhou@nicira.com> > --- > ovsdb/monitor.c | 27 +++++++++++++++++---------- > 1 file changed, 17 insertions(+), 10 deletions(-) > > diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c > index 8a64fc1..f978960 100644 > --- a/ovsdb/monitor.c > +++ b/ovsdb/monitor.c > @@ -547,6 +547,21 @@ ovsdb_monitor_compose_row_update( > return row_json; > } > > +static size_t > +ovsdb_monitor_max_columns(struct ovsdb_monitor *dbmon) > +{ > + struct shash_node *node; > + size_t max_columns = 0; > + > + SHASH_FOR_EACH (node, &dbmon->tables) { > + struct ovsdb_monitor_table *mt = node->data; > + > + max_columns = MAX(max_columns, mt->n_columns); > + } > + > + return max_columns; > +} > + > /* Constructs and returns JSON for a <table-updates> object (as described in > * RFC 7047) for all the outstanding changes within 'monitor', starting from > * 'transaction'. */ > @@ -555,17 +570,9 @@ ovsdb_monitor_compose_update(struct ovsdb_monitor *dbmon, > bool initial, uint64_t transaction) > { > struct shash_node *node; > - unsigned long int *changed; > struct json *json; > - size_t max_columns; > - > - max_columns = 0; > - SHASH_FOR_EACH (node, &dbmon->tables) { > - struct ovsdb_monitor_table *mt = node->data; > - > - max_columns = MAX(max_columns, mt->n_columns); > - } > - changed = xmalloc(bitmap_n_bytes(max_columns)); > + size_t max_columns = ovsdb_monitor_max_columns(dbmon); > + unsigned long int *changed = xmalloc(bitmap_n_bytes(max_columns)); > > json = NULL; > SHASH_FOR_EACH (node, &dbmon->tables) { > -- > 1.9.1 >
"dev" <dev-bounces@openvswitch.org> wrote on 22/10/2015 07:49:05 AM: > > I have also pushed the patches to my github account: > > https://github.com/azhou-nicira/ovs-review/tree/monitor2 > Thanks. I will base my mointor_cond code on top of your patches.
On Fri, Oct 23, 2015 at 8:06 AM, Liran Schour <LIRANS@il.ibm.com> wrote: > "dev" <dev-bounces@openvswitch.org> wrote on 22/10/2015 07:49:05 AM: >> >> I have also pushed the patches to my github account: >> >> https://github.com/azhou-nicira/ovs-review/tree/monitor2 >> > Thanks. I will base my mointor_cond code on top of your patches. Great! Please let me know if you have comments on those patches.
"dev" <dev-bounces@openvswitch.org> wrote on 22/10/2015 07:45:27 AM: > + > +Both monitor and monitor2 sessions can exist concurrently. However, > +monitor and monitor2 shares the smae <json-value> parameter space; it > +must be unique among all monitor and monitor2 seesion. > + Small typo, same instead of smae. Otherwise looks good.
"dev" <dev-bounces@openvswitch.org> wrote on 22/10/2015 07:45:27 AM: > > Add functions that can generate "update2" notification for a > "monitor2" session. "monitor2" and "update2" are RFC 7047 extensions > deescribed by ovsdb-server(1) manpage. See the manpage changes > for more details. > > Signed-off-by: Andy Zhou <azhou@nicira.com> ... > + } else { > + diff_json = json_object_create(); > + const char *op; > + > + for (i = 0; i < mt->n_columns; i++) { > + const struct ovsdb_monitor_column *c = &mt->columns[i]; > + > + if (!(type & c->select)) { > + /* We don't care about this type of change for this > + * particular column (but we will care about it for some > + * other column). */ > + continue; > + } > + > + if (type == OJMS_MODIFY) { > + struct ovsdb_datum *diff; > + > + if (!bitmap_is_set(changed, i)) { > + continue; > + } > + > + diff = ovsdb_datum_diff(&row->old[i], &row->new[i], > + &c->column->type); > + json_object_put(diff_json, c->column->name, > + ovsdb_datum_to_json(diff, &c->column->type)); > + ovsdb_datum_destroy(diff, &c->column->type); > + free(diff); > + } else { > + if (!ovsdb_datum_is_default(&row->new[i], > &c->column->type)) { > + json_object_put(diff_json, c->column->name, > + ovsdb_datum_to_json(&row->new[i], > + &c->column->type)); > + } Why to use here ovsdb_datum_is_default()? For example if a column is of type INTEGER and the value is 0, you will miss that value in the update message. Seems like a bug.
On Mon, Nov 2, 2015 at 12:27 AM, Liran Schour <LIRANS@il.ibm.com> wrote: > "dev" <dev-bounces@openvswitch.org> wrote on 22/10/2015 07:45:27 AM: >> >> Add functions that can generate "update2" notification for a >> "monitor2" session. "monitor2" and "update2" are RFC 7047 extensions >> deescribed by ovsdb-server(1) manpage. See the manpage changes >> for more details. >> >> Signed-off-by: Andy Zhou <azhou@nicira.com> > > ... > > >> + } else { >> + diff_json = json_object_create(); >> + const char *op; >> + >> + for (i = 0; i < mt->n_columns; i++) { >> + const struct ovsdb_monitor_column *c = &mt->columns[i]; >> + >> + if (!(type & c->select)) { >> + /* We don't care about this type of change for this >> + * particular column (but we will care about it for some >> + * other column). */ >> + continue; >> + } >> + >> + if (type == OJMS_MODIFY) { >> + struct ovsdb_datum *diff; >> + >> + if (!bitmap_is_set(changed, i)) { >> + continue; >> + } >> + >> + diff = ovsdb_datum_diff(&row->old[i], &row->new[i], >> + &c->column->type); >> + json_object_put(diff_json, c->column->name, >> + ovsdb_datum_to_json(diff, >> &c->column->type)); >> + ovsdb_datum_destroy(diff, &c->column->type); >> + free(diff); >> + } else { >> + if (!ovsdb_datum_is_default(&row->new[i], >> &c->column->type)) { >> + json_object_put(diff_json, c->column->name, >> + ovsdb_datum_to_json(&row->new[i], >> + >> &c->column->type)); >> + } > > Why to use here ovsdb_datum_is_default()? For example if a column is of type > INTEGER and the value is 0, you will miss that value in the update message. > Seems like a bug. > This is for 'initial' or 'insert' rows. In those cases, the rows will be created with the default values on the client side. In your example, the column will be created with value 0. It seems sending information about 0 will be redundant? Please let me know if I am still missing something here. In any case, It seems I did not add enough comments about this optimization. Will add more in the next rev. Thanks for reviewing.
"dev" <dev-bounces@openvswitch.org> wrote on 22/10/2015 07:45:29 AM: > > Add monito2 option to ovsdb-client. Sed ovsdb-client(1) manpage patch > for details. > > Signed-off-by: Andy Zhou <azhou@nicira.com> > --- ... > static void > +monitor2_print_row(struct json *row, const char *type, const char *uuid, > + const struct ovsdb_column_set *columns, struct table *t) > +{ > + if (!strcmp(type, "delete")) { > + if (row) { Should be: if (row->type != JSON_NULL) { > + ovs_error(0, "delete method does not expect <row>"); > + return; > + } > +
On Mon, Nov 2, 2015 at 11:50 PM, Liran Schour <LIRANS@il.ibm.com> wrote: > "dev" <dev-bounces@openvswitch.org> wrote on 22/10/2015 07:45:29 AM: >> >> Add monito2 option to ovsdb-client. Sed ovsdb-client(1) manpage patch >> for details. >> >> Signed-off-by: Andy Zhou <azhou@nicira.com> >> --- > ... > >> static void >> +monitor2_print_row(struct json *row, const char *type, const char *uuid, >> + const struct ovsdb_column_set *columns, struct table >> *t) >> +{ >> + if (!strcmp(type, "delete")) { >> + if (row) { > > Should be: if (row->type != JSON_NULL) { > Yes, that's a bug. Thanks for catching it!
On Wed, Oct 21, 2015 at 09:45:23PM -0700, Andy Zhou wrote: > This function will have multiple callers in later patches. > > Signed-off-by: Andy Zhou <azhou@nicira.com> Acked-by: Ben Pfaff <blp@ovn.org>
On Wed, Oct 21, 2015 at 09:45:31PM -0700, Andy Zhou wrote: > Add test to make sure ovs-vswitchd fall back to use the > "monitor" method when connecting to an older ovsdb-server that > does not support "monitor2". > > For testing backward compatibility, add an ovs-appctl command: > "ovsdb-server/disable-monitor2". This command will restart > all currently open jsonrpc connections, but without support for > 'monitor2' JSON-RPC method for the new connections. > > There is no corresponding enable command, since this feature is only > useful for testing. 'monitor2' will be available when ovsdb-server > restarts. > > Signed-off-by: Andy Zhou <azhou@nicira.com> Thanks! Acked-by: Ben Pfaff <blp@ovn.org>
diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c index 8a64fc1..f978960 100644 --- a/ovsdb/monitor.c +++ b/ovsdb/monitor.c @@ -547,6 +547,21 @@ ovsdb_monitor_compose_row_update( return row_json; } +static size_t +ovsdb_monitor_max_columns(struct ovsdb_monitor *dbmon) +{ + struct shash_node *node; + size_t max_columns = 0; + + SHASH_FOR_EACH (node, &dbmon->tables) { + struct ovsdb_monitor_table *mt = node->data; + + max_columns = MAX(max_columns, mt->n_columns); + } + + return max_columns; +} + /* Constructs and returns JSON for a <table-updates> object (as described in * RFC 7047) for all the outstanding changes within 'monitor', starting from * 'transaction'. */ @@ -555,17 +570,9 @@ ovsdb_monitor_compose_update(struct ovsdb_monitor *dbmon, bool initial, uint64_t transaction) { struct shash_node *node; - unsigned long int *changed; struct json *json; - size_t max_columns; - - max_columns = 0; - SHASH_FOR_EACH (node, &dbmon->tables) { - struct ovsdb_monitor_table *mt = node->data; - - max_columns = MAX(max_columns, mt->n_columns); - } - changed = xmalloc(bitmap_n_bytes(max_columns)); + size_t max_columns = ovsdb_monitor_max_columns(dbmon); + unsigned long int *changed = xmalloc(bitmap_n_bytes(max_columns)); json = NULL; SHASH_FOR_EACH (node, &dbmon->tables) {
This function will have multiple callers in later patches. Signed-off-by: Andy Zhou <azhou@nicira.com> --- ovsdb/monitor.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)