On Wed, Jan 24, 2018 at 05:44:05PM -0800, Justin Pettit wrote:
>
>
> > 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?
I did intend to change it. Done.
> > @@ -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.
It doesn't look like it. I removed the comment.
> > +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"?
Yes, thanks, fixed.
> There's an assert on "db->txn", but not one checking that 'outstanding_txns'
> empty.
Hmm. I added a function to abort transactions in an ovsdb_idl_db (based
on ovsdb_idl_txn_abort_all()) and added a call to it here.
> > +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?
I don't think anything is needed. Comment removed.
> > +/* 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'.
In the comment? Thanks, fixed.
> > @@ -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?
No, it's OK to have outstanding transactions committing in the
background, you just can't be constructing two transactions at once
since they would stomp on each other.
> 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.
Thanks for the review. I don't think anything further is needed for
this one. As you said, it's mostly moving code around.
> Acked-by: Justin Pettit <[email protected]>
I'm appending the incremental, although it's kind of hard to read due to
the renaming, sorry.
--8<--------------------------cut here-------------------------->8--
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 24ba5b50fddc..285c453e1985 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -165,7 +165,7 @@ static void ovsdb_idl_send_monitor_request(struct ovsdb_idl
*,
struct ovsdb_idl {
struct ovsdb_idl_db server;
- struct ovsdb_idl_db db; /* XXX rename 'data'? */
+ struct ovsdb_idl_db data;
/* Session state.
*
@@ -253,6 +253,7 @@ static void ovsdb_idl_row_clear_old(struct ovsdb_idl_row *);
static void ovsdb_idl_row_clear_new(struct ovsdb_idl_row *);
static void ovsdb_idl_row_clear_arcs(struct ovsdb_idl_row *, bool
destroy_dsts);
+static void ovsdb_idl_db_txn_abort_all(struct ovsdb_idl_db *);
static void ovsdb_idl_txn_abort_all(struct ovsdb_idl *);
static bool ovsdb_idl_db_txn_process_reply(struct ovsdb_idl_db *,
const struct jsonrpc_msg *msg);
@@ -365,7 +366,7 @@ ovsdb_idl_create(const char *remote, const struct
ovsdb_idl_class *class,
struct ovsdb_idl *idl;
idl = xzalloc(sizeof *idl);
- ovsdb_idl_db_init(&idl->db, class, idl, monitor_everything_by_default);
+ ovsdb_idl_db_init(&idl->data, class, idl, monitor_everything_by_default);
idl->session = jsonrpc_session_open(remote, retry);
idl->state_seqno = UINT_MAX;
idl->request_id = NULL;
@@ -380,7 +381,6 @@ ovsdb_idl_set_remote(struct ovsdb_idl *idl, const char
*remote,
{
if (idl) {
idl->session = jsonrpc_session_open(remote, retry);
- /* XXX update condition */
idl->state_seqno = UINT_MAX;
}
}
@@ -389,6 +389,7 @@ static void
ovsdb_idl_db_destroy(struct ovsdb_idl_db *db)
{
ovs_assert(!db->txn);
+ ovsdb_idl_db_txn_abort_all(db);
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);
@@ -403,6 +404,7 @@ ovsdb_idl_db_destroy(struct ovsdb_idl_db *db)
hmap_destroy(&db->outstanding_txns);
free(db->lock_name);
json_destroy(db->lock_request_id);
+ json_destroy(db->monitor_id);
}
/* Destroys 'idl' and all of the data structures that it manages. */
@@ -413,7 +415,7 @@ ovsdb_idl_destroy(struct ovsdb_idl *idl)
ovsdb_idl_clear(idl);
jsonrpc_session_close(idl->session);
- ovsdb_idl_db_destroy(&idl->db);
+ ovsdb_idl_db_destroy(&idl->data);
json_destroy(idl->request_id);
free(idl);
}
@@ -468,7 +470,7 @@ ovsdb_idl_db_clear(struct ovsdb_idl_db *db)
static void
ovsdb_idl_clear(struct ovsdb_idl *idl)
{
- ovsdb_idl_db_clear(&idl->db);
+ ovsdb_idl_db_clear(&idl->data);
}
static void
@@ -487,7 +489,7 @@ ovsdb_idl_run(struct ovsdb_idl *idl)
{
int i;
- ovs_assert(!idl->db.txn);
+ ovs_assert(!idl->data.txn);
ovsdb_idl_send_cond_change(idl);
@@ -503,11 +505,12 @@ ovsdb_idl_run(struct ovsdb_idl *idl)
idl->request_id = NULL;
ovsdb_idl_txn_abort_all(idl);
- ovsdb_idl_send_schema_request(idl, &idl->db);
+ ovsdb_idl_send_schema_request(idl, &idl->data);
idl->state = IDL_S_SCHEMA_REQUESTED;
- if (idl->db.lock_name) {
+ if (idl->data.lock_name) {
jsonrpc_session_send(
- idl->session, ovsdb_idl_db_compose_lock_request(&idl->db));
+ idl->session,
+ ovsdb_idl_db_compose_lock_request(&idl->data));
}
}
@@ -516,7 +519,7 @@ ovsdb_idl_run(struct ovsdb_idl *idl)
break;
}
- if (ovsdb_idl_db_parse_update_rpc(&idl->db, msg)) {
+ if (ovsdb_idl_db_parse_update_rpc(&idl->data, msg)) {
/* ovsdb_idl_db_parse_update_rpc() did all the processing. */
} else if (msg->type == JSONRPC_REPLY
&& idl->request_id
@@ -527,8 +530,8 @@ ovsdb_idl_run(struct ovsdb_idl *idl)
switch (idl->state) {
case IDL_S_SCHEMA_REQUESTED:
/* Reply to our "get_schema" request. */
- idl->db.schema = json_clone(msg->result);
- ovsdb_idl_send_monitor_request(idl, &idl->db, true);
+ idl->data.schema = json_clone(msg->result);
+ ovsdb_idl_send_monitor_request(idl, &idl->data, true);
idl->state = IDL_S_MONITOR_COND_REQUESTED;
break;
@@ -537,25 +540,25 @@ ovsdb_idl_run(struct ovsdb_idl *idl)
/* Reply to our "monitor" or "monitor_cond" request. */
if (idl->state == IDL_S_MONITOR_REQUESTED) {
idl->state = IDL_S_MONITORING;
- ovsdb_idl_db_parse_monitor_reply(&idl->db, msg->result,
+ ovsdb_idl_db_parse_monitor_reply(&idl->data, msg->result,
false);
} else { /* IDL_S_MONITOR_COND_REQUESTED. */
idl->state = IDL_S_MONITORING_COND;
- ovsdb_idl_db_parse_monitor_reply(&idl->db, msg->result,
+ ovsdb_idl_db_parse_monitor_reply(&idl->data, msg->result,
true);
}
/* Schema is not useful after monitor request is accepted
* by the server. */
- json_destroy(idl->db.schema);
- idl->db.schema = NULL;
+ json_destroy(idl->data.schema);
+ idl->data.schema = NULL;
break;
case IDL_S_MONITORING_COND:
/* Conditional monitor clauses were updated. Send out
* the next condition changes, in any, immediately. */
ovsdb_idl_send_cond_change(idl);
- idl->db.cond_seqno++;
+ idl->data.cond_seqno++;
break;
case IDL_S_MONITORING:
@@ -563,7 +566,7 @@ ovsdb_idl_run(struct ovsdb_idl *idl)
default:
OVS_NOT_REACHED();
}
- } else if (ovsdb_idl_db_process_lock_replies(&idl->db, msg)) {
+ } else if (ovsdb_idl_db_process_lock_replies(&idl->data, msg)) {
/* ovsdb_idl_db_process_lock_replies() did all the processing. */
} else if (msg->type == JSONRPC_ERROR
&& idl->state == IDL_S_MONITOR_COND_REQUESTED
@@ -574,7 +577,7 @@ ovsdb_idl_run(struct ovsdb_idl *idl)
/* Fall back to using "monitor" method. */
json_destroy(idl->request_id);
idl->request_id = NULL;
- ovsdb_idl_send_monitor_request(idl, &idl->db, false);
+ ovsdb_idl_send_monitor_request(idl, &idl->data, false);
idl->state = IDL_S_MONITOR_REQUESTED;
}
} else if (msg->type == JSONRPC_ERROR
@@ -597,7 +600,7 @@ ovsdb_idl_run(struct ovsdb_idl *idl)
idl->state = IDL_S_NO_SCHEMA;
} else if ((msg->type == JSONRPC_ERROR
|| msg->type == JSONRPC_REPLY)
- && ovsdb_idl_db_txn_process_reply(&idl->db, msg)) {
+ && ovsdb_idl_db_txn_process_reply(&idl->data, msg)) {
/* ovsdb_idl_txn_process_reply() did everything needful. */
} else {
/* This can happen if ovsdb_idl_txn_destroy() is called to destroy
@@ -609,7 +612,7 @@ ovsdb_idl_run(struct ovsdb_idl *idl)
}
jsonrpc_msg_destroy(msg);
}
- ovsdb_idl_row_destroy_postprocess(&idl->db);
+ ovsdb_idl_row_destroy_postprocess(&idl->data);
}
/* Arranges for poll_block() to wake up when ovsdb_idl_run() has something to
@@ -641,7 +644,7 @@ ovsdb_idl_wait(struct ovsdb_idl *idl)
unsigned int
ovsdb_idl_get_seqno(const struct ovsdb_idl *idl)
{
- return idl->db.change_seqno;
+ return idl->data.change_seqno;
}
/* Returns a "sequence number" that represents the number of conditional
@@ -661,7 +664,7 @@ ovsdb_idl_get_seqno(const struct ovsdb_idl *idl)
unsigned int
ovsdb_idl_get_condition_seqno(const struct ovsdb_idl *idl)
{
- return idl->db.cond_seqno;
+ return idl->data.cond_seqno;
}
/* Returns true if 'idl' successfully connected to the remote database and
@@ -701,7 +704,7 @@ ovsdb_idl_force_reconnect(struct ovsdb_idl *idl)
void
ovsdb_idl_verify_write_only(struct ovsdb_idl *idl)
{
- idl->db.verify_write_only = true;
+ idl->data.verify_write_only = true;
}
/* Returns true if 'idl' is currently connected or trying to connect
@@ -811,7 +814,7 @@ void
ovsdb_idl_check_consistency(const struct ovsdb_idl *idl)
{
/* Consistency is broken while a transaction is in progress. */
- if (!idl->db.txn) {
+ if (!idl->data.txn) {
return;
}
@@ -820,8 +823,8 @@ ovsdb_idl_check_consistency(const struct ovsdb_idl *idl)
struct uuid *dsts = NULL;
size_t allocated_dsts = 0;
- for (size_t i = 0; i < idl->db.class_->n_tables; i++) {
- const struct ovsdb_idl_table *table = &idl->db.tables[i];
+ for (size_t i = 0; i < idl->data.class_->n_tables; i++) {
+ const struct ovsdb_idl_table *table = &idl->data.tables[i];
const struct ovsdb_idl_table_class *class = table->class_;
const struct ovsdb_idl_row *row;
@@ -868,7 +871,7 @@ ovsdb_idl_check_consistency(const struct ovsdb_idl *idl)
const struct ovsdb_idl_class *
ovsdb_idl_get_class(const struct ovsdb_idl *idl)
{
- return idl->db.class_;
+ return idl->data.class_;
}
/* Given 'column' in some table in 'class', returns the table's class. */
@@ -945,7 +948,7 @@ void
ovsdb_idl_add_column(struct ovsdb_idl *idl,
const struct ovsdb_idl_column *column)
{
- ovsdb_idl_db_add_column(&idl->db, column);
+ ovsdb_idl_db_add_column(&idl->data, column);
}
static void
@@ -983,7 +986,7 @@ void
ovsdb_idl_add_table(struct ovsdb_idl *idl,
const struct ovsdb_idl_table_class *tc)
{
- ovsdb_idl_db_add_table(&idl->db, tc);
+ ovsdb_idl_db_add_table(&idl->data, tc);
}
/* A single clause within an ovsdb_idl_condition. */
@@ -1208,7 +1211,7 @@ ovsdb_idl_set_condition(struct ovsdb_idl *idl,
const struct ovsdb_idl_table_class *tc,
const struct ovsdb_idl_condition *condition)
{
- return ovsdb_idl_db_set_condition(&idl->db, tc, condition);
+ return ovsdb_idl_db_set_condition(&idl->data, tc, condition);
}
static struct json *
@@ -1286,23 +1289,21 @@ 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 */
+ * from the server yet. Don't generate another request in this case. */
if (!jsonrpc_session_is_connected(idl->session)
|| idl->state != IDL_S_MONITORING_COND
|| idl->request_id) {
return;
}
- struct jsonrpc_msg *msg = ovsdb_idl_db_compose_cond_change(&idl->db);
+ struct jsonrpc_msg *msg = ovsdb_idl_db_compose_cond_change(&idl->data);
if (msg) {
idl->request_id = json_clone(msg->id);
jsonrpc_session_send(idl->session, msg);
}
}
-/* Turns off OVSDB_IDL_ALERT for 'column' in 'idl'.
+/* Turns off OVSDB_IDL_ALERT for 'column' in 'db'.
*
* This function should be called between ovsdb_idl_create() and the first call
* to ovsdb_idl_run().
@@ -1323,7 +1324,7 @@ void
ovsdb_idl_omit_alert(struct ovsdb_idl *idl,
const struct ovsdb_idl_column *column)
{
- ovsdb_idl_db_omit_alert(&idl->db, column);
+ ovsdb_idl_db_omit_alert(&idl->data, column);
}
static void
@@ -1342,7 +1343,7 @@ ovsdb_idl_db_omit(struct ovsdb_idl_db *db,
void
ovsdb_idl_omit(struct ovsdb_idl *idl, const struct ovsdb_idl_column *column)
{
- ovsdb_idl_db_omit(&idl->db, column);
+ ovsdb_idl_db_omit(&idl->data, column);
}
/* Returns the most recent IDL change sequence number that caused a
@@ -1353,7 +1354,7 @@ ovsdb_idl_table_get_seqno(const struct ovsdb_idl *idl,
const struct ovsdb_idl_table_class *table_class)
{
struct ovsdb_idl_table *table
- = ovsdb_idl_db_table_from_class(&idl->db, table_class);
+ = ovsdb_idl_db_table_from_class(&idl->data, table_class);
unsigned int max_seqno = table->change_seqno[OVSDB_IDL_CHANGE_INSERT];
if (max_seqno < table->change_seqno[OVSDB_IDL_CHANGE_MODIFY]) {
@@ -1390,10 +1391,10 @@ void
ovsdb_idl_track_add_column(struct ovsdb_idl *idl,
const struct ovsdb_idl_column *column)
{
- if (!(*ovsdb_idl_db_get_mode(&idl->db, column) & OVSDB_IDL_ALERT)) {
+ if (!(*ovsdb_idl_db_get_mode(&idl->data, column) & OVSDB_IDL_ALERT)) {
ovsdb_idl_add_column(idl, column);
}
- *ovsdb_idl_db_get_mode(&idl->db, column) |= OVSDB_IDL_TRACK;
+ *ovsdb_idl_db_get_mode(&idl->data, column) |= OVSDB_IDL_TRACK;
}
void
@@ -1401,8 +1402,8 @@ ovsdb_idl_track_add_all(struct ovsdb_idl *idl)
{
size_t i, j;
- for (i = 0; i < idl->db.class_->n_tables; i++) {
- const struct ovsdb_idl_table_class *tc = &idl->db.class_->tables[i];
+ for (i = 0; i < idl->data.class_->n_tables; i++) {
+ const struct ovsdb_idl_table_class *tc = &idl->data.class_->tables[i];
for (j = 0; j < tc->n_columns; j++) {
const struct ovsdb_idl_column *column = &tc->columns[j];
@@ -1432,7 +1433,7 @@ ovsdb_idl_track_get_first(const struct ovsdb_idl *idl,
const struct ovsdb_idl_table_class *table_class)
{
struct ovsdb_idl_table *table
- = ovsdb_idl_db_table_from_class(&idl->db, table_class);
+ = ovsdb_idl_db_table_from_class(&idl->data, table_class);
if (!ovs_list_is_empty(&table->track_list)) {
return CONTAINER_OF(ovs_list_front(&table->track_list), struct
ovsdb_idl_row, track_node);
@@ -1515,7 +1516,7 @@ ovsdb_idl_db_track_clear(struct ovsdb_idl_db *db)
void
ovsdb_idl_track_clear(struct ovsdb_idl *idl)
{
- ovsdb_idl_db_track_clear(&idl->db);
+ ovsdb_idl_db_track_clear(&idl->data);
}
static void
@@ -2177,7 +2178,7 @@ ovsdb_idl_create_index(struct ovsdb_idl *idl,
const struct ovsdb_idl_table_class *tc,
const char *index_name)
{
- return ovsdb_idl_db_create_index(&idl->db, tc, index_name);
+ return ovsdb_idl_db_create_index(&idl->data, tc, index_name);
}
/* Generic comparator that can compare each index, using the custom
@@ -2384,7 +2385,7 @@ ovsdb_idl_initialize_cursor(struct ovsdb_idl *idl,
const char *index_name,
struct ovsdb_idl_index_cursor *cursor)
{
- return ovsdb_idl_db_initialize_cursor(&idl->db, tc, index_name, cursor);
+ return ovsdb_idl_db_initialize_cursor(&idl->data, tc, index_name, cursor);
}
/* ovsdb_idl_index_write_ writes a datum in an ovsdb_idl_row,
@@ -2851,7 +2852,7 @@ static struct ovsdb_idl_table *
ovsdb_idl_table_from_class(const struct ovsdb_idl *idl,
const struct ovsdb_idl_table_class *table_class)
{
- return ovsdb_idl_db_table_from_class(&idl->db, table_class);
+ return ovsdb_idl_db_table_from_class(&idl->data, table_class);
}
/* Called by ovsdb-idlc generated code. */
@@ -3072,10 +3073,10 @@ ovsdb_idl_txn_create(struct ovsdb_idl *idl)
{
struct ovsdb_idl_txn *txn;
- ovs_assert(!idl->db.txn);
- idl->db.txn = txn = xmalloc(sizeof *txn);
+ ovs_assert(!idl->data.txn);
+ idl->data.txn = txn = xmalloc(sizeof *txn);
txn->request_id = NULL;
- txn->db = &idl->db;
+ txn->db = &idl->data;
hmap_init(&txn->txn_rows);
txn->status = TXN_UNCOMMITTED;
txn->error = NULL;
@@ -4150,15 +4151,22 @@ ovsdb_idl_txn_insert(struct ovsdb_idl_txn *txn,
}
static void
-ovsdb_idl_txn_abort_all(struct ovsdb_idl *idl)
+ovsdb_idl_db_txn_abort_all(struct ovsdb_idl_db *db)
{
struct ovsdb_idl_txn *txn;
- HMAP_FOR_EACH (txn, hmap_node, &idl->db.outstanding_txns) {
+ HMAP_FOR_EACH (txn, hmap_node, &db->outstanding_txns) {
ovsdb_idl_txn_complete(txn, TXN_TRY_AGAIN);
}
}
+static void
+ovsdb_idl_txn_abort_all(struct ovsdb_idl *idl)
+{
+ ovsdb_idl_db_txn_abort_all(&idl->server);
+ ovsdb_idl_db_txn_abort_all(&idl->data);
+}
+
static struct ovsdb_idl_txn *
ovsdb_idl_db_txn_find(struct ovsdb_idl_db *db, const struct json *id)
{
@@ -4436,7 +4444,7 @@ void
ovsdb_idl_set_lock(struct ovsdb_idl *idl, const char *lock_name)
{
for (;;) {
- struct jsonrpc_msg *msg = ovsdb_idl_db_set_lock(&idl->db, lock_name);
+ struct jsonrpc_msg *msg = ovsdb_idl_db_set_lock(&idl->data, lock_name);
if (!msg) {
break;
}
@@ -4453,7 +4461,7 @@ ovsdb_idl_set_lock(struct ovsdb_idl *idl, const char
*lock_name)
bool
ovsdb_idl_has_lock(const struct ovsdb_idl *idl)
{
- return idl->db.has_lock;
+ return idl->data.has_lock;
}
/* Returns true if 'idl' is configured to obtain a lock but the database server
@@ -4461,7 +4469,7 @@ ovsdb_idl_has_lock(const struct ovsdb_idl *idl)
bool
ovsdb_idl_is_lock_contended(const struct ovsdb_idl *idl)
{
- return idl->db.is_lock_contended;
+ return idl->data.is_lock_contended;
}
static void
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
