[ovs-dev,mointor2,1/9] ovsdb: refactor, add ovsdb_monitor_max_columns()
diff mbox

Message ID 1445489131-21483-1-git-send-email-azhou@nicira.com
State Changes Requested
Headers show

Commit Message

Andy Zhou Oct. 22, 2015, 4:45 a.m. UTC
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(-)

Comments

Andy Zhou Oct. 22, 2015, 4:49 a.m. UTC | #1
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
>
Liran Schour Oct. 23, 2015, 3:06 p.m. UTC | #2
"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.
Andy Zhou Oct. 23, 2015, 8:31 p.m. UTC | #3
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.
Liran Schour Oct. 25, 2015, 7:19 a.m. UTC | #4
"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.
Liran Schour Nov. 2, 2015, 8:27 a.m. UTC | #5
"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.
Andy Zhou Nov. 2, 2015, 6:02 p.m. UTC | #6
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.
Liran Schour Nov. 3, 2015, 7:50 a.m. UTC | #7
"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;
> +        }
> +
Andy Zhou Nov. 3, 2015, 5:12 p.m. UTC | #8
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!
Ben Pfaff Nov. 10, 2015, 4:27 p.m. UTC | #9
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>
Ben Pfaff Nov. 10, 2015, 5:49 p.m. UTC | #10
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>

Patch
diff mbox

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) {