| Message ID | 1463495229-26258-2-git-send-email-lirans@il.ibm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, May 17, 2016 at 05:26:59PM +0300, Liran Schour wrote: > Columns indexing is different in ovsdb_row then in ovsdb_monitor_row. > We need mapping between the 2 for condition evaluation. > > signed-off-by: Liran Schour <lirans@il.ibm.com> In ovsdb_monitor_table_columns_sort(), I don't understand why this loop starts at 1 instead of 0: > + qsort(mt->columns, mt->n_columns, sizeof *mt->columns, > + compare_ovsdb_monitor_column); > + for (i = 1; i < mt->n_columns; i++) { > + /* re-set index map due to sort */ > + mt->columns_index_map[mt->columns[i].column->index] = i; > + } In ovsdb_monitor_add_table(), please use sizeof *mt->columns_index_map instead of sizeof(unsigned int), following CodingStyle.md: + mt->columns_index_map = + xmalloc(sizeof(unsigned int) * shash_count(&table->schema->columns)); Also in ovsdb_monitor_add_table(), I think that the code would be a little easier to read if shash_count(&table->schema->columns) were put into a variables, e.g. n_columns. This new columns_index_map means that it's no longer necessary to work so hard to find duplicates--rather, we can find a duplicate at the time we try to add a column: a new column is a duplicate if columns_index_map[column->index] != -1 at the time we try to add it. It would probably be nicer to simply report the error at the time we add it, instead of adding a sort step and a re-indexing step. Please add a period at the end of this comment, to make it look like a complete thought: + /* Columns in ovsdb_monitor_row have different indexes then in + * ovsdb_row. This field maps between column->index to the index in the + * ovsdb_monitor_row. It is used for condition evaluation */ Thanks, Ben.
Ben Pfaff <blp@ovn.org> wrote on 01/06/2016 09:18:46 PM: > On Tue, May 17, 2016 at 05:26:59PM +0300, Liran Schour wrote: > > Columns indexing is different in ovsdb_row then in ovsdb_monitor_row. > > We need mapping between the 2 for condition evaluation. > > > > signed-off-by: Liran Schour <lirans@il.ibm.com> > > In ovsdb_monitor_table_columns_sort(), I don't understand why this loop > starts at 1 instead of 0: > > + qsort(mt->columns, mt->n_columns, sizeof *mt->columns, > > + compare_ovsdb_monitor_column); > > + for (i = 1; i < mt->n_columns; i++) { > > + /* re-set index map due to sort */ > > + mt->columns_index_map[mt->columns[i].column->index] = i; > > + } You are right. Should start from 0. > In ovsdb_monitor_add_table(), please use sizeof *mt->columns_index_map > instead of sizeof(unsigned int), following CodingStyle.md: > + mt->columns_index_map = > + xmalloc(sizeof(unsigned int) * shash_count(&table->schema->columns)); > Will fix that. > Also in ovsdb_monitor_add_table(), I think that the code would be a > little easier to read if shash_count(&table->schema->columns) were put > into a variables, e.g. n_columns. > Will do that. > This new columns_index_map means that it's no longer necessary to work > so hard to find duplicates--rather, we can find a duplicate at the time > we try to add a column: a new column is a duplicate if > columns_index_map[column->index] != -1 at the time we try to add it. It > would probably be nicer to simply report the error at the time we add > it, instead of adding a sort step and a re-indexing step. > Will change the code to find column duplication on column add. > Please add a period at the end of this comment, to make it look like a > complete thought: > + /* Columns in ovsdb_monitor_row have different indexes then in > + * ovsdb_row. This field maps between column->index to the index in the > + * ovsdb_monitor_row. It is used for condition evaluation */ > > Thanks, > > Ben. > Sure. Thanks, - Liran
On Mon, Jun 06, 2016 at 01:11:44PM +0300, Liran Schour wrote: > Ben Pfaff <blp@ovn.org> wrote on 01/06/2016 09:18:46 PM: > > This new columns_index_map means that it's no longer necessary to work > > so hard to find duplicates--rather, we can find a duplicate at the time > > we try to add a column: a new column is a duplicate if > > columns_index_map[column->index] != -1 at the time we try to add it. It > > would probably be nicer to simply report the error at the time we add > > it, instead of adding a sort step and a re-indexing step. > > Will change the code to find column duplication on column add. When I kept reading in later patches, I realized that there is some value in sorting the columns because it makes it easier to compare two sets of columns for equality. I don't know whether this affects when it makes sense to detect duplicates.
Ben Pfaff <blp@ovn.org> wrote on 06/06/2016 06:13:44 PM: > On Mon, Jun 06, 2016 at 01:11:44PM +0300, Liran Schour wrote: > > Ben Pfaff <blp@ovn.org> wrote on 01/06/2016 09:18:46 PM: > > > This new columns_index_map means that it's no longer necessary to work > > > so hard to find duplicates--rather, we can find a duplicate at the time > > > we try to add a column: a new column is a duplicate if > > > columns_index_map[column->index] != -1 at the time we try to add it. It > > > would probably be nicer to simply report the error at the time we add > > > it, instead of adding a sort step and a re-indexing step. > > > > Will change the code to find column duplication on column add. > > When I kept reading in later patches, I realized that there is some > value in sorting the columns because it makes it easier to compare two > sets of columns for equality. I don't know whether this affects when it > makes sense to detect duplicates. > Right. Changed the code that duplication of column is detected on ovsdb_add_column(). Sorting columns set and then comparing with other sets is down under ovsdb_monitor_add().
diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c index e910e3f..8991712 100644 --- a/ovsdb/monitor.c +++ b/ovsdb/monitor.c @@ -116,6 +116,11 @@ struct ovsdb_monitor_table { struct ovsdb_monitor_column *columns; size_t n_columns; + /* Columns in ovsdb_monitor_row have different indexes then in + * ovsdb_row. This field maps between column->index to the index in the + * ovsdb_monitor_row. It is used for condition evaluation */ + unsigned int *columns_index_map; + /* Contains 'ovsdb_monitor_changes' indexed by 'transaction'. */ struct hmap changes; }; @@ -304,6 +309,19 @@ ovsdb_monitor_row_destroy(const struct ovsdb_monitor_table *mt, } } +static void +ovsdb_monitor_table_columns_sort(const struct ovsdb_monitor_table *mt) +{ + int i; + + qsort(mt->columns, mt->n_columns, sizeof *mt->columns, + compare_ovsdb_monitor_column); + for (i = 1; i < mt->n_columns; i++) { + /* re-set index map due to sort */ + mt->columns_index_map[mt->columns[i].column->index] = i; + } +} + void ovsdb_monitor_add_jsonrpc_monitor(struct ovsdb_monitor *dbmon, struct ovsdb_jsonrpc_monitor *jsonrpc_monitor) @@ -341,11 +359,17 @@ ovsdb_monitor_add_table(struct ovsdb_monitor *m, const struct ovsdb_table *table) { struct ovsdb_monitor_table *mt; + int i; mt = xzalloc(sizeof *mt); mt->table = table; shash_add(&m->tables, table->schema->name, mt); hmap_init(&mt->changes); + mt->columns_index_map = + xmalloc(sizeof(unsigned int) * shash_count(&table->schema->columns)); + for (i = 0; i < shash_count(&table->schema->columns); i++) { + mt->columns_index_map[i] = -1; + } } void @@ -366,6 +390,7 @@ ovsdb_monitor_add_column(struct ovsdb_monitor *dbmon, } mt->select |= select; + mt->columns_index_map[column->index] = mt->n_columns; c = &mt->columns[mt->n_columns++]; c->column = column; c->select = select; @@ -385,8 +410,7 @@ ovsdb_monitor_table_check_duplicates(struct ovsdb_monitor *m, if (mt) { /* Check for duplicate columns. */ - qsort(mt->columns, mt->n_columns, sizeof *mt->columns, - compare_ovsdb_monitor_column); + ovsdb_monitor_table_columns_sort(mt); for (i = 1; i < mt->n_columns; i++) { if (mt->columns[i].column == mt->columns[i - 1].column) { return mt->columns[i].column->name; @@ -1112,6 +1136,7 @@ ovsdb_monitor_destroy(struct ovsdb_monitor *dbmon) } hmap_destroy(&mt->changes); free(mt->columns); + free(mt->columns_index_map); free(mt); } shash_destroy(&dbmon->tables);
Columns indexing is different in ovsdb_row then in ovsdb_monitor_row. We need mapping between the 2 for condition evaluation. signed-off-by: Liran Schour <lirans@il.ibm.com> --- ovsdb/monitor.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-)