[ovs-dev] [PATCH 09/15] ovsdb-server: Add support for a built-in _Server database.

Ben Pfaff blp at ovn.org
Thu Feb 1 18:59:54 UTC 2018


On Tue, Jan 30, 2018 at 03:27:18PM -0800, Justin Pettit wrote:
> 
> > On Dec 31, 2017, at 9:16 PM, Ben Pfaff <blp at ovn.org> wrote:
> > 
> > @@ -5,6 +7,7 @@
> > /ovsdb-idlc
> > /ovsdb-server
> > /ovsdb-server.1
> > +/ovsdb-server.5
> 
> Do you need to add the new man page to the distributions?

Yes.  Thanks, fixed.

> 	debian/openvswitch-switch.manpages
> 	rhel/openvswitch-fedora.spec.in
> 	rhel/openvswitch.spec.in
> 	xenserver/openvswitch-xen.spec.in
> 
> Also, I think you may want to add a reference to "Documentation/ref/index.rst".

Yes.  Thanks, fixed.

> > +# _Server schema documentation
> > +EXTRA_DIST += ovsdb/_server.xml
> > +CLEANFILES += ovsdb/ovsdb-server.5
> > +man_MANS += ovsdb/ovsdb-server.5
> > +ovsdb/ovsdb-server.5: \
> > +	ovsdb/ovsdb-doc ovsdb/_server.xml ovsdb/_server.ovsschema \
> > +	$(VSWITCH_PIC)
> 
> Did you intend to include VSWITCH_PIC?

No.  Removed.

> > @@ -328,6 +333,7 @@ main(int argc, char *argv[])
> >             ovs_fatal(0, "%s", error);
> >         }
> >     }
> > +    add_server_db(&server_config);
> 
> I don't think there's anything that frees 'server_config'.  Not a huge
> deal since it would be on shutdown, but it's always nice to have a
> clean valgrind run.

server_config is a local variable, and not a pointer.  What kind of
freeing would you like to see?

> > +/* Add the internal _Server database to the server configuration. */
> > +static void
> > +add_server_db(struct server_config *config)
> > +{
> > +    struct json *schema_json = json_from_string(
> > +#include "ovsdb/_server.ovsschema.inc"
> > +        );
> > +    ovs_assert(schema_json->type == JSON_OBJECT);
> > +
> > +    struct ovsdb_schema *schema;
> > +    struct ovsdb_error *error OVS_UNUSED = ovsdb_schema_from_json(schema_json,
> > +                                                                  &schema);
> > +    ovs_assert(!error);
> > +    json_destroy(schema_json);
> > +
> > +    struct db *db = xzalloc(sizeof *db);
> > +    db->filename = xstrdup("<internal>");
> > +    db->db = ovsdb_create(schema);
> > +    add_db(config, db->db->schema->name, db);
> > +}
> 
> Probably not a huge deal, since there's a single instance that runs as
> long as the process, but I don't think there's anything that free the
> memory allocated from this internal database.

It's not really different from other databases, other than it being
updated from the ovsdb-server main loop rather than from JSON-RPC
clients, so I think that any freeing (or not freeing) applied to other
databases should work equally well (or badly) for this one.

