[ovs-dev] [idl 4/4] ovsdb-idl: Improve documentation.

Ben Pfaff blp at nicira.com
Wed Mar 28 00:00:56 UTC 2012


Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 lib/ovsdb-idl.c      |  126 ++++++++++++++++++++++++++++++++++++++++++++++++--
 lib/ovsdb-idl.h      |   51 ++++++++++++++++++++-
 python/ovs/db/idl.py |  116 ++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 280 insertions(+), 13 deletions(-)

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 5019c43..81f1548 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -357,8 +357,23 @@ ovsdb_idl_wait(struct ovsdb_idl *idl)
     jsonrpc_session_recv_wait(idl->session);
 }
 
-/* Returns a number that represents the state of 'idl'.  When 'idl' is updated
- * (by ovsdb_idl_run()), the return value changes. */
+/* Returns a "sequence number" that represents the state of 'idl'.  When
+ * ovsdb_idl_run() changes the database, the sequence number changes.  The
+ * initial fetch of the entire contents of the remote database is considered to
+ * be one kind of change.  Successfully acquiring a lock, if one has been
+ * configured with ovsdb_idl_set_lock(), is also considered to be a change.
+ *
+ * As long as the sequence number does not change, the client may continue to
+ * use any data structures it obtains from 'idl'.  But when it changes, the
+ * client must not access any of these data structures again, because they
+ * could have freed or reused for other purposes.
+ *
+ * The sequence number can occasionally change even if the database does not.
+ * This happens if the connection to the database drops and reconnects, which
+ * causes the database contents to be reloaded even if they didn't change.  (It
+ * could also happen if the database server sends out a "change" that reflects
+ * what the IDL already thought was in the database.  The database server is
+ * not supposed to do that, but bugs could in theory cause it to do so.) */
 unsigned int
 ovsdb_idl_get_seqno(const struct ovsdb_idl *idl)
 {
@@ -992,6 +1007,7 @@ ovsdb_idl_table_from_class(const struct ovsdb_idl *idl,
     return &idl->tables[table_class - idl->class->tables];
 }
 
+/* Called by ovsdb-idlc generated code. */
 struct ovsdb_idl_row *
 ovsdb_idl_get_row_arc(struct ovsdb_idl_row *src,
                       struct ovsdb_idl_table_class *dst_table_class,
@@ -1036,6 +1052,8 @@ ovsdb_idl_get_row_arc(struct ovsdb_idl_row *src,
     }
 }
 
+/* Searches 'tc''s table in 'idl' for a row with UUID 'uuid'.  Returns a
+ * pointer to the row if there is one, otherwise a null pointer.  */
 const struct ovsdb_idl_row *
 ovsdb_idl_get_row_for_uuid(const struct ovsdb_idl *idl,
                            const struct ovsdb_idl_table_class *tc,
@@ -1058,6 +1076,12 @@ next_real_row(struct ovsdb_idl_table *table, struct hmap_node *node)
     return NULL;
 }
 
+/* Returns a row in 'table_class''s table in 'idl', or a null pointer if that
+ * table is empty.
+ *
+ * Database tables are internally maintained as hash tables, so adding or
+ * removing rows while traversing the same table can cause some rows to be
+ * visited twice or not at apply. */
 const struct ovsdb_idl_row *
 ovsdb_idl_first_row(const struct ovsdb_idl *idl,
                     const struct ovsdb_idl_table_class *table_class)
@@ -1067,6 +1091,8 @@ ovsdb_idl_first_row(const struct ovsdb_idl *idl,
     return next_real_row(table, hmap_first(&table->rows));
 }
 
+/* Returns a row following 'row' within its table, or a null pointer if 'row'
+ * is the last row in its table. */
 const struct ovsdb_idl_row *
 ovsdb_idl_next_row(const struct ovsdb_idl_row *row)
 {
@@ -1143,6 +1169,11 @@ ovsdb_idl_row_is_synthetic(const struct ovsdb_idl_row *row)
 static void ovsdb_idl_txn_complete(struct ovsdb_idl_txn *txn,
                                    enum ovsdb_idl_txn_status);
 
+/* Returns a string representation of 'status'.  The caller must not modify or
+ * free the returned string.
+ *
+ * The return value is probably useful only for debug log messages and unit
+ * tests. */
 const char *
 ovsdb_idl_txn_status_to_string(enum ovsdb_idl_txn_status status)
 {
@@ -1167,6 +1198,9 @@ ovsdb_idl_txn_status_to_string(enum ovsdb_idl_txn_status status)
     return "<unknown>";
 }
 
+/* Starts a new transaction on 'idl'.  A given ovsdb_idl may only have a single
+ * active transaction at a time.  See the large comment in ovsdb-idl.h for
+ * general information on transactions. */
 struct ovsdb_idl_txn *
 ovsdb_idl_txn_create(struct ovsdb_idl *idl)
 {
@@ -1185,7 +1219,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);
 
@@ -1210,6 +1243,13 @@ ovsdb_idl_txn_add_comment(struct ovsdb_idl_txn *txn, const char *s, ...)
     va_end(args);
 }
 
+/* Marks 'txn' as a transaction that will not actually modify the database.  In
+ * almost every way, the transaction is treated like other transactions.  It
+ * must be committed or aborted like other transactions, it will be sent to the
+ * database server like other transactions, and so on.  The only difference is
+ * that the operations sent to the database server will include, as the last
+ * step, an "abort" operation, so that any changes made by the transaction will
+ * not actually take effect. */
 void
 ovsdb_idl_txn_set_dry_run(struct ovsdb_idl_txn *txn)
 {
@@ -1242,6 +1282,11 @@ ovsdb_idl_txn_increment(struct ovsdb_idl_txn *txn,
     txn->inc_row = row->uuid;
 }
 
+/* Destroys 'txn' and frees all associated memory.  If ovsdb_idl_txn_commit()
+ * has been called for 'txn' but the commit is still incomplete (that is, the
+ * last call returned TXN_INCOMPLETE) then the transaction may or may not still
+ * end up committing at the database server, but the client will not be able to
+ * get any further status information back. */
 void
 ovsdb_idl_txn_destroy(struct ovsdb_idl_txn *txn)
 {
@@ -1261,6 +1306,7 @@ ovsdb_idl_txn_destroy(struct ovsdb_idl_txn *txn)
     free(txn);
 }
 
+/* Causes poll_block() to wake up if 'txn' has completed committing. */
 void
 ovsdb_idl_txn_wait(const struct ovsdb_idl_txn *txn)
 {
@@ -1391,6 +1437,55 @@ ovsdb_idl_txn_disassemble(struct ovsdb_idl_txn *txn)
     hmap_init(&txn->txn_rows);
 }
 
+/* Attempts to commit 'txn'.  Returns the status of the commit operation, one
+ * of the following TXN_* constants:
+ *
+ *   TXN_INCOMPLETE:
+ *
+ *       The transaction is in progress, but not yet complete.  The caller
+ *       should call again later, after calling ovsdb_idl_run() to let the IDL
+ *       do OVSDB protocol processing.
+ *
+ *   TXN_UNCHANGED:
+ *
+ *       The transaction is complete.  (It didn't actually change the database,
+ *       so the IDL didn't send any request to the database server.)
+ *
+ *   TXN_ABORTED:
+ *
+ *       The caller previously called ovsdb_idl_txn_abort().
+ *
+ *   TXN_SUCCESS:
+ *
+ *       The transaction was successful.  The update made by the transaction
+ *       (and possibly other changes made by other database clients) should
+ *       already be visible in the IDL.
+ *
+ *   TXN_TRY_AGAIN:
+ *
+ *       The transaction failed for some transient reason, e.g. because a
+ *       "verify" operation reported an inconsistency or due to a network
+ *       problem.  The caller should wait for a change to the database, then
+ *       compose a new transaction, and commit the new transaction.
+ *
+ *       Use the return value of ovsdb_idl_get_seqno() to wait for a change in
+ *       the database.  It is important to use its return value *before* the
+ *       initial call to ovsdb_idl_txn_commit() as the baseline for this
+ *       purpose, because the change that one should wait for can happen after
+ *       the initial call but before the call that returns TXN_TRY_AGAIN, and
+ *       using some other baseline value in that situation could cause an
+ *       indefinite wait if the database rarely changes.
+ *
+ *   TXN_NOT_LOCKED:
+ *
+ *       The transaction failed because the IDL has been configured to require
+ *       a database lock (with ovsdb_idl_set_lock()) but didn't get it yet or
+ *       has already lost it.
+ *
+ * Committing a transaction rolls back all of the changes that it made to the
+ * IDL's copy of the database.  If the transaction commits successfully, then
+ * the database server will send an update and, thus, the IDL will be updated
+ * with the committed changes. */
 enum ovsdb_idl_txn_status
 ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn)
 {
@@ -1596,7 +1691,10 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn)
 
 /* Attempts to commit 'txn', blocking until the commit either succeeds or
  * fails.  Returns the final commit status, which may be any TXN_* value other
- * than TXN_INCOMPLETE. */
+ * than TXN_INCOMPLETE.
+ *
+ * This function calls ovsdb_idl_run() on 'txn''s IDL, so it may cause the
+ * return value of ovsdb_idl_get_seqno() to change. */
 enum ovsdb_idl_txn_status
 ovsdb_idl_txn_commit_block(struct ovsdb_idl_txn *txn)
 {
@@ -1612,6 +1710,9 @@ ovsdb_idl_txn_commit_block(struct ovsdb_idl_txn *txn)
     return status;
 }
 
+/* Returns the final (incremented) value of the column in 'txn' that was set to
+ * be incremented by ovsdb_idl_txn_increment().  'txn' must have committed
+ * successfully. */
 int64_t
 ovsdb_idl_txn_get_increment_new_value(const struct ovsdb_idl_txn *txn)
 {
@@ -1619,6 +1720,12 @@ ovsdb_idl_txn_get_increment_new_value(const struct ovsdb_idl_txn *txn)
     return txn->inc_new_value;
 }
 
+/* Aborts 'txn' without sending it to the database server.  This is effective
+ * only if ovsdb_idl_txn_commit() has not yet been called for 'txn'.
+ * Otherwise, it has no effect.
+ *
+ * Aborting a transaction doesn't free its memory.  Use
+ * ovsdb_idl_txn_destroy() to do that. */
 void
 ovsdb_idl_txn_abort(struct ovsdb_idl_txn *txn)
 {
@@ -1628,6 +1735,14 @@ ovsdb_idl_txn_abort(struct ovsdb_idl_txn *txn)
     }
 }
 
