This makes the code a lot more straightforward. Looks good, Ethan
On Thu, Jul 14, 2011 at 14:27, Ben Pfaff <[email protected]> wrote: > ovsdb_jsonrpc_server keeps track of its remotes in a shash indexed on the > remote name specified in the database Manager record, but > ovsdb_jsonrpc_server_get_remote_status() added the name returned by > jsonrpc_session_get_name() to the shash returned to the ovsdb-server code. > If that name happened to be different (which is entirely possible because > the latter returns a "canonicalized" name in some cases) then the > ovsdb-server code couldn't find it. Furthermore, if an inbound (e.g. > "ptcp:") Manager got a connection and then lost it, the status info in > that Manager never got updated to reflect that, because the code considered > that that "couldn't happen" and didn't bother to do any updates. > > This commit simplifies the logic. Now ovsdb-server just asks for a single > status record at a time, using the name that is indexed in the > ovsdb_jsonrpc_server shash, avoiding that whole issue. > --- > ovsdb/jsonrpc-server.c | 50 ++++++++++++++++++++-------------------------- > ovsdb/jsonrpc-server.h | 8 +++--- > ovsdb/ovsdb-server.c | 51 +++++++++++++++++------------------------------ > 3 files changed, 45 insertions(+), 64 deletions(-) > > diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c > index 1da1578..9f40a1a 100644 > --- a/ovsdb/jsonrpc-server.c > +++ b/ovsdb/jsonrpc-server.c > @@ -22,6 +22,7 @@ > > #include "bitmap.h" > #include "column.h" > +#include "dynamic-string.h" > #include "json.h" > #include "jsonrpc.h" > #include "ovsdb-error.h" > @@ -53,9 +54,9 @@ static void ovsdb_jsonrpc_session_close_all(struct > ovsdb_jsonrpc_remote *); > static void ovsdb_jsonrpc_session_reconnect_all(struct ovsdb_jsonrpc_remote > *); > static void ovsdb_jsonrpc_session_set_all_options( > struct ovsdb_jsonrpc_remote *, const struct ovsdb_jsonrpc_options *); > -static void ovsdb_jsonrpc_session_get_status( > +static bool ovsdb_jsonrpc_session_get_status( > const struct ovsdb_jsonrpc_remote *, > - struct shash *); > + struct ovsdb_jsonrpc_remote_status *); > > /* Triggers. */ > static void ovsdb_jsonrpc_trigger_create(struct ovsdb_jsonrpc_session *, > @@ -197,19 +198,23 @@ ovsdb_jsonrpc_server_del_remote(struct shash_node *node) > free(remote); > } > > -void > -ovsdb_jsonrpc_server_get_remote_status(const struct ovsdb_jsonrpc_server > *svr, > - struct shash *statuses) > +/* Stores status information for the remote named 'target', which should have > + * been configured on 'svr' with a call to > ovsdb_jsonrpc_server_set_remotes(), > + * into '*status'. On success returns true, on failure (if 'svr' doesn't > have > + * a remote named 'target' or if that remote is an inbound remote that has no > + * active connections) returns false. On failure, 'status' will be zeroed. > + */ > +bool > +ovsdb_jsonrpc_server_get_remote_status( > + const struct ovsdb_jsonrpc_server *svr, const char *target, > + struct ovsdb_jsonrpc_remote_status *status) > { > - struct shash_node *node; > + const struct ovsdb_jsonrpc_remote *remote; > > - shash_init(statuses); > + memset(status, 0, sizeof *status); > > - SHASH_FOR_EACH (node, &svr->remotes) { > - const struct ovsdb_jsonrpc_remote *remote = node->data; > - > - ovsdb_jsonrpc_session_get_status(remote, statuses); > - } > + remote = shash_find_data(&svr->remotes, target); > + return remote && ovsdb_jsonrpc_session_get_status(remote, status); > } > > /* Forces all of the JSON-RPC sessions managed by 'svr' to disconnect and > @@ -438,32 +443,19 @@ ovsdb_jsonrpc_session_set_all_options( > } > } > > -static void > +static bool > ovsdb_jsonrpc_session_get_status(const struct ovsdb_jsonrpc_remote *remote, > - struct shash *shash) > + struct ovsdb_jsonrpc_remote_status *status) > { > const struct ovsdb_jsonrpc_session *s; > const struct jsonrpc_session *js; > - const char *name; > - struct ovsdb_jsonrpc_remote_status *status; > struct reconnect_stats rstats; > > - /* We only look at the first session in the list. There should be only > one > - * node in the list for outbound connections. We don't track status for > - * each individual inbound connection if someone configures the DB that > - * way. Since outbound connections are the norm, this is fine. */ > if (list_is_empty(&remote->sessions)) { > - return; > + return false; > } > s = CONTAINER_OF(remote->sessions.next, struct ovsdb_jsonrpc_session, > node); > js = s->js; > - if (!js) { > - return; > - } > - name = jsonrpc_session_get_name(js); > - > - status = xzalloc(sizeof *status); > - shash_add(shash, name, status); > > status->is_connected = jsonrpc_session_is_connected(js); > status->last_error = jsonrpc_session_get_status(js); > @@ -474,6 +466,8 @@ ovsdb_jsonrpc_session_get_status(const struct > ovsdb_jsonrpc_remote *remote, > ? UINT_MAX : rstats.msec_since_connect / 1000; > status->sec_since_disconnect = rstats.msec_since_disconnect == UINT_MAX > ? UINT_MAX : rstats.msec_since_disconnect / 1000; > + > + return true; > } > > static const char * > diff --git a/ovsdb/jsonrpc-server.h b/ovsdb/jsonrpc-server.h > index dc78635..1d612cd 100644 > --- a/ovsdb/jsonrpc-server.h > +++ b/ovsdb/jsonrpc-server.h > @@ -1,4 +1,4 @@ > -/* Copyright (c) 2009, 2010 Nicira Networks > +/* Copyright (c) 2009, 2010, 2011 Nicira Networks > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -42,9 +42,9 @@ struct ovsdb_jsonrpc_remote_status { > unsigned int sec_since_disconnect; > bool is_connected; > }; > -void ovsdb_jsonrpc_server_get_remote_status( > - const struct ovsdb_jsonrpc_server *, > - struct shash * /* of 'struct ovsdb_jsonrpc_remote_status' */ ); > +bool ovsdb_jsonrpc_server_get_remote_status( > + const struct ovsdb_jsonrpc_server *, const char *target, > + struct ovsdb_jsonrpc_remote_status *); > > void ovsdb_jsonrpc_server_reconnect(struct ovsdb_jsonrpc_server *); > > diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c > index 4202b30..2d6a397 100644 > --- a/ovsdb/ovsdb-server.c > +++ b/ovsdb/ovsdb-server.c > @@ -462,12 +462,12 @@ query_db_remotes(const char *name, const struct ovsdb > *db, > > static void > update_remote_row(const struct ovsdb_row *row, struct ovsdb_txn *txn, > - const struct shash *statuses) > + const struct ovsdb_jsonrpc_server *jsonrpc) > { > + struct ovsdb_jsonrpc_remote_status status; > struct ovsdb_row *rw_row; > const char *target; > - const struct ovsdb_jsonrpc_remote_status *status; > - char *keys[4], *values[4]; > + char *keys[8], *values[8]; > size_t n = 0; > > /* Get the "target" (protocol/host/port) spec. */ > @@ -475,43 +475,36 @@ update_remote_row(const struct ovsdb_row *row, struct > ovsdb_txn *txn, > /* Bad remote spec or incorrect schema. */ > return; > } > - > - /* Prepare to modify this row. */ > rw_row = ovsdb_txn_row_modify(txn, row); > - > - /* Find status information for this target. */ > - status = shash_find_data(statuses, target); > - if (!status) { > - /* Should never happen, but just in case... */ > - return; > - } > + ovsdb_jsonrpc_server_get_remote_status(jsonrpc, target, &status); > > /* Update status information columns. */ > + write_bool_column(rw_row, "is_connected", status.is_connected); > > - write_bool_column(rw_row, "is_connected", > - status->is_connected); > - > - keys[n] = xstrdup("state"); > - values[n++] = xstrdup(status->state); > - if (status->sec_since_connect != UINT_MAX) { > + if (status.state) { > + keys[n] = xstrdup("state"); > + values[n++] = xstrdup(status.state); > + } > + if (status.sec_since_connect != UINT_MAX) { > keys[n] = xstrdup("sec_since_connect"); > - values[n++] = xasprintf("%u", status->sec_since_connect); > + values[n++] = xasprintf("%u", status.sec_since_connect); > } > - if (status->sec_since_disconnect != UINT_MAX) { > + if (status.sec_since_disconnect != UINT_MAX) { > keys[n] = xstrdup("sec_since_disconnect"); > - values[n++] = xasprintf("%u", status->sec_since_disconnect); > + values[n++] = xasprintf("%u", status.sec_since_disconnect); > } > - if (status->last_error) { > + if (status.last_error) { > keys[n] = xstrdup("last_error"); > values[n++] = > - xstrdup(ovs_retval_to_string(status->last_error)); > + xstrdup(ovs_retval_to_string(status.last_error)); > } > write_string_string_column(rw_row, "status", keys, values, n); > } > > static void > update_remote_rows(const struct ovsdb *db, struct ovsdb_txn *txn, > - const char *remote_name, const struct shash *statuses) > + const char *remote_name, > + const struct ovsdb_jsonrpc_server *jsonrpc) > { > const struct ovsdb_table *table, *ref_table; > const struct ovsdb_column *column; > @@ -541,7 +534,7 @@ update_remote_rows(const struct ovsdb *db, struct > ovsdb_txn *txn, > > ref_row = ovsdb_table_get_row(ref_table, &datum->keys[i].uuid); > if (ref_row) { > - update_remote_row(ref_row, txn, statuses); > + update_remote_row(ref_row, txn, jsonrpc); > } > } > } > @@ -552,20 +545,16 @@ update_remote_status(const struct ovsdb_jsonrpc_server > *jsonrpc, > const struct sset *remotes, struct ovsdb *db) > { > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > - struct shash statuses; > struct ovsdb_txn *txn; > const bool durable_txn = false; > struct ovsdb_error *error; > const char *remote; > > - /* Get status of current connections. */ > - ovsdb_jsonrpc_server_get_remote_status(jsonrpc, &statuses); > - > txn = ovsdb_txn_create(db); > > /* Iterate over --remote arguments given on command line. */ > SSET_FOR_EACH (remote, remotes) { > - update_remote_rows(db, txn, remote, &statuses); > + update_remote_rows(db, txn, remote, jsonrpc); > } > > error = ovsdb_txn_commit(txn, durable_txn); > @@ -573,8 +562,6 @@ update_remote_status(const struct ovsdb_jsonrpc_server > *jsonrpc, > VLOG_ERR_RL(&rl, "Failed to update remote status: %s", > ovsdb_error_to_string(error)); > } > - > - shash_destroy_free_data(&statuses); > } > > /* Reconfigures ovsdb-server based on information in the database. */ > -- > 1.7.4.4 > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
