Thanks for pointing that out. I sent out a fix:
https://patchwork.ozlabs.org/patch/851073/
On Tue, Dec 19, 2017 at 02:13:38PM -0800, Yifeng Sun wrote:
> At the start of ovsdb_txn_commit_, if there is an error, it looks like that
> txn doesn't get freed.
> After that, it surely is freed, as you said.
>
> error = for_each_txn_row(txn, determine_changes);
>
>
> if (error) {
>
>
> return OVSDB_WRAP_BUG("can't happen", error);
>
>
> }
>
>
>
> On Tue, Dec 19, 2017 at 2:03 PM, Ben Pfaff <b...@ovn.org> wrote:
>
> > Thank you for the review.
> >
> > ovsdb_txn_commit() is at least meant to free its argument whether it
> > succeeds or not. Do you see a bug there?
> >
> > I applied this to master.
> >
> > On Tue, Dec 19, 2017 at 09:50:45AM -0800, Yifeng Sun wrote:
> > > It seems that txn is not freed when ovsdb_txn_commit returns an error.
> > > Other than that, this patch looks good to me.
> > >
> > > Thanks,
> > > Yifeng
> > >
> > > On Fri, Dec 15, 2017 at 11:01 AM, Ben Pfaff <b...@ovn.org> wrote:
> > >
> > > > This member was only used in one particular code path, so this commit
> > > > adds code to pass it around as a function parameter instead.
> > > >
> > > > Signed-off-by: Ben Pfaff <b...@ovn.org>
> > > > ---
> > > > ovsdb/ovsdb-server.c | 40 ++++++++++++++++------------------------
> > > > 1 file changed, 16 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
> > > > index f9e4e529e32e..ffdcf6810ddf 100644
> > > > --- a/ovsdb/ovsdb-server.c
> > > > +++ b/ovsdb/ovsdb-server.c
> > > > @@ -62,13 +62,9 @@
> > > > VLOG_DEFINE_THIS_MODULE(ovsdb_server);
> > > >
> > > > struct db {
> > > > - /* Initialized in main(). */
> > > > char *filename;
> > > > struct ovsdb_file *file;
> > > > struct ovsdb *db;
> > > > -
> > > > - /* Only used by update_remote_status(). */
> > > > - struct ovsdb_txn *txn;
> > > > };
> > > >
> > > > /* SSL configuration. */
> > > > @@ -846,9 +842,10 @@ update_remote_row(const struct ovsdb_row *row,
> > struct
> > > > ovsdb_txn *txn,
> > > > }
> > > >
> > > > static void
> > > > -update_remote_rows(const struct shash *all_dbs,
> > > > +update_remote_rows(const struct shash *all_dbs, const struct db *db_,
> > > > const char *remote_name,
> > > > - const struct ovsdb_jsonrpc_server *jsonrpc)
> > > > + const struct ovsdb_jsonrpc_server *jsonrpc,
> > > > + struct ovsdb_txn *txn)
> > > > {
> > > > const struct ovsdb_table *table, *ref_table;
> > > > const struct ovsdb_column *column;
> > > > @@ -866,7 +863,8 @@ update_remote_rows(const struct shash *all_dbs,
> > > > return;
> > > > }
> > > >
> > > > - if (column->type.key.type != OVSDB_TYPE_UUID
> > > > + if (db != db_
> > > > + || column->type.key.type != OVSDB_TYPE_UUID
> > > > || !column->type.key.u.uuid.refTable
> > > > || column->type.value.type != OVSDB_TYPE_VOID) {
> > > > return;
> > > > @@ -884,7 +882,7 @@ update_remote_rows(const struct shash *all_dbs,
> > > >
> > > > ref_row = ovsdb_table_get_row(ref_table,
> > > > &datum->keys[i].uuid);
> > > > if (ref_row) {
> > > > - update_remote_row(ref_row, db->txn, jsonrpc);
> > > > + update_remote_row(ref_row, txn, jsonrpc);
> > > > }
> > > > }
> > > > }
> > > > @@ -895,26 +893,20 @@ update_remote_status(const struct
> > > > ovsdb_jsonrpc_server *jsonrpc,
> > > > const struct sset *remotes,
> > > > struct shash *all_dbs)
> > > > {
> > > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> > > > - const char *remote;
> > > > - struct db *db;
> > > > struct shash_node *node;
> > > > + SHASH_FOR_EACH (node, all_dbs) {
> > > > + struct db *db = node->data;
> > > > + struct ovsdb_txn *txn = ovsdb_txn_create(db->db);
> > > >
> > > > - SHASH_FOR_EACH(node, all_dbs) {
> > > > - db = node->data;
> > > > - db->txn = ovsdb_txn_create(db->db);
> > > > - }
> > > > -
> > > > - /* Iterate over --remote arguments given on command line. */
> > > > - SSET_FOR_EACH (remote, remotes) {
> > > > - update_remote_rows(all_dbs, remote, jsonrpc);
> > > > - }
> > > > + /* Iterate over --remote arguments given on command line. */
> > > > + const char *remote;
> > > > + SSET_FOR_EACH (remote, remotes) {
> > > > + update_remote_rows(all_dbs, db, remote, jsonrpc, txn);
> > > > + }
> > > >
> > > > - SHASH_FOR_EACH(node, all_dbs) {
> > > > - struct ovsdb_error *error;
> > > > - db = node->data;
> > > > - error = ovsdb_txn_commit(db->txn, false);
> > > > + struct ovsdb_error *error = ovsdb_txn_commit(txn, false);
> > > > if (error) {
> > > > + static struct vlog_rate_limit rl =
> > VLOG_RATE_LIMIT_INIT(1, 1);
> > > > char *msg = ovsdb_error_to_string_free(error);
> > > > VLOG_ERR_RL(&rl, "Failed to update remote status: %s",
> > msg);
> > > > free(msg);
> > > > --
> > > > 2.10.2
> > > >
> > > > _______________________________________________
> > > > dev mailing list
> > > > d...@openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > >
> >
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
