On Tue, Jan 30, 2018 at 03:27:18PM -0800, Justin Pettit wrote: > > > On Dec 31, 2017, at 9:16 PM, Ben Pfaff <b...@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 <jpet...@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 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev