[ovs-dev] [PATCH ovn] Don't leak values other than 1 or 0 as bool return values on C89 compiler

Dumitru Ceara dceara at redhat.com
Wed May 20 07:30:59 UTC 2020


On 5/20/20 12:12 AM, Ihar Hrachyshka wrote:
> While the code base makes use of bool type defined in C99, we don't assume C99
> semantics for integer or pointer conversions to bool. (This is explained in the
> project coding style guide.) In C89 environments, it's important to normalize
> bool variables and returned values to 1 | 0 to avoid issues down the line.
> 
> This patch updates all functions in the tree that return bool values and that
> could, in degenerate cases, return bools set to values different from 1 | 0.
> 
> Signed-off-by: Ihar Hrachyshka <ihrachys at redhat.com>
> ---
>  controller/pinctrl.c | 6 +++---
>  ic/ovn-ic.c          | 2 +-
>  lib/ovn-util.c       | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index bea446c89..18b5c68dd 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -1674,7 +1674,7 @@ pinctrl_handle_tcp_reset(struct rconn *swconn, const struct flow *ip_flow,
>  static bool
>  is_dhcp_flags_broadcast(ovs_be16 flags)
>  {
> -    return flags & htons(DHCP_BROADCAST_FLAG);
> +    return !!(flags & htons(DHCP_BROADCAST_FLAG));
>  }
>  
>  /* Called with in the pinctrl_handler thread context. */
> @@ -4606,7 +4606,7 @@ pinctrl_ip_mcast_handle_igmp(struct ip_mcast_snoop *ip_ms,
>          break;
>      }
>      ovs_rwlock_unlock(&ip_ms->ms->rwlock);
> -    return group_change;
> +    return !!group_change;
>  }

Hi Ihar,

Thanks for fixing this! While this is correct, I'm wondering if in the
igmp/mld cases it would be cleaner to be explicit and add something like:

if (mcast_snooping_add_report(ip_ms->ms, pkt_in, IP_MCAST_VLAN,
                              port_key_data)) {
    group_change = true;
}

and respectively:

if (mcast_snooping_add_mld(ip_ms->ms, pkt_in, IP_MCAST_VLAN,
                           port_key_data)) {
    group_change = true;
}

This because the other mcast_snooping_* functions return bool, i.e.,
true if the IGMP/MLD packet triggered a change in the multicast group
memberships.

What do you think?

Thanks,
Dumitru

>  
>  static bool
> @@ -4654,7 +4654,7 @@ pinctrl_ip_mcast_handle_mld(struct ip_mcast_snoop *ip_ms,
>          break;
>      }
>      ovs_rwlock_unlock(&ip_ms->ms->rwlock);
> -    return group_change;
> +    return !!group_change;
>  }
>  
>  static void
> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> index a1ed25623..9e7ceaf98 100644
> --- a/ic/ovn-ic.c
> +++ b/ic/ovn-ic.c
> @@ -525,7 +525,7 @@ get_router_uuid_by_sb_pb(struct ic_context *ctx,
>  {
>      const struct sbrec_port_binding *router_pb = find_peer_port(ctx, sb_pb);
>      if (!router_pb || !router_pb->datapath) {
> -        return NULL;
> +        return false;
>      }
>  
>      return smap_get_uuid(&router_pb->datapath->external_ids, "logical-router",
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index 3482edb8d..10345b012 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -210,7 +210,7 @@ extract_ip_addresses(const char *address, struct lport_addresses *laddrs)
>  {
>      int ofs;
>      if (parse_and_store_addresses(address, laddrs, &ofs, false)) {
> -        return (laddrs->n_ipv4_addrs || laddrs->n_ipv6_addrs);
> +        return !!(laddrs->n_ipv4_addrs || laddrs->n_ipv6_addrs);
>      }
>  
>      return false;
> 



More information about the dev mailing list