[ovs-dev] [PATCH] ovsdb-server: Drop 'txn' member from struct db.

[Yifeng Sun]

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 <blp at 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 <blp at 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 <blp at 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
> > > dev at openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
>