> > +/* Updates the Database table in the _Server database. */
> > +static void
> > +update_server_status(struct shash *all_dbs)
> > +{
> > +    struct db *server_db = shash_find_data(all_dbs, "_Server");
> > +    struct ovsdb_table *database_table = shash_find_data(
> > +        &server_db->db->tables, "Database");
> > +    struct ovsdb_txn *txn = ovsdb_txn_create(server_db->db);
> > +
> > +    /* Update rows for databases that still exist.
> > +     * Delete rows for databases that no longer exist. */
> > +    const struct ovsdb_row *row, *next_row;
> > +    HMAP_FOR_EACH_SAFE (row, next_row, hmap_node, &database_table->rows) {
> > +        const char *name;
> > +        ovsdb_util_read_string_column(row, "name", &name);
> > +        struct db *db = shash_find_data(all_dbs, name);
> > +        if (!db || !db->db) {
> > +            ovsdb_txn_row_delete(txn, row);
> > +        } else {
> > +            update_database_status(ovsdb_txn_row_modify(txn, row), db);
> >         }
> >     }
> > +
> > +    /* Add rows for new databases.
> > +     *
> > +     * This is O(n**2) but usually there are only 2 or 3 databases. */
> > +    struct shash_node *node;
> > +    SHASH_FOR_EACH (node, all_dbs) {
> > +        struct db *db = node->data;
> > +
> > +        if (!db->db) {
> > +            continue;
> > +        }
> > +
> > +        HMAP_FOR_EACH (row, hmap_node, &database_table->rows) {
> > +            const char *name;
> > +            ovsdb_util_read_string_column(row, "name", &name);
> > +            if (!strcmp(name, node->name)) {
> > +                goto next;
> > +            }
> > +        }
> > +
> > +        /* Add row. */
> > +        struct ovsdb_row *row = ovsdb_row_create(database_table);
> > +        uuid_generate(ovsdb_row_get_uuid_rw(row));
> > +        update_database_status(row, db);
> > +        ovsdb_txn_row_insert(txn, row);
> > +
> > +    next:;
> > +    }
> > +
> > +    commit_txn(txn, "_Server");
> > }
> 
> I see a memory leak when I run unit tests (e.g., "testing database multiplexing implementation") under valgrind:
> 
> ==123484== 6 bytes in 6 blocks are definitely lost in loss record 4 of 115
> ==123484==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd6
> 4-linux.so)
> ==123484==    by 0x437084: xmalloc (util.c:120)
> ==123484==    by 0x4370B3: xmemdup (util.c:142)
> ==123484==    by 0x4257BE: ovsdb_atom_init_default (ovsdb-data.c:77)
> ==123484==    by 0x42580D: alloc_default_atoms (ovsdb-data.c:317)
> ==123484==    by 0x425F48: ovsdb_datum_init_default (ovsdb-data.c:913)
> ==123484==    by 0x412C12: ovsdb_row_create (row.c:56)
> ==123484==    by 0x4051F7: update_server_status (ovsdb-server.c:1030)
> ==123484==    by 0x4051F7: main_loop (ovsdb-server.c:226)
> ==123484==    by 0x4051F7: main (ovsdb-server.c:438)

Oops.  Fixed.

