On Mar 14, 2011, at 10:11 AM, Ben Pfaff wrote:
> Thanks for so patiently refining this.
Thank you for so patiently reviewing all the patches!
> I think that the function-level comment on
> reconnect_get_last_connect_elapsed() is incorrect: if 'fsm' connected,
> then the number of milliseconds since 'fsm' was last connected is 0,
> but this function won't necessarily return that. Maybe "the number of
> milliseconds since 'fsm' last successfully connected to its peer" or
> something else that indicates that it's the edge disconnected ->
> connected that we're tracking?
>
> Same feedback for reconnect_get_last_disconnect_elapsed().
I agree, that should be explained more clearly. How about this?
---cut here---
diff --git a/lib/reconnect.c b/lib/reconnect.c
index 3f49245..c169016 100644
--- a/lib/reconnect.c
+++ b/lib/reconnect.c
@@ -642,8 +642,9 @@ reconnect_is_connected(const struct reconnect *fsm)
return is_connected_state(fsm->state);
}
-/* Returns the number of milliseconds since 'fsm' was last connected
- * to its peer. */
+/* Returns the number of milliseconds since 'fsm' last successfully connected
+ * to its peer (even if it has since disconnected). Returns UINT_MAX if never
+ * connected. */
unsigned int
reconnect_get_last_connect_elapsed(const struct reconnect *fsm,
long long int now)
@@ -652,8 +653,9 @@ reconnect_get_last_connect_elapsed(const struct reconnect
*fsm,
: now - fsm->last_connected;
}
-/* Returns the number of milliseconds since 'fsm' was last disconnected
- * from its peer. */
+/* Returns the number of milliseconds since 'fsm' last disconnected
+ * from its peer (even if it has since reconnected). Returns UINT_MAX if never
+ * disconnected. */
unsigned int
reconnect_get_last_disconnect_elapsed(const struct reconnect *fsm,
long long int now)
---cut here---
> In update_remote_row(), I would have expected the key/value pairs for
> sec_since_connect and sec_since_disconnect to be omitted entirely,
> instead of being set to empty values, if there have been no
> connections or disconnections.
I agree. Pete says he can live with it either way, but he thinks it's "more
self-documenting" if the keys always exist. I think he has a point.
---cut here---
diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index b3790cf..8d44d84 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -495,12 +495,14 @@ update_remote_row(const struct ovsdb_row *row, struct
ovsdb_txn *txn,
keys[n] = xstrdup("state");
values[n++] = xstrdup(status->state);
- keys[n] = xstrdup("sec_since_connect");
- values[n++] = status->sec_since_connect == UINT_MAX
- ? xstrdup("") : xasprintf("%u", status->sec_since_connect);
- keys[n] = xstrdup("sec_since_disconnect");
- values[n++] = status->sec_since_disconnect == UINT_MAX
- ? xstrdup("") : xasprintf("%u", status->sec_since_disconnect);
+ if (status->sec_since_connect != UINT_MAX) {
+ keys[n] = xstrdup("sec_since_connect");
+ values[n++] = xasprintf("%u", status->sec_since_connect);
+ }
+ if (status->sec_since_disconnect != UINT_MAX) {
+ keys[n] = xstrdup("sec_since_disconnect");
+ values[n++] = xasprintf("%u", status->sec_since_disconnect);
+ }
if (status->last_error) {
keys[n] = xstrdup("last_error");
values[n++] =
---cut here---
> Did you carefully look through the updated test results to make sure
> that they make sense? I did for an earlier version of this patch, but
> I didn't yet for this version. If you haven't, let me know, and I
> will.
I did pore over them carefully, since I made pretty major changes to the way
connect and disconnect events get reported. I spent quite a bit of time making
sure that the results were what I expected. It certainly wouldn't hurt to have
someone else double-check, though. In general I made the reconnect tests not
report information about connect or disconnect times unless a connect or
disconnect event had occurred in that cycle, or if the times weren't what were
expected (last time plus delta).
thanks,
-Andrew
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
