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

[Liran Schour]

Ben Pfaff <blp at 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 at 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.