On Thu, Apr 09, 2015 at 06:40:24PM -0700, Andy Zhou wrote:
> Currently, each monitor table contains a single hmap 'changes' to
> track updates. This patch introduces a new data structure
> 'ovsdb_monitor_changes' that stores the updates 'rows' tagged by
> its first commit transaction id. Each 'ovsdb_monitor_changes' is
> refenece counted allowing multiple jsonrpc_monitors to share them.
>
> The next patch will allow each ovsdb monitor table to store a list
> of 'ovsdb_monitor_changes'. This patch stores only one, same as
> before.
>
> Signed-off-by: Andy Zhou <az...@nicira.com>
> Acked-by: Ben Pfaff <b...@nicira.com>
>
> ---
> v1->v2: fix asserts in ovsdb_monitor_table_track_changes()
> fix a memory leak in ovsdb_monitor_table_add_changes();
> remove ovsdb_monitor_renew_tracking_changes(), transaction
> number update is now part of ovsdb_monitor_compose_update()
>
> v2->v3: no change
I'm not sure I understand the sentence beginning 'Generate' in the
following comment. Do you mean that generating the update when 'n_refs
== 1' will destroy the whole "struct ovsdb_monitor_changes"? (There's
no object obviously named 'changes'.)
+/* Contains 'struct ovsdb_monitor_row's for rows that have been
+ * updated but not yet flushed to all the jsonrpc connection.
+ *
+ * 'n_refs' represent the number of jsonrpc connections that have
+ * not received updates. Generate the update for the last jsonprc
+ * connection will also remove rows contained in 'changes'.
+ *
+ * 'transaction' stores the first update's transaction id.
+ * */
+struct ovsdb_monitor_changes {
+ struct ovsdb_monitor_table *mt;
+ struct hmap rows;
+ int n_refs;
+ uint64_t transaction;
+};
I'm not sure that the new ovs_assert() in ovsdb_monitor_row_find() is
valuable, because we'll get a segfault anyway in the next line if it
would fail.
I think that the change to ovsdb/jsonrpc-server.c in this patch is only
a stylistic change. If so, could it be folded into whatever previous
patch previously changed (added?) that code?
It looks like both callers of ovsdb_monitor_changes_destroy_rows()
just afterwards free() the ovsdb_monitor_changes object. Could
ovsdb_monitor_changes_destroy_rows() be renamed to
ovsdb_monitor_changes_destroy() with that free() call folded in?
I would write ovsdb_monitor_table_untrack_changes() slightly more
compactly, from:
struct ovsdb_monitor_changes *changes;
changes = mt->changes;
if (changes) {
to:
struct ovsdb_monitor_changes *changes = mt->changes;
if (changes) {
I notice now that ovsdb_monitor_compose_update() lacks a comment
explaining the 'unflushed' parameter. It would be nice to add one.
Acked-by: Ben Pfaff <b...@nicira.com>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
