[ovs-dev] [PATCH monitor_cond V3 03/10] ovsdb: allow non-monitored columns for condition evaluation

Andy Zhou azhou at ovn.org
Fri Feb 5 08:10:59 UTC 2016


On Wed, Feb 3, 2016 at 5:53 AM, Liran Schour <lirans at il.ibm.com> wrote:

> This commit allows to add non-monitored columns to a monitored table.
> It will be used to evaluate conditions on non-monitored columns.
> Update notification includes only monitored columns.
>
> Signed-off-by: Liran Schour <lirans at il.ibm.com>
> ---
>  ovsdb/jsonrpc-server.c | 23 +++++++++++------------
>  ovsdb/monitor.c        | 30 ++++++++++++++++++++++--------
>  ovsdb/monitor.h        |  2 +-
>  3 files changed, 34 insertions(+), 21 deletions(-)
>
> diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
> index 2065702..fcda8dc 100644
> --- a/ovsdb/jsonrpc-server.c
> +++ b/ovsdb/jsonrpc-server.c
> @@ -1077,8 +1077,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;
> @@ -1142,8 +1141,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;
> @@ -1151,8 +1150,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);
>              }
>          }
>      }
> @@ -1202,7 +1201,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;
>
> @@ -1217,20 +1215,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 39422d9..7802560 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;
> +    }
>
What is the reason for above logic?   Strictly speaking, this seems to be
bug that may cause it caller
ovsdb_monitor_table_check_duplicates() to fail to detect duplication when
both monitored columns and non-monitored
are mixed.   It so happens that ovsdb_monitor_table_check_duplicates() are
only called after monitored columns are inserted,
 so all columns should be monitored when this function is called  -- then
the above logic seems redundant.

On the other hand, condition columns are added and checked using a
different (and less efficient) way. It would be great if we can somehow
refactor  these logics to remove some duplications.

>From user perspective,  Any duplications in conditions columns are not
reported to the user, but silently ignored. Is this intentional?

+
>      return a->column < b->column ? -1 : a->column > b->column;
>  }
>
> @@ -378,15 +386,15 @@ 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);
>      }
>
> @@ -395,6 +403,10 @@ ovsdb_monitor_add_column(struct ovsdb_monitor *dbmon,
>      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 +589,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))  {
>
If monitored only columns 'select' are always OJMS_NONE, is the
!c->monitored check redundant?

>              /* 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 +646,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 +1029,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;
>          }
>      }
> -

It seems we are going out to way trying to share monitors that shares only
the monitored columns. Is this the common case?
I'd think the common case will be all columns are the same (monitored or
not), but the condition expression will be different.
If true, then the logic may be much simpler.  Please correct me if I am
wrong.

>



     return true;
>  }
>
> diff --git a/ovsdb/monitor.h b/ovsdb/monitor.h
> index d6e9635..1f3dc6e 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 *,
> --
> 2.1.4
>
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>



More information about the dev mailing list