On Fri, May 07, 2021 at 09:01:01PM +0200, Dumitru Ceara wrote:
> On 5/7/21 7:55 PM, Ben Pfaff wrote:
> > It's not very often that we get a bug fix for a 4 1/2 year old patch. I
> > hope you're willing to backport.
>
> Sure, no problem. I can send backports for all relevant branches once
> this is accepted.
>
> I think it wasn't really spotted before because there aren't so many
> users of the IDL/CS modules that use the return value of
> ovsdb_idl_set_condition()/ovsdb_cs_set_condition().
>
> OVN started doing that since be4b7719dc0d ("ovsdb-idlc: Return expected
> sequence number while setting conditions.") and 58e5d32b011b
> ("ovn-controller: Fix nb_cfg update with monitor_cond_change in flight.").
Oh, I see.
If this is a bug that doesn't actually affect older versions, because
the return value wasn't used for anything that mattered, then there's no
need to backport it.
> > I thought about asking for a test but this seems really challenging to
> > test. Distributed systems are hard.
> >
> > I've stared at this and thought about it and it seems correct. Here's
> > my ack:
> > Acked-by: Ben Pfaff <[email protected]>
> >
> > I have tiny comments on the code itself. One is that any_req_cond is
> > only used in one branch of the 'if', so it could be calculated just on
> > that branch. The other is that any_req_cond is a bool so that
> > the !! in !!any_req_cond can be removed.
>
> Ah, I was under the (wrong) impression that there's no guarantee in the
> standard about bool having only 0 and 1 as possible values. I'll send a
> v2 taking care of your comments.
C99 introduced bool as a 1-bit type. Compilers only slowly updated to
add support for that, so for a long time C programmers use conditional
compilation with a fallback to a typedef to a character type, which
obviously does have more than 0 and 1 as possible values. All the
compilers that OVS/OVN care about do support bool as a 1-bit type now,
so it's no longer anything to worry about. We should update
coding-style.rst to reflect this; it still has the following:
- ``bool`` and ``<stdbool.h>``, but don't assume that ``bool`` or
``_Bool`` can only take on the values ``0`` or ``1``, because this
behavior can't be simulated on C89 compilers.
Also, don't assume that a conversion to ``bool`` or ``_Bool``
follows C99 semantics, i.e. use ``(bool) (some_value != 0)``
rather than ``(bool) some_value``. The latter might produce
unexpected results on non-C99 environments. For example, if
``bool`` is implemented as a typedef of char and ``some_value =
0x10000000``.
However, in this case, the variable in question is only assigned true or
false, which are always defined as 1 or 0, so it works correctly even if
it has more than 1 bit.
Boy, that's a lot of fuss for being able to remove !!, maybe I should
have stayed quiet :-)
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
