[ovs-dev] [PATCH] idl: Optionally warn when writing to read-write columns.

[Ethan Jackson]

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 <ethan at nicira.com>
---

Here's an updated version.


---
 lib/ovsdb-idl.c   | 23 ++++++++++++++++++++---
 lib/ovsdb-idl.h   |  1 +
 vswitchd/bridge.c |  1 +
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 6118852..ceea056 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -70,6 +70,7 @@ struct ovsdb_idl {
     struct json *monitor_request_id;
     unsigned int last_monitor_request_seqno;
     unsigned int change_seqno;
+    bool verify_write_only;
 
     /* Database locking. */
     char *lock_name;            /* Name of lock we need, NULL if none. */
@@ -402,6 +403,14 @@ ovsdb_idl_force_reconnect(struct ovsdb_idl *idl)
 {
     jsonrpc_session_force_reconnect(idl->session);
 }
+
+/* Causes 'idl' to reject writes to columns which are not marked write only
+ * using ovsdb_idl_omit_alert(). */
+void
+ovsdb_idl_verify_write_only(struct ovsdb_idl *idl)
+{
+    idl->verify_write_only = true;
+}
 
 static unsigned char *
 ovsdb_idl_get_mode(struct ovsdb_idl *idl,
@@ -1824,6 +1833,7 @@ ovsdb_idl_txn_write(const struct ovsdb_idl_row *row_,
     struct ovsdb_idl_row *row = CONST_CAST(struct ovsdb_idl_row *, row_);
     const struct ovsdb_idl_table_class *class;
     size_t column_idx;
+    bool write_only;
 
     if (ovsdb_idl_row_is_synthetic(row)) {
         ovsdb_datum_destroy(datum, &column->type);
@@ -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;
 
     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("Bug: Attempt to write to a read/write column (%s:%s) when"
+                 " explicitly configured not to.", class->name, column->name);
+        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)) {
+    if (write_only && ovsdb_datum_equals(ovsdb_idl_read(row, column),
+                                         datum, &column->type)) {
         ovsdb_datum_destroy(datum, &column->type);
         return;
     }
diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h
index c48ad1b..27008b7 100644
--- a/lib/ovsdb-idl.h
+++ b/lib/ovsdb-idl.h
@@ -57,6 +57,7 @@ bool ovsdb_idl_is_lock_contended(const struct ovsdb_idl *);
 unsigned int ovsdb_idl_get_seqno(const struct ovsdb_idl *);
 bool ovsdb_idl_has_ever_connected(const struct ovsdb_idl *);
 void ovsdb_idl_force_reconnect(struct ovsdb_idl *);
+void ovsdb_idl_verify_write_only(struct ovsdb_idl *);
 
 /* Choosing columns and tables to replicate. */
 
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 940e5e7..d161c4c 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -274,6 +274,7 @@ bridge_init(const char *remote)
     idl = ovsdb_idl_create(remote, &ovsrec_idl_class, true);
     idl_seqno = ovsdb_idl_get_seqno(idl);
     ovsdb_idl_set_lock(idl, "ovs_vswitchd");
+    ovsdb_idl_verify_write_only(idl);
 
     ovsdb_idl_omit_alert(idl, &ovsrec_open_vswitch_col_cur_cfg);
     ovsdb_idl_omit_alert(idl, &ovsrec_open_vswitch_col_statistics);
-- 
1.7.12