> > diff --git a/ovsdb/ovsdb-util.c b/ovsdb/ovsdb-util.c
> > index 5ee5e4ddaf8d..06d25af49a18 100644
> > --- a/ovsdb/ovsdb-util.c
> > +++ b/ovsdb/ovsdb-util.c
> > @@ -22,6 +22,38 @@
> > 
> > VLOG_DEFINE_THIS_MODULE(ovsdb_util);
> > 
> > +static void
> > +ovsdb_util_clear_column(struct ovsdb_row *row, const char *column_name)
> > +{
> > ...
> > +    if (column->type.n_min) {
> > +        if (!VLOG_DROP_DBG(&rl)) {
> > +            char *type_name = ovsdb_type_to_english(&column->type);
> > +            VLOG_DBG("Table `%s' column `%s' has type %s, which requires "
> > +                     "a value, when it was expected to be optional",
> > +                     schema->name, column_name, type_name);
> 
> This error message seemed a little unclear to me.  What about
> something along the lines of "Table x column y has type z, which
> requires a value, but an attempt was made to clear it"?

That is better.  Thank you.  Changed.

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

Thanks.

I'm appending the incremental that I folded in.

--8<--------------------------cut here-------------------------->8--

diff --git a/Documentation/ref/index.rst b/Documentation/ref/index.rst
index d83b809f5e5b..5c851323009c 100644
--- a/Documentation/ref/index.rst
+++ b/Documentation/ref/index.rst
@@ -109,6 +109,10 @@ The remainder are still in roff format can be found below:
      - `(pdf) <http://openvswitch.org/support/dist-docs/ovsdb-server.1.pdf>`__
      - `(html) <http://openvswitch.org/support/dist-docs/ovsdb-server.1.html>`__
      - `(plain text) <http://openvswitch.org/support/dist-docs/ovsdb-server.1.txt>`__
+   * - ovsdb-server(5)
+     - `(pdf) <http://openvswitch.org/support/dist-docs/ovsdb-server.5.pdf>`__
+     - `(html) <http://openvswitch.org/support/dist-docs/ovsdb-server.5.html>`__
+     - `(plain text) <http://openvswitch.org/support/dist-docs/ovsdb-server.5.txt>`__
    * - ovsdb-tool(1)
      - `(pdf) <http://openvswitch.org/support/dist-docs/ovsdb-tool.1.pdf>`__
      - `(html) <http://openvswitch.org/support/dist-docs/ovsdb-tool.1.html>`__
diff --git a/debian/openvswitch-switch.manpages b/debian/openvswitch-switch.manpages
index a2f661a3e58c..c85cbfd30b8f 100644
--- a/debian/openvswitch-switch.manpages
+++ b/debian/openvswitch-switch.manpages
@@ -1,4 +1,5 @@
 ovsdb/ovsdb-server.1
+ovsdb/ovsdb-server.5
 utilities/ovs-ctl.8
 utilities/ovs-dpctl-top.8
 utilities/ovs-dpctl.8
diff --git a/ovsdb/automake.mk b/ovsdb/automake.mk
index c90e2e5b77f9..5760cba44681 100644
--- a/ovsdb/automake.mk
+++ b/ovsdb/automake.mk
@@ -128,8 +128,7 @@ EXTRA_DIST += ovsdb/_server.xml
 CLEANFILES += ovsdb/ovsdb-server.5
 man_MANS += ovsdb/ovsdb-server.5
 ovsdb/ovsdb-server.5: \
-	ovsdb/ovsdb-doc ovsdb/_server.xml ovsdb/_server.ovsschema \
-	$(VSWITCH_PIC)
+	ovsdb/ovsdb-doc ovsdb/_server.xml ovsdb/_server.ovsschema
 	$(AM_V_GEN)$(OVSDB_DOC) \
 		--version=$(VERSION) \
 		$(srcdir)/ovsdb/_server.ovsschema \
diff --git a/ovsdb/ovsdb-util.c b/ovsdb/ovsdb-util.c
index 06d25af49a18..4bda9782bc06 100644
--- a/ovsdb/ovsdb-util.c
+++ b/ovsdb/ovsdb-util.c
@@ -40,7 +40,7 @@ ovsdb_util_clear_column(struct ovsdb_row *row, const char *column_name)
         if (!VLOG_DROP_DBG(&rl)) {
             char *type_name = ovsdb_type_to_english(&column->type);
             VLOG_DBG("Table `%s' column `%s' has type %s, which requires "
-                     "a value, when it was expected to be optional",
+                     "a value, but an attempt was made to clear it",
                      schema->name, column_name, type_name);
             free(type_name);
         }
@@ -225,6 +225,7 @@ ovsdb_util_write_singleton(struct ovsdb_row *row, const char *column_name,
         if (ovsdb_atom_equals(&datum->keys[0], atom, type)) {
             return;
         }
+        ovsdb_atom_destroy(&datum->keys[0], type);
     } else {
         ovsdb_datum_destroy(datum, &column->type);
         datum->n = 1;
diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
index ede62c8442c3..1468bb5c4e3b 100644
--- a/rhel/openvswitch-fedora.spec.in
+++ b/rhel/openvswitch-fedora.spec.in
@@ -564,6 +564,7 @@ fi
 %{_mandir}/man1/ovsdb-client.1*
 %{_mandir}/man1/ovsdb-server.1*
 %{_mandir}/man1/ovsdb-tool.1*
+%{_mandir}/man5/ovsdb-server.5*
 %{_mandir}/man5/ovs-vswitchd.conf.db.5*
 %{_mandir}/man5/ovsdb.5*
 %{_mandir}/man5/vtep.5*
diff --git a/rhel/openvswitch.spec.in b/rhel/openvswitch.spec.in
index 104b2732956d..26eb73f3b44d 100644
--- a/rhel/openvswitch.spec.in
+++ b/rhel/openvswitch.spec.in
@@ -227,6 +227,7 @@ exit 0
 /usr/share/man/man1/ovsdb-client.1.gz
 /usr/share/man/man1/ovsdb-server.1.gz
 /usr/share/man/man1/ovsdb-tool.1.gz
+/usr/share/man/man5/ovsdb-server.5.gz
 /usr/share/man/man5/ovs-vswitchd.conf.db.5.gz
 %{_mandir}/man5/ovsdb.5*
 /usr/share/man/man5/vtep.5.gz
diff --git a/xenserver/openvswitch-xen.spec.in b/xenserver/openvswitch-xen.spec.in
index 564fad68488e..c1fbcc75ebea 100644
--- a/xenserver/openvswitch-xen.spec.in
+++ b/xenserver/openvswitch-xen.spec.in
@@ -481,6 +481,7 @@ exit 0
 /usr/share/man/man1/ovsdb-client.1.gz
 /usr/share/man/man1/ovsdb-server.1.gz
 /usr/share/man/man1/ovsdb-tool.1.gz
+/usr/share/man/man5/ovsdb-server.5.gz
 /usr/share/man/man5/ovs-vswitchd.conf.db.5.gz
 /usr/share/man/man5/vtep.5.gz
 /usr/share/man/man7/ovs-fields.7.gz


More information about the dev mailing list