[ovs-dev] [PATCH] ovsdb-cs: Consider all tables when computing expected cond seqno.
Dumitru Ceara
dceara at redhat.com
Fri May 7 19:13:39 UTC 2021
On 5/7/21 9:09 PM, Ben Pfaff wrote:
> 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.
We're discussing whether OVN stable branches should be using OVS
submodules pointing to OVS stable branches so I think it still makes
sense to backport to the LTS and supported stable branches.
I see some OVN tests failing every now and then because of the bug this
patch fixes so it would help reducing the false positives especially on
stable branches.
>
>>> 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 <blp at ovn.org>
>>>
>>> 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 :-)
>
:)
More information about the dev
mailing list