[ovs-dev] [PATCH 09/15] ovsdb-server: Add support for a built-in _Server database.
Justin Pettit
jpettit at ovn.org
Tue Jan 30 23:27:18 UTC 2018
> 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?
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".
> +# _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?
> @@ -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.
> +/* 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.
> +/* 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)
> 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"?
Acked-by: Justin Pettit <jpettit at ovn.org>
--Justin
More information about the dev
mailing list