On Fri, May 29, 2015 at 12:00 PM, Ben Pfaff <[email protected]> wrote: > 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 <[email protected]> >> Acked-by: Ben Pfaff <[email protected]> >> >> --- >> 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'.) > I meant rows only. But with fix suggested blow, it becomes the whole object. Fixed comment accordingly. > +/* 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. Removed. > > 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? Fixed. Thanks for the suggestion. It is cleaner this way. > > 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) { > Fixed.
> I notice now that ovsdb_monitor_compose_update() lacks a comment > explaining the 'unflushed' parameter. It would be nice to add one. > Added as follows: /* 'unflushed' should point to value that is the transaction ID that did * was not updated. The update contains changes between * ['unflushed, ovsdb->n_transcations]. Before the function returns, this * value will be updated to ovsdb->n_transactions + 1, ready for the next * update. */ _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
