[ovs-dev,05/15] ovsdb: Drop distinction between monitors and replicas.

Message ID 20180101051640.13043-5-blp@ovn.org
State Accepted
Delegated to: Justin Pettit
Headers show
Series
  • [ovs-dev,01/15] log: Add async commit support.
Related show

Commit Message

Ben Pfaff Jan. 1, 2018, 5:16 a.m.
Until now, OVSDB distinguished "monitors", which are associated with OVSDB
JSON-RPC client sessions and allow clients to find out about database
changes, from "replicas", which are associated with databases and also find
out about database changes and act on them in some way.  Now that
committing to disk has been broken into a separate concept, there is a
one-to-one and "onto" relationship between monitors and replicas: every
monitor M has a replica R and R is associated with M as well.  It's easier
if we just treat them as a single entity, and that's what this commit
implements.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ovsdb/monitor.c     | 56 +++++++++++++++++++++++++----------------------------
 ovsdb/monitor.h     |  8 +++++++-
 ovsdb/ovsdb.c       | 32 ++++--------------------------
 ovsdb/ovsdb.h       | 21 +-------------------
 ovsdb/transaction.c |  6 ++----
 5 files changed, 40 insertions(+), 83 deletions(-)

Comments

Justin Pettit Jan. 13, 2018, 2:24 a.m. | #1
> On Dec 31, 2017, at 9:16 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
> index b2ecd109ed60..3e58c3fbd274 100644
> --- a/ovsdb/monitor.c
> +++ b/ovsdb/monitor.c
> 
> +void
> +ovsdb_monitors_remove(struct ovsdb *db)
> +{
> +    struct ovsdb_monitor *m, *next_m;
> +
> +    LIST_FOR_EACH_SAFE (m, next_m, list_node, &db->monitors) {
> +        struct jsonrpc_monitor_node *jm, *next_jm;
> +
> +        /* Delete all front end monitors. Removing the last front
> +         * end monitor will also destroy the corresponding 'ovsdb_monitor'.
> +         * ovsdb monitor will also be destroied.  */

I found the last sentence confusing based on the previous sentence.  Also, "destroyed" is misspelled.

Acked-by: Justin Pettit <jpettit@ovn.org>

--Justin
Ben Pfaff Jan. 22, 2018, 7:21 p.m. | #2
On Fri, Jan 12, 2018 at 06:24:28PM -0800, Justin Pettit wrote:
> 
> 
> > On Dec 31, 2017, at 9:16 PM, Ben Pfaff <blp@ovn.org> wrote:
> > 
> > diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
> > index b2ecd109ed60..3e58c3fbd274 100644
> > --- a/ovsdb/monitor.c
> > +++ b/ovsdb/monitor.c
> > 
> > +void
> > +ovsdb_monitors_remove(struct ovsdb *db)
> > +{
> > +    struct ovsdb_monitor *m, *next_m;
> > +
> > +    LIST_FOR_EACH_SAFE (m, next_m, list_node, &db->monitors) {
> > +        struct jsonrpc_monitor_node *jm, *next_jm;
> > +
> > +        /* Delete all front end monitors. Removing the last front
> > +         * end monitor will also destroy the corresponding 'ovsdb_monitor'.
> > +         * ovsdb monitor will also be destroied.  */
> 
> I found the last sentence confusing based on the previous sentence.  Also, "destroyed" is misspelled.

Thanks, I fixed the comment.

> Acked-by: Justin Pettit <jpettit@ovn.org>

Thanks!

Patch

diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
index b2ecd109ed60..3e58c3fbd274 100644
--- a/ovsdb/monitor.c
+++ b/ovsdb/monitor.c
@@ -41,7 +41,6 @@ 
 
 VLOG_DEFINE_THIS_MODULE(ovsdb_monitor);
 
-static const struct ovsdb_replica_class ovsdb_jsonrpc_replica_class;
 static struct hmap ovsdb_monitors = HMAP_INITIALIZER(&ovsdb_monitors);
 
 /* Keep state of session's conditions */
@@ -66,7 +65,7 @@  struct ovsdb_monitor_table_condition {
 
 /* A collection of tables being monitored. */
 struct ovsdb_monitor {
-    struct ovsdb_replica replica;
+    struct ovs_list list_node;  /* In struct ovsdb's "monitors" list. */
     struct shash tables;     /* Holds "struct ovsdb_monitor_table"s. */
     struct ovs_list jsonrpc_monitors;  /* Contains "jsonrpc_monitor_node"s. */
     struct ovsdb *db;
@@ -239,13 +238,6 @@  compare_ovsdb_monitor_column(const void *a_, const void *b_)
     return a->column < b->column ? -1 : a->column > b->column;
 }
 
-static struct ovsdb_monitor *
-ovsdb_monitor_cast(struct ovsdb_replica *replica)
-{
-    ovs_assert(replica->class == &ovsdb_jsonrpc_replica_class);
-    return CONTAINER_OF(replica, struct ovsdb_monitor, replica);
-}
-
 /* Finds and returns the ovsdb_monitor_row in 'mt->changes->rows' for the
  * given 'uuid', or NULL if there is no such row. */
 static struct ovsdb_monitor_row *
@@ -380,8 +372,7 @@  ovsdb_monitor_create(struct ovsdb *db,
 
     dbmon = xzalloc(sizeof *dbmon);
 
-    ovsdb_replica_init(&dbmon->replica, &ovsdb_jsonrpc_replica_class);
-    ovsdb_add_replica(db, &dbmon->replica);
+    ovs_list_push_back(&db->monitors, &dbmon->list_node);
     ovs_list_init(&dbmon->jsonrpc_monitors);
     dbmon->db = db;
     dbmon->n_transactions = 0;
@@ -1547,7 +1538,7 @@  ovsdb_monitor_destroy(struct ovsdb_monitor *dbmon)
 {
     struct shash_node *node;
 
-    ovs_list_remove(&dbmon->replica.node);
+    ovs_list_remove(&dbmon->list_node);
 
     if (!hmap_node_is_null(&dbmon->hmap_node)) {
         hmap_remove(&ovsdb_monitors, &dbmon->hmap_node);
@@ -1574,11 +1565,8 @@  ovsdb_monitor_destroy(struct ovsdb_monitor *dbmon)
 }
 
 static void
-ovsdb_monitor_commit(struct ovsdb_replica *replica,
-                     const struct ovsdb_txn *txn,
-                     bool durable OVS_UNUSED)
+ovsdb_monitor_commit(struct ovsdb_monitor *m, const struct ovsdb_txn *txn)
 {
-    struct ovsdb_monitor *m = ovsdb_monitor_cast(replica);
     struct ovsdb_monitor_aux aux;
 
     ovsdb_monitor_init_aux(&aux, m);
@@ -1603,17 +1591,30 @@  ovsdb_monitor_commit(struct ovsdb_replica *replica,
     }
 }
 
-static void
-ovsdb_monitor_destroy_callback(struct ovsdb_replica *replica)
+void
+ovsdb_monitors_commit(struct ovsdb *db, const struct ovsdb_txn *txn)
 {
-    struct ovsdb_monitor *dbmon = ovsdb_monitor_cast(replica);
-    struct jsonrpc_monitor_node *jm, *next;
+    struct ovsdb_monitor *m;
 
-    /* Delete all front end monitors. Removing the last front
-     * end monitor will also destroy the corresponding 'ovsdb_monitor'.
-     * ovsdb monitor will also be destroied.  */
-    LIST_FOR_EACH_SAFE(jm, next, node, &dbmon->jsonrpc_monitors) {
-        ovsdb_jsonrpc_monitor_destroy(jm->jsonrpc_monitor);
+    LIST_FOR_EACH (m, list_node, &db->monitors) {
+        ovsdb_monitor_commit(m, txn);
+    }
+}
+
+void
+ovsdb_monitors_remove(struct ovsdb *db)
+{
+    struct ovsdb_monitor *m, *next_m;
+
+    LIST_FOR_EACH_SAFE (m, next_m, list_node, &db->monitors) {
+        struct jsonrpc_monitor_node *jm, *next_jm;
+
+        /* Delete all front end monitors. Removing the last front
+         * end monitor will also destroy the corresponding 'ovsdb_monitor'.
+         * ovsdb monitor will also be destroied.  */
+        LIST_FOR_EACH_SAFE (jm, next_jm, node, &m->jsonrpc_monitors) {
+            ovsdb_jsonrpc_monitor_destroy(jm->jsonrpc_monitor);
+        }
     }
 }
 
@@ -1629,8 +1630,3 @@  ovsdb_monitor_get_memory_usage(struct simap *usage)
         simap_increase(usage, "json-caches", hmap_count(&dbmon->json_cache));
     }
 }
-
-static const struct ovsdb_replica_class ovsdb_jsonrpc_replica_class = {
-    ovsdb_monitor_commit,
-    ovsdb_monitor_destroy_callback,
-};
diff --git a/ovsdb/monitor.h b/ovsdb/monitor.h
index 21f27c62160d..99d43c45dff9 100644
--- a/ovsdb/monitor.h
+++ b/ovsdb/monitor.h
@@ -1,5 +1,5 @@ 
 /*
- * Copyright (c) 2015 Nicira, Inc.
+ * Copyright (c) 2015, 2017 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -17,10 +17,14 @@ 
 #ifndef OVSDB_MONITOR_H
 #define OVSDB_MONITOR_H
 
+struct ovsdb;
+struct ovsdb_column;
 struct ovsdb_monitor;
 struct ovsdb_jsonrpc_monitor;
 struct ovsdb_monitor_session_condition;
 struct ovsdb_condition;
+struct ovsdb_txn;
+struct simap;
 
 enum ovsdb_monitor_selection {
     OJMS_NONE = 0,              /* None for this iteration */
@@ -42,6 +46,8 @@  enum ovsdb_monitor_version {
 
 struct ovsdb_monitor *ovsdb_monitor_create(struct ovsdb *db,
                        struct ovsdb_jsonrpc_monitor *jsonrpc_monitor);
+void ovsdb_monitors_remove(struct ovsdb *);
+void ovsdb_monitors_commit(struct ovsdb *, const struct ovsdb_txn *);
 
 struct ovsdb_monitor *ovsdb_monitor_add(struct ovsdb_monitor *dbmon);
 
diff --git a/ovsdb/ovsdb.c b/ovsdb/ovsdb.c
index 213d0d4823e1..19755e673861 100644
--- a/ovsdb/ovsdb.c
+++ b/ovsdb/ovsdb.c
@@ -19,6 +19,7 @@ 
 
 #include "column.h"
 #include "file.h"
+#include "monitor.h"
 #include "openvswitch/json.h"
 #include "ovsdb-error.h"
 #include "ovsdb-parser.h"
@@ -330,7 +331,7 @@  ovsdb_create(struct ovsdb_schema *schema)
     db = xmalloc(sizeof *db);
     db->schema = schema;
     db->file = NULL;
-    ovs_list_init(&db->replicas);
+    ovs_list_init(&db->monitors);
     ovs_list_init(&db->triggers);
     db->run_triggers = false;
 
@@ -370,13 +371,8 @@  ovsdb_destroy(struct ovsdb *db)
             ovsdb_file_destroy(db->file);
         }
 
-        /* Remove all the replicas. */
-        while (!ovs_list_is_empty(&db->replicas)) {
-            struct ovsdb_replica *r
-                = CONTAINER_OF(ovs_list_pop_back(&db->replicas),
-                               struct ovsdb_replica, node);
-            ovsdb_remove_replica(db, r);
-        }
+        /* Remove all the monitors. */
+        ovsdb_monitors_remove(db);
 
         /* Delete all the tables.  This also deletes their schemas. */
         SHASH_FOR_EACH (node, &db->tables) {
@@ -419,23 +415,3 @@  ovsdb_get_table(const struct ovsdb *db, const char *name)
 {
     return shash_find_data(&db->tables, name);
 }
-
-void
-ovsdb_replica_init(struct ovsdb_replica *r,
-                   const struct ovsdb_replica_class *class)
-{
-    r->class = class;
-}
-
-void
-ovsdb_add_replica(struct ovsdb *db, struct ovsdb_replica *r)
-{
-    ovs_list_push_back(&db->replicas, &r->node);
-}
-
-void
-ovsdb_remove_replica(struct ovsdb *db OVS_UNUSED, struct ovsdb_replica *r)
-{
-    ovs_list_remove(&r->node);
-    (r->class->destroy)(r);
-}
diff --git a/ovsdb/ovsdb.h b/ovsdb/ovsdb.h
index 06cd3a72e49e..9d915f0f15ae 100644
--- a/ovsdb/ovsdb.h
+++ b/ovsdb/ovsdb.h
@@ -57,7 +57,7 @@  bool ovsdb_schema_equal(const struct ovsdb_schema *,
 struct ovsdb {
     struct ovsdb_schema *schema;
     struct ovsdb_file *file;    /* If nonnull, log for transactions. */
-    struct ovs_list replicas;   /* Contains "struct ovsdb_replica"s. */
+    struct ovs_list monitors;   /* Contains "struct ovsdb_monitor"s. */
     struct shash tables;        /* Contains "struct ovsdb_table *"s. */
 
     /* Triggers. */
@@ -79,24 +79,5 @@  struct json *ovsdb_execute(struct ovsdb *, const struct ovsdb_session *,
                            const char *role, const char *id,
                            long long int elapsed_msec,
                            long long int *timeout_msec);
-
-/* Database replication. */
-
-struct ovsdb_replica {
-    struct ovs_list node;       /* Element in "struct ovsdb" replicas list. */
-    const struct ovsdb_replica_class *class;
-};
-
-struct ovsdb_replica_class {
-    void (*commit)(struct ovsdb_replica *,
-                   const struct ovsdb_txn *, bool durable);
-    void (*destroy)(struct ovsdb_replica *);
-};
-
-void ovsdb_replica_init(struct ovsdb_replica *,
-                        const struct ovsdb_replica_class *);
-
-void ovsdb_add_replica(struct ovsdb *, struct ovsdb_replica *);
-void ovsdb_remove_replica(struct ovsdb *, struct ovsdb_replica *);
 
 #endif /* ovsdb/ovsdb.h */
diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c
index ba17834aa633..f1502ffb398c 100644
--- a/ovsdb/transaction.c
+++ b/ovsdb/transaction.c
@@ -21,6 +21,7 @@ 
 #include "openvswitch/dynamic-string.h"
 #include "file.h"
 #include "hash.h"
+#include "monitor.h"
 #include "openvswitch/hmap.h"
 #include "openvswitch/json.h"
 #include "openvswitch/list.h"
@@ -808,7 +809,6 @@  update_version(struct ovsdb_txn *txn OVS_UNUSED, struct ovsdb_txn_row *txn_row)
 static struct ovsdb_error *
 ovsdb_txn_commit_(struct ovsdb_txn *txn, bool durable)
 {
-    struct ovsdb_replica *replica;
     struct ovsdb_error *error;
 
     /* Figure out what actually changed, and abort early if the transaction
@@ -873,9 +873,7 @@  ovsdb_txn_commit_(struct ovsdb_txn *txn, bool durable)
             return error;
         }
     }
-    LIST_FOR_EACH (replica, node, &txn->db->replicas) {
-        replica->class->commit(replica, txn, durable);
-    }
+    ovsdb_monitors_commit(txn->db, txn);
 
     /* Finalize commit. */
     txn->db->run_triggers = true;