+/* Returns a string that reports the error status for 'txn'.  The caller must
+ * not modify or free the returned string.  A call to ovsdb_idl_txn_destroy()
+ * for 'txn' may free the returned string.
+ *
+ * The return value is ordinarily one of the strings that
+ * ovsdb_idl_txn_status_to_string() would return, but if the transaction failed
+ * due to an error reported by the database server, the return value is that
+ * error. */
 const char *
 ovsdb_idl_txn_get_error(const struct ovsdb_idl_txn *txn)
 {
@@ -2101,6 +2216,8 @@ ovsdb_idl_txn_process_reply(struct ovsdb_idl *idl,
     return true;
 }
 
+/* Returns the transaction currently active for 'row''s IDL.  A transaction
+ * must currently be active. */
 struct ovsdb_idl_txn *
 ovsdb_idl_txn_get(const struct ovsdb_idl_row *row)
 {
@@ -2109,6 +2226,7 @@ ovsdb_idl_txn_get(const struct ovsdb_idl_row *row)
     return txn;
 }
 
+/* Returns the IDL on which 'txn' acts. */
 struct ovsdb_idl *
 ovsdb_idl_txn_get_idl (struct ovsdb_idl_txn *txn)
 {
diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h
index 34ccf06..b7beccc 100644
--- a/lib/ovsdb-idl.h
+++ b/lib/ovsdb-idl.h
@@ -112,7 +112,56 @@ const struct ovsdb_datum *ovsdb_idl_get(const struct ovsdb_idl_row *,
 
 bool ovsdb_idl_row_is_synthetic(const struct ovsdb_idl_row *);
 
-/* Transactions. */
+/* Transactions.
+ *
+ * A transaction may modify the contents of a database by modifying the values
+ * of columns, deleting rows, inserting rows, or adding checks that columns in
+ * the database have not changed ("verify" operations), through
+ * ovsdb_idl_txn_*() functions.  (The OVSDB IDL code generator produces helper
+ * functions that internally call the ovsdb_idl_txn_*() functions.  These are
+ * likely to be more convenient.)
+ *
+ * Reading and writing columsn and inserting and deleting rows are all
+ * straightforward.  The reasons to verify columns are less obvious.
+ * Verification is the key to maintaining transactional integrity.  Because
+ * OVSDB handles multiple clients, it can happen that between the time that
+ * OVSDB client A reads a column and writes a new value, OVSDB client B has
+ * written that column.  Client A's write should not ordinarily overwrite
+ * client B's, especially if the column in question is a "map" column that
+ * contains several more or less independent data items.  If client A adds a
+ * "verify" operation before it writes the column, then the transaction fails
+ * in case client B modifies it first.  Client A will then see the new value of
+ * the column and compose a new transaction based on the new contents written
+ * by client B.
+ *
+ * When a transaction is complete, which must be before the next call to
+ * ovsdb_idl_run() on 'idl', call ovsdb_idl_txn_commit() or
+ * ovsdb_idl_txn_abort().
+ *
+ * The life-cycle of a transaction looks like this:
+ *
+ * 1. Create the transaction and record the initial sequence number:
+ *
+ *     seqno = ovsdb_idl_get_seqno(idl);
+ *     txn = ovsdb_idl_txn_create(idl);
+ *
+ * 2. Modify the database with ovsdb_idl_txn_*() functions directly or
+ *    indirectly.
+ *
+ * 3. Commit the transaction by calling ovsdb_idl_txn_commit().  The first call
+ *    to this function probably returns TXN_INCOMPLETE.  The client must keep
+ *    calling again along as this remains true, calling ovsdb_idl_run() in
+ *    between to let the IDL do protocol processing.  (If the client doesn't
+ *    have anything else to do in the meantime, it can use
+ *    ovsdb_idl_txn_commit_block() to avoid having to loop itself.)
+ *
+ * 4. If the final status is TXN_TRY_AGAIN, wait for ovsdb_idl_get_seqno() to
+ *    change from the saved 'seqno' (it's possible that it's already changed,
+ *    in which case the client should not wait at all), then start over from
+ *    step 1.  Only a call to ovsdb_idl_run() will change the return value of
+ *    ovsdb_idl_get_seqno().  (ovsdb_idl_txn_commit_block() calls
+ *    ovsdb_idl_run().)
+ */
 
 enum ovsdb_idl_txn_status {
     TXN_UNCOMMITTED,            /* Not yet committed or aborted. */
diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index f36ab03..4c11499 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -52,7 +52,13 @@ class Idl:
       'rows' map values.  Refer to Row for more details.
 
     - 'change_seqno': A number that represents the IDL's state.  When the IDL
-      is updated (by Idl.run()), its value changes.
+      is updated (by Idl.run()), its value changes.  The sequence number can
+      occasionally change even if the database does not.  This happens if the
+      connection to the database drops and reconnects, which causes the
+      database contents to be reloaded even if they didn't change.  (It could
+      also happen if the database server sends out a "change" that reflects
+      what the IDL already thought was in the database.  The database server is
+      not supposed to do that, but bugs could in theory cause it to do so.)
 
     - 'lock_name': The name of the lock configured with Idl.set_lock(), or None
       if no lock is configured.
@@ -635,6 +641,49 @@ class _InsertedRow(object):
 
 
 class Transaction(object):
+    """A transaction may modify the contents of a database by modifying the
+    values of columns, deleting rows, inserting rows, or adding checks that
+    columns in the database have not changed ("verify" operations), through
+    Row methods.
+
+    Reading and writing columsn and inserting and deleting rows are all
+    straightforward.  The reasons to verify columns are less obvious.
+    Verification is the key to maintaining transactional integrity.  Because
+    OVSDB handles multiple clients, it can happen that between the time that
+    OVSDB client A reads a column and writes a new value, OVSDB client B has
+    written that column.  Client A's write should not ordinarily overwrite
+    client B's, especially if the column in question is a "map" column that
+    contains several more or less independent data items.  If client A adds a
+    "verify" operation before it writes the column, then the transaction fails
+    in case client B modifies it first.  Client A will then see the new value
+    of the column and compose a new transaction based on the new contents
+    written by client B.
+
+    When a transaction is complete, which must be before the next call to
+    Idl.run(), call Transaction.commit() or Transaction.abort().
+
+    The life-cycle of a transaction looks like this:
+
+    1. Create the transaction and record the initial sequence number:
+
+        seqno = idl.change_seqno(idl)
+        txn = Transaction(idl)
+
+    2. Modify the database with Row and Transaction methods.
+
+    3. Commit the transaction by calling Transaction.commit().  The first call
+       to this function probably returns Transaction.INCOMPLETE.  The client
+       must keep calling again along as this remains true, calling Idl.run() in
+       between to let the IDL do protocol processing.  (If the client doesn't
+       have anything else to do in the meantime, it can use
+       Transaction.commit_block() to avoid having to loop itself.)
+
+    4. If the final status is Transaction.TRY_AGAIN, wait for Idl.change_seqno
+       to change from the saved 'seqno' (it's possible that it's already
+       changed, in which case the client should not wait at all), then start
+       over from step 1.  Only a call to Idl.run() will change the return value
+       of Idl.change_seqno.  (Transaction.commit_block() calls Idl.run().)"""
+
     # Status values that Transaction.commit() can return.
     UNCOMMITTED = "uncommitted"  # Not yet committed or aborted.
     UNCHANGED = "unchanged"      # Transaction didn't include any changes.
@@ -694,6 +743,8 @@ class Transaction(object):
         self._comments.append(comment)
 
     def wait(self, poller):
+        """Causes poll_block() to wake up if this transaction has completed
+        committing."""
         if self._status not in (Transaction.UNCOMMITTED,
                                 Transaction.INCOMPLETE):
             poller.immediate_wake()
@@ -722,16 +773,56 @@ class Transaction(object):
         self._txn_rows = {}
 
     def commit(self):
-        """Attempts to commit this transaction and returns the status of the
-        commit operation, one of the constants declared as class attributes.
-        If the return value is Transaction.INCOMPLETE, then the transaction is
-        not yet complete and the caller should try calling again later, after
-        calling Idl.run() to run the Idl.
+        """Attempts to commit 'txn'.  Returns the status of the commit
+        operation, one of the following constants:
+
+          Transaction.INCOMPLETE:
+
+              The transaction is in progress, but not yet complete.  The caller
+              should call again later, after calling Idl.run() to let the
+              IDL do OVSDB protocol processing.
+
+          Transaction.UNCHANGED:
+
+              The transaction is complete.  (It didn't actually change the
+              database, so the IDL didn't send any request to the database
+              server.)
+
+          Transaction.ABORTED:
+
+              The caller previously called Transaction.abort().
+
+          Transaction.SUCCESS:
+
+              The transaction was successful.  The update made by the
+              transaction (and possibly other changes made by other database
+              clients) should already be visible in the IDL.
+
+          Transaction.TRY_AGAIN:
+
+              The transaction failed for some transient reason, e.g. because a
+              "verify" operation reported an inconsistency or due to a network
+              problem.  The caller should wait for a change to the database,
+              then compose a new transaction, and commit the new transaction.
+
+              Use Idl.change_seqno to wait for a change in the database.  It is
+              important to use its value *before* the initial call to
+              Transaction.commit() as the baseline for this purpose, because
+              the change that one should wait for can happen after the initial
+              call but before the call that returns Transaction.TRY_AGAIN, and
+              using some other baseline value in that situation could cause an
+              indefinite wait if the database rarely changes.
+
+          Transaction.NOT_LOCKED:
+
+              The transaction failed because the IDL has been configured to
+              require a database lock (with Idl.set_lock()) but didn't
+              get it yet or has already lost it.
 
         Committing a transaction rolls back all of the changes that it made to
-        the Idl's copy of the database.  If the transaction commits
+        the IDL's copy of the database.  If the transaction commits
         successfully, then the database server will send an update and, thus,
-        the Idl will be updated with the committed changes."""
+        the IDL will be updated with the committed changes."""
         # The status can only change if we're the active transaction.
         # (Otherwise, our status will change only in Idl.run().)
         if self != self.idl.txn:
@@ -849,6 +940,12 @@ class Transaction(object):
         return self._status
 
     def commit_block(self):
+        """Attempts to commit this transaction, blocking until the commit
+        either succeeds or fails.  Returns the final commit status, which may
+        be any Transaction.* value other than Transaction.INCOMPLETE.
+
+        This function calls Idl.run() on this transaction'ss IDL, so it may
+        cause Idl.change_seqno to change."""
         while True:
             status = self.commit()
             if status != Transaction.INCOMPLETE:
@@ -862,6 +959,9 @@ class Transaction(object):
             poller.block()
 
     def get_increment_new_value(self):
+        """Returns the final (incremented) value of the column in this
+        transaction that was set to be incremented by Row.increment.  This
+        transaction must have committed successfully."""
         assert self._status == Transaction.SUCCESS
         return self._inc_new_value
 
-- 
1.7.2.5




More information about the dev mailing list