diff mbox

[ovs-dev,monitor_cond,V5,03/18] ovsdb: allow unmonitored columns in condition evaluation

Message ID 1457078953-22280-4-git-send-email-lirans@il.ibm.com
State Changes Requested
Headers show

Commit Message

Liran Schour March 4, 2016, 8:08 a.m. UTC 
This commit allows to add unmonitored columns to a monitored table
due to condition update.
It will be used to evaluate conditions on unmonitored columns.
Update notification includes only monitored columns.
Due to the limited number of columns We do not remove unused unmonitored
columns due to condition update to keep the code simple.

Signed-off-by: Liran Schour <lirans@il.ibm.com>

---
v4->v5:
* None monitored --> unmonitored

v3->v4:
* Do not add duplicated none-monitored column
---
 ovsdb/jsonrpc-server.c | 23 +++++++++++------------
 ovsdb/monitor.c        | 42 ++++++++++++++++++++++++++++++++++--------
 ovsdb/monitor.h        |  2 +-
 3 files changed, 46 insertions(+), 21 deletions(-)

Comments

Ben Pfaff March 21, 2016, 8:14 p.m. UTC | #1 
On Fri, Mar 04, 2016 at 08:08:58AM +0000, Liran Schour wrote:
> This commit allows to add unmonitored columns to a monitored table
> due to condition update.
> It will be used to evaluate conditions on unmonitored columns.
> Update notification includes only monitored columns.
> Due to the limited number of columns We do not remove unused unmonitored
> columns due to condition update to keep the code simple.
> 
> Signed-off-by: Liran Schour <lirans@il.ibm.com>

Thanks for the revised series!

Can the following check in ovsdb_monitor_add_column() be reduced to a
check for mt->columns_index_map[column->index] != -1?  Why check only
for unmonitored columns?  Also, I don't understand the comment about not
removing unmonitored columns.

> +    /* Check duplication only for unmonitored columns.
> +     * We do not remove unused unmonitored columns due to condition
> +     * update */
> +    if (!monitored) {
> +        int i;
> +        for (i = 0; i < mt->n_columns; i++) {
> +            if (mt->columns[i].column == column) {
> +                /* column exists */
> +                return;
> +            }
> +        }
> +    }

