> On Dec 31, 2017, at 9:16 PM, Ben Pfaff <[email protected]> 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 <[email protected]>
--Justin
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
