[ovs-dev] [PATCH 1/2] ovsdb-idl: Make it possible to omit or pay less attention to columns.

Ben Pfaff blp at nicira.com
Wed Aug 11 22:26:44 UTC 2010


On Tue, Aug 10, 2010 at 11:49:46PM -0700, Justin Pettit wrote:
> On Jul 19, 2010, at 4:30 PM, Ben Pfaff wrote:
> 
> > +enum ovsdb_idl_mode {
> > +    /* Client reads and may write this column and wants to be alerted upon
> > +     * updates to it.
> > +     *
> > +     * This is the default. */
> > +    OVSDB_IDL_MODE_RW,
> > +
> > +    /* Client may read and write this column, but doesn't care to be alerted
> > +     * when it is updated.
> > +     *
> > +     * This is useful for columns that a client treats as "write-only", that
> > +     * is, it updates them but doesn't want to get alerted about its own
> > +     * updates.  It also won't be alerted about other clients' updates, so this
> > +     * is suitable only for use by a client that "owns" a particular column. */
> > +    OVSDB_IDL_MODE_WO,
> > +
> > +    /* Client won't read or write this column at all. */
> > +    OVSDB_IDL_MODE_NONE
> > +};
> 
> I don't believe that this code actually enforces that a client doesn't
> violate the mode; it only affects whether they're alerted about
> changes through a monitor.  I think it would be good to be more
> explicit about this--especially in the case of NONE.

Good point.  I added a comment for that case.  It's easy enough to
prevent writes to columns that have mode NONE, so I also added an
appropriate assertion in ovsdb_idl_txn_write().

> > +static void
> > +ovsdb_idl_set_mode(struct ovsdb_idl *idl,
> > +                   const struct ovsdb_idl_column *column,
> > +                   enum ovsdb_idl_mode mode)
> > +{
> > +    size_t i;
> > +
> > +    for (i = 0; i < idl->class->n_tables; i++) {
> > +        const struct ovsdb_idl_table *table = &idl->tables[i];
> > +        const struct ovsdb_idl_table_class *tc = table->class;
> > +
> > +        if (column >= tc->columns && column < &tc->columns[tc->n_columns]) {
> > +            unsigned char *modep = &table->modes[column - tc->columns];
> > +            assert(*modep == OVSDB_IDL_MODE_RW || *modep == mode);
> > +            *modep = mode;
> > +            return;
> > +        }
> > +    }
> 
> It may be nice to document that this function doesn't allow the mode
> to be changed once it's already been in a mode other than read-write.

This is just a static helper function for ovsdb_idl_set_write_only() and
ovsdb_idl_omit(), and each of those functions has a comment like "This
function is mutually exclusive with ovsdb_idl_omit() for any given
column."  So really I think that this should be adequately documented.

> > +/* By default, 'idl' replicates all of the columns in the remote database, and
> > + * ovsdb_idl_run() returns true upon a change to any column in the database.
> > + * Call this function to avoid alerting ovsdb_idl_run()'s caller upon changes
> > + * to 'column'.
> > + *
> > + * This is useful for columns that a client treats as "write-only", that is, it
> > + * updates them but doesn't want to get alerted about its own updates.  It also
> > + * won't be alerted about other clients' updates, so this is suitable only for
> > + * use by a client that "owns" a particular column.
> > + *
> > + * The client must be careful not to retain pointers to data in 'column' across
> > + * calls to ovsdb_idl_run(), even when that function returns false, because
> > + * the client is not alerted to changes.
> > + *
> > + * This function should be called after ovsdb_idl_create(), but before the
> > + * first call to ovsdb_idl_run().  This function is mutually exclusive with
> > + * ovsdb_idl_omit() for any given column. */
> > +void
> > +ovsdb_idl_set_write_only(struct ovsdb_idl *idl,
> > +                         const struct ovsdb_idl_column *column)
> > +{
> > +    ovsdb_idl_set_mode(idl, column, OVSDB_IDL_MODE_WO);
> > +}
> 
> It might be nice to note that there's no turning back once this
> function has been called, based on the assertion above.

Doesn't the comment already say that?

> > -static void
> > +static bool
> > ovsdb_idl_process_update(struct ovsdb_idl_table *table,
> 
> It may be nice to explain what the return type means.

OK, done.

> > -static void
> > +static bool
> > ovsdb_idl_row_update(struct ovsdb_idl_row *row, const struct json *row_json)
> 
> Ditto here.

OK, done.

> > /* When a row A refers to row B through a column with a "refTable" constraint,
> > @@ -787,13 +860,17 @@ ovsdb_idl_delete_row(struct ovsdb_idl_row *row)
> >     }
> > }
> > 
> > -static void
> > +static bool
> > ovsdb_idl_modify_row(struct ovsdb_idl_row *row, const struct json *row_json)
> 
> And here, too, Swayze.

OK, done.




More information about the dev mailing list