diff mbox series

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

Message ID 20200519221231.168406-1-ihrachys@redhat.com
State New
Headers show
Series [ovs-dev,ovn] Don't leak values other than 1 or 0 as bool return values on C89 compiler | expand

Commit Message

Ihar Hrachyshka May 19, 2020, 10:12 p.m. UTC
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@redhat.com>
---
 controller/pinctrl.c | 6 +++---
 ic/ovn-ic.c          | 2 +-
 lib/ovn-util.c       | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

Comments

Ankur Sharma May 19, 2020, 11:24 p.m. UTC | #1
Acked-by: Ankur Sharma <ankur.sharma@nutanix.com>

Regards,
Ankur
Dumitru Ceara May 20, 2020, 7:30 a.m. UTC | #2
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@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;
>
diff mbox series

Patch

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;
 }
 
 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;