[ovs-dev] [PATCH 1/1] ovsdb-idl: Add additional support for change tracking.

Ansari, Shad shad.ansari at hpe.com
Wed Oct 28 02:29:04 UTC 2015


> I see a few style errors, e.g. the { should be on a line of its own
> here:
>     static bool
>     ovsdb_idl_track_is_set(struct ovsdb_idl_table *table) {
> 
> ovsdb_idl_track_is_set() seems expensive given that it is being executed
> on every row destroy.
> 
> I'm not really pleased with the idea that this creates two different
> modes for ovsdb_idl_row_destroy() that have significantly different
> behavior and need to be tested separately.  In practice, that testing
> will be incomplete (I note that you added tests--thank you!--but of
> course ovs-vswitchd is only going to use one mode or the other, and it's
> the main user of ovsdb-idl in the OVS testsuite).
> 
> So, I'd be happier if we could reduce this to a single mode.  That seems
> reasonable to me: have ovsdb_idl_row_destroy() always queue up a row for
> tracking, then at a high level (the end of ovsdb_idl_run() may be a
> reasonable choice) automatically destroy the tracking queues for the
> tables which the client doesn't care to track.  Does that sound OK to
> you?
> 
> I'm a little nervous about ovsdb_idl_track_add_column().  It seems to
> have a pitfall in that it does nothing, silently, if the column isn't
> already set up for alerts.  It seems to me that it would be harder to
> misuse if it either turned on alerts automatically or if it
> assert-failed if they were not already turned on.
> 
> Thanks,
> 
> Ben.

Patch is re-submitted (on ovs-dev mailing list and a pull request) addressing all above comments.




More information about the dev mailing list