ovsdb-idl: Improve ovsdb_idl_txn_increment() interface.

The previous interface was just bizarre.

Signed-off-by: Ben Pfaff <blp@nicira.com>

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 27a957605ce511933fb4d9e70262f00b332d9a8e..dcbad1054153e1877f2acdf011c5d2a3cd3a3776 100644 (file)
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -94,9 +94,9 @@ struct ovsdb_idl_txn {
     unsigned int commit_seqno;
 
     /* Increments. */
-    char *inc_table;
-    char *inc_column;
-    struct json *inc_where;
+    const char *inc_table;
+    const char *inc_column;
+    struct uuid inc_row;
     unsigned int inc_index;
     int64_t inc_new_value;
 
@@ -1185,7 +1185,6 @@ ovsdb_idl_txn_create(struct ovsdb_idl *idl)
 
     txn->inc_table = NULL;
     txn->inc_column = NULL;
-    txn->inc_where = NULL;
 
     hmap_init(&txn->inserted_rows);
 
@@ -1216,14 +1215,30 @@ ovsdb_idl_txn_set_dry_run(struct ovsdb_idl_txn *txn)
     txn->dry_run = true;
 }
 
+/* Causes 'txn', when committed, to increment the value of 'column' within
+ * 'row' by 1.  'column' must have an integer type.  After 'txn' commits
+ * successfully, the client may retrieve the final (incremented) value of
+ * 'column' with ovsdb_idl_txn_get_increment_new_value().
+ *
+ * The client could accomplish something similar with ovsdb_idl_read(),
+ * ovsdb_idl_txn_verify() and ovsdb_idl_txn_write(), or with ovsdb-idlc
+ * generated wrappers for these functions.  However, ovsdb_idl_txn_increment()
+ * will never (by itself) fail because of a verify error.
+ *
+ * The intended use is for incrementing the "next_cfg" column in the
+ * Open_vSwitch table. */
 void
-ovsdb_idl_txn_increment(struct ovsdb_idl_txn *txn, const char *table,
-                        const char *column, const struct json *where)
+ovsdb_idl_txn_increment(struct ovsdb_idl_txn *txn,
+                        const struct ovsdb_idl_row *row,
+                        const struct ovsdb_idl_column *column)
 {
     assert(!txn->inc_table);
-    txn->inc_table = xstrdup(table);
-    txn->inc_column = xstrdup(column);
-    txn->inc_where = where ? json_clone(where) : json_array_create_empty();
+    assert(column->type.key.type == OVSDB_TYPE_INTEGER);
+    assert(column->type.value.type == OVSDB_TYPE_VOID);
+
+    txn->inc_table = row->table->class->name;
+    txn->inc_column = column->name;
+    txn->inc_row = row->uuid;
 }
 
 void
@@ -1238,9 +1253,6 @@ ovsdb_idl_txn_destroy(struct ovsdb_idl_txn *txn)
     ovsdb_idl_txn_abort(txn);
     ds_destroy(&txn->comment);
     free(txn->error);
-    free(txn->inc_table);
-    free(txn->inc_column);
-    json_destroy(txn->inc_where);
     HMAP_FOR_EACH_SAFE (insert, next, hmap_node, &txn->inserted_rows) {
         free(insert);
     }
