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

[Justin Pettit]

> On Feb 1, 2018, at 11:10 AM, Ben Pfaff  wrote:
> 
> I think that these are the same things that are getting allocated in
> open_db() for "real" databases, so they should be getting freed in the
> same way.  (Maybe there is a memory leak in that case too, but I do not
> see it yet.)

That makes sense.  Thank you.

--Justin


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Ben Pfaff]

On Thu, Feb 01, 2018 at 11:05:35AM -0800, Justin Pettit wrote:
> 
> > On Feb 1, 2018, at 10:59 AM, Ben Pfaff  wrote:
> > 
> > On Tue, Jan 30, 2018 at 03:27:18PM -0800, Justin Pettit wrote:
> >> 
> >>> On Dec 31, 2017, at 9:16 PM, Ben Pfaff  wrote:
> >>> 
> >>> @@ -328,6 +333,7 @@ main(int argc, char *argv[])
> >>>ovs_fatal(0, "%s", error);
> >>>}
> >>>}
> >>> +add_server_db(_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?
> 
> I meant the things that were allocated in add_server_db().

OK.

I think that these are the same things that are getting allocated in
open_db() for "real" databases, so they should be getting freed in the
same way.  (Maybe there is a memory leak in that case too, but I do not
see it yet.)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Justin Pettit]

> On Feb 1, 2018, at 10:59 AM, Ben Pfaff  wrote:
> 
> On Tue, Jan 30, 2018 at 03:27:18PM -0800, Justin Pettit wrote:
>> 
>>> On Dec 31, 2017, at 9:16 PM, Ben Pfaff  wrote:
>>> 
>>> @@ -328,6 +333,7 @@ main(int argc, char *argv[])
>>>ovs_fatal(0, "%s", error);
>>>}
>>>}
>>> +add_server_db(_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?

I meant the things that were allocated in add_server_db().

--Justin


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Ben Pfaff]

On Tue, Jan 30, 2018 at 03:27:18PM -0800, Justin Pettit wrote:
> 
> > On Dec 31, 2017, at 9:16 PM, Ben Pfaff  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(_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,
> > +  );
> > +ovs_assert(!error);
> > +json_destroy(schema_json);
> > +
> > +struct db *db = xzalloc(sizeof *db);
> > +db->filename = xstrdup("");
> > +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(
> > +_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, _table->rows) {
> > +const char *name;
> > +ovsdb_util_read_string_column(row, "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, _table->rows) {
> > +const char *name;
> > +ovsdb_util_read_string_column(row, "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: 

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

[Justin Pettit]

> On Dec 31, 2017, at 9:16 PM, Ben Pfaff  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(_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,
> +  );
> +ovs_assert(!error);
> +json_destroy(schema_json);
> +
> +struct db *db = xzalloc(sizeof *db);
> +db->filename = xstrdup("");
> +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(
> +_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, _table->rows) {
> +const char *name;
> +ovsdb_util_read_string_column(row, "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, _table->rows) {
> +const char *name;
> +ovsdb_util_read_string_column(row, "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