ovsdb_monitor_compose_row_update() and
ovsdb_monitor_compose_row_update2() iterate through all columns and then
skip the ones that are not monitored.  Can they just iterate through the
monitored ones, e.g. change
     for (i = 0; i < mt->n_columns; i++) {
to
     for (i = 0; i < mt->n_monitored_columns; i++) {
Liran Schour March 26, 2016, 2:13 p.m. UTC | #2 
Ben Pfaff <blp@ovn.org> wrote on 21/03/2016 10:14:47 PM:

> On Fri, Mar 04, 2016 at 08:08:58AM +0000, Liran Schour wrote:
> > This commit allows to add unmonitored columns to a monitored table
> > due to condition update.
> > It will be used to evaluate conditions on unmonitored columns.
> > Update notification includes only monitored columns.
> > Due to the limited number of columns We do not remove unused 
unmonitored
> > columns due to condition update to keep the code simple.
> > 
> > Signed-off-by: Liran Schour <lirans@il.ibm.com>
> 
> Thanks for the revised series!
> 
> Can the following check in ovsdb_monitor_add_column() be reduced to a
> check for mt->columns_index_map[column->index] != -1? Why check only
> for unmonitored columns?  Also, I don't understand the comment about not
> removing unmonitored columns.
> 
> > +    /* Check duplication only for unmonitored columns.
> > +     * We do not remove unused unmonitored columns due to condition
> > +     * update */
> > +    if (!monitored) {
> > +        int i;
> > +        for (i = 0; i < mt->n_columns; i++) {
> > +            if (mt->columns[i].column == column) {
> > +                /* column exists */
> > +                return;
> > +            }
> > +        }
> > +    }
> 

Changed the check to mt->columns_index_map[column->index] != -1.
We check duplication only for unmonitored columns because for duplicated 
monitored columns we cancel the whole monitor request on 
ovsdb_monitor_table_check_duplicates() call. Duplicated unmonitored 
columns should only cause a return without adding this column.
The comment is not clear, will change it to the following:

/* Check duplication only for unmonitored columns.
 * Duplicated monitored columns will cause cancelation of monitor request
 * on ovsdb_monitor_table_check_duplicates() call */
 
> ovsdb_monitor_compose_row_update() and
> ovsdb_monitor_compose_row_update2() iterate through all columns and then
> skip the ones that are not monitored.  Can they just iterate through the
> monitored ones, e.g. change
>      for (i = 0; i < mt->n_columns; i++) {
> to
>      for (i = 0; i < mt->n_monitored_columns; i++) {
> 

Right, applied this change to the next patch series.
diff mbox

Patch

diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
index 15dbc4e..770ed08 100644
--- a/ovsdb/jsonrpc-server.c
+++ b/ovsdb/jsonrpc-server.c
@@ -1093,8 +1093,7 @@  parse_bool(struct ovsdb_parser *parser, const char *name, bool default_value)
 static struct ovsdb_error * OVS_WARN_UNUSED_RESULT
 ovsdb_jsonrpc_parse_monitor_request(struct ovsdb_monitor *dbmon,
                                     const struct ovsdb_table *table,
-                                    const struct json *monitor_request,
-                                    size_t *allocated_columns)
+                                    const struct json *monitor_request)
 {
     const struct ovsdb_table_schema *ts = table->schema;
     enum ovsdb_monitor_selection select;
@@ -1158,8 +1157,8 @@  ovsdb_jsonrpc_parse_monitor_request(struct ovsdb_monitor *dbmon,
                 return ovsdb_syntax_error(columns, NULL, "%s is not a valid "
                                           "column name", s);
             }
-            ovsdb_monitor_add_column(dbmon, table, column, select,
-                                     allocated_columns);
+            ovsdb_monitor_add_column(dbmon, table, column,
+                                     select, true);
         }
     } else {
         struct shash_node *node;
@@ -1167,8 +1166,8 @@  ovsdb_jsonrpc_parse_monitor_request(struct ovsdb_monitor *dbmon,
         SHASH_FOR_EACH (node, &ts->columns) {
             const struct ovsdb_column *column = node->data;
             if (column->index != OVSDB_COL_UUID) {
-                ovsdb_monitor_add_column(dbmon, table, column, select,
-                                         allocated_columns);
+                ovsdb_monitor_add_column(dbmon, table, column,
+                                         select, true);
             }
         }
     }
@@ -1218,7 +1217,6 @@  ovsdb_jsonrpc_monitor_create(struct ovsdb_jsonrpc_session *s, struct ovsdb *db,
     SHASH_FOR_EACH (node, json_object(monitor_requests)) {
         const struct ovsdb_table *table;
         const char *column_name;
-        size_t allocated_columns;
         const struct json *mr_value;
         size_t i;
 
@@ -1233,20 +1231,21 @@  ovsdb_jsonrpc_monitor_create(struct ovsdb_jsonrpc_session *s, struct ovsdb *db,
 
         /* Parse columns. */
         mr_value = node->data;
-        allocated_columns = 0;
         if (mr_value->type == JSON_ARRAY) {
             const struct json_array *array = &mr_value->u.array;
 
             for (i = 0; i < array->n; i++) {
-                error = ovsdb_jsonrpc_parse_monitor_request(
-                    m->dbmon, table, array->elems[i], &allocated_columns);
+                error = ovsdb_jsonrpc_parse_monitor_request(m->dbmon,
+                                                            table,
+                                                            array->elems[i]);
                 if (error) {
                     goto error;
                 }
             }
         } else {
-            error = ovsdb_jsonrpc_parse_monitor_request(
-                m->dbmon, table, mr_value, &allocated_columns);
+            error = ovsdb_jsonrpc_parse_monitor_request(m->dbmon,
+                                                        table,
+                                                        mr_value);
             if (error) {
                 goto error;
             }
diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
index 2645301..2da711f 100644
--- a/ovsdb/monitor.c
+++ b/ovsdb/monitor.c
@@ -75,6 +75,7 @@  struct jsonrpc_monitor_node {
 struct ovsdb_monitor_column {
     const struct ovsdb_column *column;
     enum ovsdb_monitor_selection select;
+    bool monitored;
 };
 
 /* A row that has changed in a monitored table. */
@@ -115,6 +116,8 @@  struct ovsdb_monitor_table {
     /* Columns being monitored. */
     struct ovsdb_monitor_column *columns;
     size_t n_columns;
+    size_t n_monitored_columns;
+    size_t allocated_columns;
 
     /* Columns in ovsdb_monitor_row have different indexes then in
      * ovsdb_row. This field maps between column->index to the index in the
@@ -203,6 +206,11 @@  compare_ovsdb_monitor_column(const void *a_, const void *b_)
     const struct ovsdb_monitor_column *a = a_;
     const struct ovsdb_monitor_column *b = b_;
 
+    /* put all monitored columns at the begining */
+    if (a->monitored != b->monitored) {
+        return a->monitored ? -1 : 1;
+    }
+
     return a->column < b->column ? -1 : a->column > b->column;
 }
 
@@ -378,23 +386,39 @@  ovsdb_monitor_add_column(struct ovsdb_monitor *dbmon,
                          const struct ovsdb_table *table,
                          const struct ovsdb_column *column,
                          enum ovsdb_monitor_selection select,
-                         size_t *allocated_columns)
+                         bool monitored)
 {
     struct ovsdb_monitor_table *mt;
     struct ovsdb_monitor_column *c;
 
     mt = shash_find_data(&dbmon->tables, table->schema->name);
 
-    if (mt->n_columns >= *allocated_columns) {
-        mt->columns = x2nrealloc(mt->columns, allocated_columns,
+    if (mt->n_columns >= mt->allocated_columns) {
+        mt->columns = x2nrealloc(mt->columns, &mt->allocated_columns,
                                  sizeof *mt->columns);
     }
 
+    /* Check duplication only for unmonitored columns.
+     * We do not remove unused unmonitored columns due to condition
+     * update */
+    if (!monitored) {
+        int i;
+        for (i = 0; i < mt->n_columns; i++) {
+            if (mt->columns[i].column == column) {
+                /* column exists */
+                return;
+            }
+        }
+    }
     mt->select |= select;
     mt->columns_index_map[column->index] = mt->n_columns;
     c = &mt->columns[mt->n_columns++];
     c->column = column;
     c->select = select;
+    c->monitored = monitored;
+    if (monitored) {
+        mt->n_monitored_columns++;
+    }
 }
 
 /* Check for duplicated column names. Return the first
@@ -577,7 +601,7 @@  ovsdb_monitor_compose_row_update(
     for (i = 0; i < mt->n_columns; i++) {
         const struct ovsdb_monitor_column *c = &mt->columns[i];
 
-        if (!(type & c->select)) {
+        if (!c->monitored || !(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). */
@@ -634,7 +658,7 @@  ovsdb_monitor_compose_row_update2(
         for (i = 0; i < mt->n_columns; i++) {
             const struct ovsdb_monitor_column *c = &mt->columns[i];
 
-            if (!(type & c->select)) {
+            if (!c->monitored || !(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). */
@@ -1017,19 +1041,21 @@  ovsdb_monitor_table_equal(const struct ovsdb_monitor_table *a,
 {
     size_t i;
 
+    ovs_assert(b->n_columns == b->n_monitored_columns);
+
     if ((a->table != b->table) ||
         (a->select != b->select) ||
-        (a->n_columns != b->n_columns)) {
+        (a->n_monitored_columns != b->n_monitored_columns)) {
         return false;
     }
 
-    for (i = 0; i < a->n_columns; i++) {
+    /* Compare only monitored columns that must be sorted already */
+    for (i = 0; i < a->n_monitored_columns; i++) {
         if ((a->columns[i].column != b->columns[i].column) ||
             (a->columns[i].select != b->columns[i].select)) {
             return false;
         }
     }
-
     return true;
 }
 
diff --git a/ovsdb/monitor.h b/ovsdb/monitor.h
index fb10435..4bc2705 100644
--- a/ovsdb/monitor.h
+++ b/ovsdb/monitor.h
@@ -58,7 +58,7 @@  void ovsdb_monitor_add_column(struct ovsdb_monitor *dbmon,
                               const struct ovsdb_table *table,
                               const struct ovsdb_column *column,
                               enum ovsdb_monitor_selection select,
-                              size_t *allocated_columns);
+                              bool monitored);
 
 const char * OVS_WARN_UNUSED_RESULT
 ovsdb_monitor_table_check_duplicates(struct ovsdb_monitor *,