Ben Pfaff <b...@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 <lir...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
