[ovs-dev] [PATCH v3 ovn 3/4] ovn-northd: Refactor parsing of *_force_snat_ip.

[Dumitru Ceara]

On 9/22/20 8:11 AM, Numan Siddique wrote:
> 
> 
> On Mon, Sep 21, 2020 at 12:32 PM Han Zhou <hzhou at ovn.org
> <mailto:hzhou at ovn.org>> wrote:
> 
>     On Thu, Sep 17, 2020 at 5:51 AM Dumitru Ceara <dceara at redhat.com
>     <mailto:dceara at redhat.com>> wrote:
>     >
>     > Avoid reparsing the *_force_snat_ip addresses for every logical router
>     port.
>     > These addresses are defined once for the whole router.
>     >
> 
>     The commit message is a little misleading. Originally it wasn't
>     reparsing
>     for every logical router port. It was per-router. This patch avoids one
>     extra parsing.
> 
>     In addition, another minor comment is that the function name
>     "empty_lport_addresses" may be "lport_addresses_is_empty" to follow the
>     naming convention of a lot of xxx_is_empty() function names in OVN/OVS.
> 
>     With these addressed:
>     Acked-by: Han Zhou <hzhou at ovn.org <mailto:hzhou at ovn.org>>
> 
> 
> 
> Hi Dumitru,
> 
> The compilation fails for clang with the below messages
> 
> *****
> 
> libtool: link: clang -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare
> -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum
> -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes
> -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers
> -Wthread-safety -fno-strict-aliasing -Wswitch-bool
> -Wlogical-not-parentheses -Wsizeof-array-argument -Wshift-negative-value
> -Qunused-arguments -Wshadow -Wno-null-pointer-arithmetic
> -Warray-bounds-pointer-arithmetic -Werror -Werror -g -O2 -o ic/ovn-ic
> ic/ovn-ic.o  lib/.libs/libovn.a
> /home/nusiddiq/work/ovs_ovn/ovs/ovsdb/.libs/libovsdb.a
> /home/nusiddiq/work/ovs_ovn/ovs/lib/.libs/libopenvswitch.a -lssl
> -lcrypto -lcap-ng
> /home/nusiddiq/work/ovs_ovn/ovs/lib/.libs/libopenvswitchavx512.a
> -lpthread -lrt -lm -lunbound
> ../northd/ovn-northd.c:681:40: error: address of
> 'od->dnat_force_snat_addrs.n_ipv4_addrs' will always evaluate to 'true'
> [-Werror,-Wpointer-bool-conversion]
>         if (&od->dnat_force_snat_addrs.n_ipv4_addrs) {
>         ~~   ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
> ../northd/ovn-northd.c:685:40: error: address of
> 'od->dnat_force_snat_addrs.n_ipv6_addrs' will always evaluate to 'true'
> [-Werror,-Wpointer-bool-conversion]
>         if (&od->dnat_force_snat_addrs.n_ipv6_addrs) {
>         ~~   ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
> *******
> 
>  I think there is no need for '&'.
> 
> Thanks
> Numan
> 

Hi Numan,

You're right, this is a very nasty copy/paste error. I'll fix it in a
new revision. I'll also try to add a test to spot this kind of issues
earlier.

Regards,
Dumitru