[ovs-dev] [PATCH ovn 2/2] ovn-nbd: Calculate 'up' state for logical ports.

[Ben Pfaff]

On Thu, Apr 02, 2015 at 11:24:51AM -0400, Russell Bryant wrote:
> On 04/02/2015 11:18 AM, Ben Pfaff wrote:
> > On Wed, Apr 01, 2015 at 08:04:21PM -0400, Russell Bryant wrote:
> >> When the state of the chassis column in the Bindings table changes for
> >> any row, ovn-nbd will notice and trigger recalculating the 'up' state
> >> for all logical ports.
> >>
> >> This can be tesed manually by starting up ovs-sandbox with ovn support
> >> enabled, running ovn-nbd, and then running the following commands:
> >>
> >>   ovn-nbctl lswitch-add sw0
> >>   ovn-nbctl lport-add sw0-port0 sw0
> >>   port_uuid=$(ovn-nbctl lport-list sw0 | awk '{print $1}')
> >>   ovsdb-client transact "[\"OVN\",\
> >>     {\"uuid-name\":\"rowd4eca046_9233_4094_bc55_e28dd49217f9\",\
> >>     \"row\":{\"logical_port\":\"$port_uuid\",\"chassis\":\"hostname\"},\
> >>     \"op\":\"insert\",\"table\":\"Bindings\"}]"
> >>
> >> ovn-nbd will then see that the 'chassis' column is set in the Bindings
> >> row for the logical port and will mark the logical port as 'up' in the
> >> northbound db.
> >>
> >> Signed-off-by: Russell Bryant <rbryant at redhat.com>
> > 
> > "sparse" complained:
> >     ../ovn/ovn-nbd.c:106:51: warning: expression using sizeof bool
> >     ../ovn/ovn-nbd.c:109:51: warning: expression using sizeof bool
> > about this code:
> >> +        if (*bindings->chassis && (!lport->up || !*lport->up)) {
> >> +            bool up = true;
> >> +            nbrec_logical_port_set_up(lport, &up, sizeof up);
> >> +        } else if (!*bindings->chassis && (!lport->up || *lport->up)) {
> >> +            bool up = false;
> >> +            nbrec_logical_port_set_up(lport, &up, sizeof up);
> > 
> > While I don't think there's anything wrong with "sizeof bool", I didn't
> > think that "sizeof" was a reasonable way to get the number of elements
> > in 'up' (which is just 1 of course), so I changed these to literal 1s.
> > 
> > With that change, I applied both patches to the ovn branch.  Thank you!
> 
> Thanks for the fix!  I guess I misinterpreted the meaning of that 3rd
> argument.
> 
> The prototype for that function seems kind of odd, anyway.  It's just a
> single boolean, so something like (lport, true) seems sufficient.  The
> same applies to how "up" appears in the generated struct.  It probably
> has something to do with convenience in how the code is auto generated
> though, so it's not a big deal.

Yes, it's a little odd.  It's because it's really a tri-valued column:
it can be true or false or empty.  But the interface could still be a
little nicer.