@@ -1528,7 +1540,8 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn)
         json_object_put_string(op, "op", "mutate");
         json_object_put_string(op, "table", txn->inc_table);
         json_object_put(op, "where",
-                        substitute_uuids(json_clone(txn->inc_where), txn));
+                        substitute_uuids(where_uuid_equals(&txn->inc_row),
+                                         txn));
         json_object_put(op, "mutations",
                         json_array_create_1(
                             json_array_create_3(
@@ -1541,7 +1554,8 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn)
         json_object_put_string(op, "op", "select");
         json_object_put_string(op, "table", txn->inc_table);
         json_object_put(op, "where",
-                        substitute_uuids(json_clone(txn->inc_where), txn));
+                        substitute_uuids(where_uuid_equals(&txn->inc_row),
+                                         txn));
         json_object_put(op, "columns",
                         json_array_create_1(json_string_create(
                                                 txn->inc_column)));

diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h
index 9b49e623a15b250d81f73148b1e6b8f93bc414e7..34ccf061931cb7542f56a2e7e7f72f551347d2aa 100644 (file)
--- a/lib/ovsdb-idl.h
+++ b/lib/ovsdb-idl.h
@@ -134,8 +134,9 @@ struct ovsdb_idl_txn *ovsdb_idl_txn_create(struct ovsdb_idl *);
 void ovsdb_idl_txn_add_comment(struct ovsdb_idl_txn *, const char *, ...)
     PRINTF_FORMAT (2, 3);
 void ovsdb_idl_txn_set_dry_run(struct ovsdb_idl_txn *);
-void ovsdb_idl_txn_increment(struct ovsdb_idl_txn *, const char *table,
-                             const char *column, const struct json *where);
+void ovsdb_idl_txn_increment(struct ovsdb_idl_txn *,
+                             const struct ovsdb_idl_row *,
+                             const struct ovsdb_idl_column *);
 void ovsdb_idl_txn_destroy(struct ovsdb_idl_txn *);
 void ovsdb_idl_txn_wait(const struct ovsdb_idl_txn *);
 enum ovsdb_idl_txn_status ovsdb_idl_txn_commit(struct ovsdb_idl_txn *);

diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index 9760fc64d319b6d7ca279b7605abefad7b26e211..fba20dcb22b56d56661bbc2e18404592a4f152cd 100644 (file)
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -604,6 +604,21 @@ class Row(object):
         self.__dict__["_changes"] = None
         del self._table.rows[self.uuid]
 
+    def increment(self, column_name):
+        """Causes the transaction, when committed, to increment the value of
+        'column_name' within this row by 1.  'column_name' must have an integer
+        type.  After the transaction commits successfully, the client may
+        retrieve the final (incremented) value of 'column_name' with
+        Transaction.get_increment_new_value().
+
+        The client could accomplish something similar by reading and writing
+        and verify()ing columns.  However, increment() will never (by itself)
+        cause a transaction to fail because of a verify error.
+
+        The intended use is for incrementing the "next_cfg" column in
+        the Open_vSwitch table."""
+        self._idl.txn._increment(self, column_name)
+
 
 def _uuid_name_from_uuid(uuid):
     return "row%s" % str(uuid).replace("-", "_")
@@ -666,9 +681,8 @@ class Transaction(object):
         self._comments = []
         self._commit_seqno = self.idl.change_seqno
 
-        self._inc_table = None
+        self._inc_row = None
         self._inc_column = None
-        self._inc_where = None
 
         self._inserted_rows = {}  # Map from UUID to _InsertedRow
 
@@ -679,12 +693,6 @@ class Transaction(object):
         relatively human-readable form.)"""
         self._comments.append(comment)
 
-    def increment(self, table, column, where):
-        assert not self._inc_table
-        self._inc_table = table
-        self._inc_column = column
-        self._inc_where = where
-
     def wait(self, poller):
         if self._status not in (Transaction.UNCOMMITTED,
                                 Transaction.INCOMPLETE):
@@ -803,18 +811,18 @@ class Transaction(object):
                     operations.append(op)
 
         # Add increment.
-        if self._inc_table and any_updates:
+        if self._inc_row and any_updates:
             self._inc_index = len(operations) - 1
 
             operations.append({"op": "mutate",
-                               "table": self._inc_table,
+                               "table": self._inc_row._table.name,
                                "where": self._substitute_uuids(
-                                   self._inc_where),
+                                   _where_uuid_equals(self._inc_row.uuid)),
                                "mutations": [[self._inc_column, "+=", 1]]})
             operations.append({"op": "select",
-                               "table": self._inc_table,
+                               "table": self._inc_row._table.name,
                                "where": self._substitute_uuids(
-                                   self._inc_where),
+                                   _where_uuid_equals(self._inc_row.uuid)),
                                "columns": [self._inc_column]})
 
         # Add comment.
@@ -898,6 +906,11 @@ class Transaction(object):
             return inserted_row.real
         return None
 
+    def _increment(self, row, column):
+        assert not self._inc_row
+        self._inc_row = row
+        self._inc_column = column
+
     def _write(self, row, column, datum):
         assert row._changes is not None
 
@@ -978,7 +991,7 @@ class Transaction(object):
                     vlog.warn("operation reply is not JSON null or object")
 
             if not soft_errors and not hard_errors and not lock_errors:
-                if self._inc_table and not self.__process_inc_reply(ops):
+                if self._inc_row and not self.__process_inc_reply(ops):
                     hard_errors = True
 
                 for insert in self._inserted_rows.itervalues():

diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index 2cfc91c6b62ddf91a52e6f12fa4d83c289f9bd14..91f167197b535d7dfa1104ab9d2efaca61f2f23c 100644 (file)
--- a/tests/ovsdb-idl.at
+++ b/tests/ovsdb-idl.at
@@ -221,7 +221,7 @@ OVSDB_CHECK_IDL([simple idl, handling verification failure],
 000: i=1 r=2 b=false s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<2>
 001: commit, status=success
 002: {"error":null,"result":[{"count":1}]}
-003: commit, status=try again now
+003: commit, status=try again
 004: i=0 r=0 b=true s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1>
 004: i=1 r=5 b=false s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<2>
 005: commit, status=success
@@ -235,7 +235,7 @@ OVSDB_CHECK_IDL([simple idl, increment operation],
       {"op": "insert",
        "table": "simple",
        "row": {}}]']],
-  [['set 0 r 2.0, increment simple i']],
+  [['set 0 r 2.0, increment 0']],
   [[000: i=0 r=0 b=false s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1>
 001: commit, status=success, increment=1
 002: i=1 r=2 b=false s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1>

diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
index cc01bbe3410323b2feab78796d54cf46fc1de71c..4e2e416cf1cd3cc9dda1979d2c04ae2ce63e4948 100644 (file)
--- a/tests/test-ovsdb.c
+++ b/tests/test-ovsdb.c
@@ -1825,10 +1825,19 @@ idl_set(struct ovsdb_idl *idl, char *commands, int step)
                           arg2);
             }
         } else if (!strcmp(name, "increment")) {
-            if (!arg2 || arg3) {
-                ovs_fatal(0, "\"increment\" command requires 2 arguments");
+            const struct idltest_simple *s;
+
+            if (!arg1 || arg2) {
+                ovs_fatal(0, "\"increment\" command requires 1 argument");
             }
-            ovsdb_idl_txn_increment(txn, arg1, arg2, NULL);
+
+            s = idltest_find_simple(idl, atoi(arg1));
+            if (!s) {
+                ovs_fatal(0, "\"set\" command asks for nonexistent "
+                          "i=%d", atoi(arg1));
+            }
+
+            ovsdb_idl_txn_increment(txn, &s->header_, &idltest_simple_col_i);
             increment = true;
         } else if (!strcmp(name, "abort")) {
             ovsdb_idl_txn_abort(txn);

diff --git a/tests/test-ovsdb.py b/tests/test-ovsdb.py
index b0e42a35dc764ea3d1ae8a28006e73b9eec21744..77b3a2c1932c2056b48a333df9479cd5df4b9029 100644 (file)
--- a/tests/test-ovsdb.py
+++ b/tests/test-ovsdb.py
@@ -1,4 +1,4 @@
-# Copyright (c) 2009, 2010, 2011 Nicira Networks
+# Copyright (c) 2009, 2010, 2011, 2012 Nicira Networks
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -292,11 +292,17 @@ def idl_set(idl, commands, step):
                                  '"%s"\n' % args[1])
                 sys.exit(1)
         elif name == "increment":
-            if len(args) != 2:
-                sys.stderr.write('"increment" command requires 2 arguments\n')
+            if len(args) != 1:
+                sys.stderr.write('"increment" command requires 1 argument\n')
+                sys.exit(1)
+
+            s = idltest_find_simple(idl, int(args[0]))
+            if not s:
+                sys.stderr.write('"set" command asks for nonexistent i=%d\n'
+                                 % int(args[0]))
                 sys.exit(1)
 
-            txn.increment(args[0], args[1], [])
+            s.increment("i")
             increment = True
         elif name == "abort":
             txn.abort()

diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index d22a518dd39f34522311c924a8a006c87154e03b..a5bd64741af72299f05b675260344c1c0b521a49 100644 (file)
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -3605,20 +3605,6 @@ cmd_wait_until(struct vsctl_context *ctx)
     }
 }
 \f
-static struct json *
-where_uuid_equals(const struct uuid *uuid)
-{
-    return
-        json_array_create_1(
-            json_array_create_3(
-                json_string_create("_uuid"),
-                json_string_create("=="),
-                json_array_create_2(
-                    json_string_create("uuid"),
-                    json_string_create_nocopy(
-                        xasprintf(UUID_FMT, UUID_ARGS(uuid))))));
-}
-
 static void
 vsctl_context_init(struct vsctl_context *ctx, struct vsctl_command *command,
                    struct ovsdb_idl *idl, struct ovsdb_idl_txn *txn,
@@ -3701,9 +3687,8 @@ do_vsctl(const char *args, struct vsctl_command *commands, size_t n_commands,
     }
 
     if (wait_for_reload) {
-        struct json *where = where_uuid_equals(&ovs->header_.uuid);
-        ovsdb_idl_txn_increment(txn, "Open_vSwitch", "next_cfg", where);
-        json_destroy(where);
+        ovsdb_idl_txn_increment(txn, &ovs->header_,
+                                &ovsrec_open_vswitch_col_next_cfg);
     }
 
     symtab = ovsdb_symbol_table_create();