On Thu, Sep 20, 2012 at 02:48:54PM -0700, Ethan Jackson wrote:
> ovs-vswitchd should only write to write-only columns. Furthermore,
> writing to a column which is not write-only can cause serious
> performance degradations. This patch causes ovs-vswitchd to log
> and reject writes to read-write columns.
>
> Signed-off-by: Ethan Jackson <[email protected]>
Thanks! This should be really helpful. I did notice a couple of
things, see below.
> @@ -1832,12 +1842,20 @@ ovsdb_idl_txn_write(const struct ovsdb_idl_row *row_,
>
> class = row->table->class;
> column_idx = column - class->columns;
> + write_only = row->table->modes[column_idx] & OVSDB_IDL_MONITOR;
I think that at the least the name here is misleading. The
OVSDB_IDL_MONITOR flag is set if the IDL will monitor a particular
column. Thus, it is set for read-only, write-only, and (if we really
did this) read/write columns.
To test for a write-only column, use == instead of &.
> assert(row->new != NULL);
> assert(column_idx < class->n_columns);
> assert(row->old == NULL ||
> row->table->modes[column_idx] & OVSDB_IDL_MONITOR);
>
> + if (row->table->idl->verify_write_only && !write_only) {
> + VLOG_ERR("Attempt to write to a read/write column (%s:%s) when"
> + " explicitly configured not to.", class->name,
> column->name);
It might be nice to add something like "internal error" or "bug"
somewhere in there, to make it clear that this isn't related to anything
the user did.
> + ovsdb_datum_destroy(datum, &column->type);
> + return;
> + }
> +
> /* If this is a write-only column and the datum being written is the same
> * as the one already there, just skip the update entirely. This is
> worth
> * optimizing because we have a lot of columns that get periodically
> @@ -1849,9 +1867,8 @@ ovsdb_idl_txn_write(const struct ovsdb_idl_row *row_,
> * transaction only does writes of existing values, without making any
> real
> * changes, we will drop the whole transaction later in
> * ovsdb_idl_txn_commit().) */
> - if (row->table->modes[column_idx] == OVSDB_IDL_MONITOR
> - && ovsdb_datum_equals(ovsdb_idl_read(row, column),
> - datum, &column->type)) {
(See, there's the correct test.)
> + if (write_only && ovsdb_datum_equals(ovsdb_idl_read(row, column),
> + datum, &column->type)) {
> ovsdb_datum_destroy(datum, &column->type);
> return;
> }
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
