>
>
> First, I like this. Thank you for doing the work.
>
> 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.
>
Ben,
I had submitted a revised patch that addresses your comments. Let me know if
this looks better:
http://openvswitch.org/pipermail/dev/2015-October/061665.html
-shad
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
