[ovs-dev] [PATCH monitor_cond V5 04/18] ovsdb: generate update notifications for monitor_cond session

Liran Schour LIRANS at il.ibm.com
Tue Mar 22 21:24:14 UTC 2016


Ben Pfaff <blp at ovn.org> wrote on 22/03/2016 07:23:33 PM:

> On Fri, Mar 04, 2016 at 08:08:59AM +0000, Liran Schour wrote:
> > Hold session's conditions in ovsdb_monitor_session_condition. Pass it
> > to ovsdb_monitor for generating "update2" notifications.
> > Add functions that can generate "update2" notification for a
> > "monitor_cond" session.
> > JSON cache is enabled only for session's with true condition only.
> > "monitor_cond" and "monitor_cond_change" are RFC 7047 extensions
> > described by ovsdb-server(1) manpage.
> > 
> > Signed-off-by: Liran Schour <lirans at il.ibm.com>
> 
> I think that ovsdb_monitor_table_condition_set() has a memory leak in
> the error case; it does not free the 'error' returned to it.
> 

Right. Will fix that.

> I'm a little confused about ovsdb_monitor_session_condition.  Why is
> this a separate data structure?  That is, why is there a separate shash
> of conditions rather than incorporating a condition into
> ovsdb_monitor_table?
> 

A single ovsdb_monitor instance can be used by many jsonrpc sessions (as 
long as they share the same set of tables and columns). 
Ovsdb_monitor_condition_session is holding the condition state per each 
jsonrpc session. Each session, in ovsdb_monitor, can have a different set 
of conditions and it is being represented by 
ovsdb_monitor_session_condition with ovsdb_monitor_table_condition per 
table.
The trivial option was to divide ovsdb_monitor into 2 different instances 
if we have 2 sessions with different sets of conditions. But, condition is 
changing during the lifetime of monitor session. And by holding sessions 
with different conditions under the same ovsdb_monitor we can reduce the 
computation needed to insert row changes to ovsdb_monitor. (insert once 
instead of twice)

> If a separate data structure is really needed then possibly it would be
> better to only include the non-"true" conditions in the shash.  Then it
> would be easy to determine how many non-"true" conditions there were
> (shash_count(conditions)) and hence whether there were any at all.
> 

Something like this can be done however I do not support this. Conditions 
might change and it is needed to keep the condition state even if it is 
true (implicit or explicit).

> I'll continue reviewing the series once I understand this point.

Hope that it is clearer now. Thanks.





More information about the dev mailing list