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

Justin Pettit jpettit at nicira.com
Wed Aug 11 06:49:46 UTC 2010


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.

> +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.

> +/* 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.

> +/* By default, 'idl' replicates all of the columns in the remote database.
> + * Call this function to omit replicating 'column'.  This saves CPU time and
> + * bandwidth to the database.
> + *
> + * This function should be called after ovsdb_idl_create(), but before the
> + * first call to ovsdb_idl_run().
> + *
> + * 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_set_write_only() for any given column. */
> +void
> +ovsdb_idl_omit(struct ovsdb_idl *idl, const struct ovsdb_idl_column *column)
> +{
> +    ovsdb_idl_set_mode(idl, column, OVSDB_IDL_MODE_NONE);
> +}

Ditto here.

> -static void
> +static bool
> ovsdb_idl_process_update(struct ovsdb_idl_table *table,

It may be nice to explain what the return type means.

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

Ditto here.

> /* 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.

--Justin






More information about the dev mailing list