> On Dec 31, 2017, at 9:16 PM, Ben Pfaff <b...@ovn.org> wrote: > > +struct ovsdb_idl { > + struct ovsdb_idl_db server; > + struct ovsdb_idl_db db; /* XXX rename 'data'? */
Did you want to change this? > @@ -353,51 +379,54 @@ ovsdb_idl_set_remote(struct ovsdb_idl *idl, const char > *remote, > bool retry) > { > if (idl) { > - ovs_assert(!idl->txn); > - jsonrpc_session_close(idl->session); > idl->session = jsonrpc_session_open(remote, retry); > + /* XXX update condition */ > idl->state_seqno = UINT_MAX; Does this "XXX" need to be addressed. > +static void > +ovsdb_idl_db_destroy(struct ovsdb_idl_db *db) > +{ > + ovs_assert(!db->txn); > + for (size_t i = 0; i < db->class_->n_tables; i++) { > + struct ovsdb_idl_table *table = &db->tables[i]; > + ovsdb_idl_condition_destroy(&table->condition); > + ovsdb_idl_destroy_indexes(table); > + shash_destroy(&table->columns); > + hmap_destroy(&table->rows); > + free(table->modes); > + } > + shash_destroy(&db->table_by_name); > + free(db->tables); > + json_destroy(db->schema); > + hmap_destroy(&db->outstanding_txns); > + free(db->lock_name); > + json_destroy(db->lock_request_id); > +} Do you need call json_destroy() on "db->monitor_id"? There's an assert on "db->txn", but not one checking that 'outstanding_txns' empty. > +static void > +ovsdb_idl_send_cond_change(struct ovsdb_idl *idl) > +{ > + /* When 'idl->request_id' is not NULL, there is an outstanding > + * conditional monitoring update request that we have not heard > + * from the server yet. Don't generate another request in this case. > + * > + * XXX per-db request_id */ > + if (!jsonrpc_session_is_connected(idl->session) > + || idl->state != IDL_S_MONITORING_COND > + || idl->request_id) { > + return; > + } Did you need to address this "XXX" comment? > +/* Turns off OVSDB_IDL_ALERT for 'column' in 'idl'. > + * > + * This function should be called between ovsdb_idl_create() and the first > call > + * to ovsdb_idl_run(). > + */ > +static void > +ovsdb_idl_db_omit_alert(struct ovsdb_idl_db *db, > + const struct ovsdb_idl_column *column) I think it should be 'db' instead of 'idl'. > @@ -2972,10 +3072,10 @@ ovsdb_idl_txn_create(struct ovsdb_idl *idl) > { > struct ovsdb_idl_txn *txn; > > - ovs_assert(!idl->txn); > - idl->txn = txn = xmalloc(sizeof *txn); > + ovs_assert(!idl->db.txn); > + idl->db.txn = txn = xmalloc(sizeof *txn); > txn->request_id = NULL; > - txn->idl = idl; > + txn->db = &idl->db; > hmap_init(&txn->txn_rows); Should there be an assert in this function that 'outstanding_txns' is empty? I didn't look super closely at the patch, since it seemed to mostly be refactoring. Let me know if you'd like me to look at it more closely. Acked-by: Justin Pettit <jpet...@ovn.org> --Justin _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev