[ovs-dev,mointor2,5/9] ovsdb: generate update2 notification for a monitor2 session

On Wed, Oct 21, 2015 at 09:45:27PM -0700, Andy Zhou wrote:
> Add functions that can generate "update2" notification for a
> "monitor2" session. "monitor2" and "update2" are RFC 7047 extensions
> deescribed by ovsdb-server(1) manpage. See the manpage changes
> for more details.
> 
> Signed-off-by: Andy Zhou <azhou@nicira.com>

Thanks for implementing this!

I think that there is only a one-character difference between
ovsdb_monitor_compose_update() and ovsdb_monitor_compose_update2(): the
first calls ovsdb_monitor_compose_row_update(), the latter calls
ovsdb_monitor_compose_row_update2().  If so, and if they won't diverge a
lot in later patches, then I'd suggest finding a way to avoid so much
code duplication.  You could, for example, use a conditional test inside
the function, or a function pointer.

There's a fair amount of code duplication from
ovsdb_monitor_compose_row_update() and
ovsdb_monitor_compose_row_update2(), also.  Is there a way to cleanly
reduce that?

Manpage
-------

I have some suggestions for the manpage.  I'm appending an incremental
diff.

Thanks,

Ben.

On Tue, Nov 10, 2015 at 9:23 AM, Ben Pfaff <blp@ovn.org> wrote:
> On Wed, Oct 21, 2015 at 09:45:27PM -0700, Andy Zhou wrote:
>> Add functions that can generate "update2" notification for a
>> "monitor2" session. "monitor2" and "update2" are RFC 7047 extensions
>> deescribed by ovsdb-server(1) manpage. See the manpage changes
>> for more details.
>>
>> Signed-off-by: Andy Zhou <azhou@nicira.com>
>
> Thanks for implementing this!
>
> I think that there is only a one-character difference between
> ovsdb_monitor_compose_update() and ovsdb_monitor_compose_update2(): the
> first calls ovsdb_monitor_compose_row_update(), the latter calls
> ovsdb_monitor_compose_row_update2().  If so, and if they won't diverge a
> lot in later patches, then I'd suggest finding a way to avoid so much
> code duplication.  You could, for example, use a conditional test inside
> the function, or a function pointer.

Thanks for pointing this out. I was planing for a bigger change than
it turns out to be.
Collapse both function into a single ovsdb_monitor_compose_update()
function that takes a
callback function that will generate row updates.

>
> There's a fair amount of code duplication from
> ovsdb_monitor_compose_row_update() and
> ovsdb_monitor_compose_row_update2(), also.  Is there a way to cleanly
> reduce that?

Yes, there are some logic duplications at the beginning part of both
functions, refactor out
common logics into a separate function in V2. Hope it looks better now.
>
> Manpage
> -------
>
Thanks for working on this. Folded those changes into V2.

> I have some suggestions for the manpage.  I'm appending an incremental
> diff.
>
> Thanks,
>
> Ben.
>
> diff --git a/ovsdb/ovsdb-server.1.in b/ovsdb/ovsdb-server.1.in
> index 1d68fd6..6c85729 100644
> --- a/ovsdb/ovsdb-server.1.in
> +++ b/ovsdb/ovsdb-server.1.in
> @@ -248,84 +248,87 @@ monitors with the same identifier.
>  .IP "4.1.12. Monitor2"
>  A new monitor method added in Open vSwitch version 2.5. Monitor2 allows
>  for more efficient update notifications (described below).
> -
> -Monitor method described in Section 4.1.5 also applies to monitor2, with
> -the following exceptions.
> -
> +.IP
> +The monitor method described in Section 4.1.5 also applies to
> +monitor2, with the following exceptions.
> +.
>  .RS
>  .IP \(bu
> -RPC request method becomes "montior2"
> +RPC request method becomes "monitor2".
>  .IP \(bu
>  Replay result follows <table-updates2>, described in Section 4.1.13.
>  .IP \(bu
>  Subsequent changes are sent to the client using the "update2" monitor
>  notification, described in Section 4.1.13
>  .RE
> -
> +.
> +.IP
>  Both monitor and monitor2 sessions can exist concurrently. However,
> -monitor and monitor2 shares the smae <json-value> parameter space; it
> -must be unique among all monitor and monitor2 seesion.
> -
> +monitor and monitor2 shares the same <json-value> parameter space; it
> +must be unique among all monitor and monitor2 sessions.
> +.
>  .IP "4.1.13. Update2 notification"
>  The "update2" notification is sent by the server to the client to report
>  changes in tables that are being monitored following a "monitor2" request
> -as described above. The notifaction has the following members:
> -
> +as described above. The notification has the following members:
> +.
>  .RS
>  .nf
>  "method": "update2"
>  "params": [<json-value>, <table-updates2>]
> -"id": NULL
> +"id": null
>  .fi
>  .RE
> -
> +.
>  .IP
>  The <json-value> in "params" is the same as the value passed as the
>  <json-value>  in "params" for the corresponding "monitor" request.
>  <table-updates2> is an object that maps from a table name to a <table-update2>.
> -A <table-update2> is an object htat maps from row's UUID to a <row-update2> object. A <row-update2> is an object with one of the following members:
> -
> +A <table-update2> is an object that maps from row's UUID to a <row-update2> object. A <row-update2> is an object with one of the following members:
> +.
>  .RS
> -.nf
> -"initial": <row>   present for "initial' updates
> -"insert":  <row>   present for "insert' updates
> -"delete":  <row>   present for "delete' updates
> -"modify":  <row>   present for "modify' updates
> -.fi
> +.IP "\(dqinitial\(dq: <row>"
> +present for "initial" updates
> +.IP "\(dqinsert\(dq: <row>"
> +present for "insert" updates
> +.IP "\(dqdelete\(dq: <row>"
> +present for "delete" updates
> +.IP "\(dqmodify\(dq: <row>"
> +present for "modify" updates
>  .RE
> -
> +.
>  .IP
>  The format of <row> is described in Section 5.1.
> -
> +.
>  .IP
> -<row> is always a NULL object with a "delete" object. In "initial" and
> -"insert" objects, <row> omits columns whose values equal to default
> +<row> is always a null object for a "delete" update. In "initial" and
> +"insert" updates, <row> omits columns whose values equal the default
>  value of the column type.
> -
> +.
>  .IP
> -In "modify" object, <row> contains only the columns that are modified.
> +For a "modify" update, <row> contains only the columns that are modified.
>  <row> stores the difference between the old and new value for those columns,
>  as described below.
> -
> +.
>  .IP
>  For columns with single value, the difference is the value of the new
>  column.
> -
> +.
>  .IP
>  The difference between two sets are all elements that only belong
>  to one of the sets.
> -
> +.
>  .IP
> -The difference between two maps are all key value pairs whose keys appears
> -in only one of the maps. And elements with overlapping
> -keys between the two maps, but are associated with different values.
> -For those elements, <row> stores the key value pair from the new column.
> -
> +The difference between two maps are all key-value pairs whose keys
> +appears in only one of the maps, plus the key-value pairs whose keys
> +appear in both maps but with different values.  For the latter
> +elements, <row> includes the value from the new column.
> +.
>  .IP
> -Note that initial views of rows are not presented in update2 notifications,
> +Initial views of rows are not presented in update2 notifications,
>  but in the response object to the monitor2 request. The formatting of the
>  <table-updates2> object, however, is the same in either case.
> -
> +.
>  .IP "5.1. Notation"
>  For <condition>, RFC 7047 only allows the use of \fB!=\fR, \fB==\fR,
>  \fBincludes\fR, and \fBexcludes\fR operators with set types.  Open