| Message ID | 20200519221231.168406-1-ihrachys@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [ovs-dev,ovn] Don't leak values other than 1 or 0 as bool return values on C89 compiler | expand |
Acked-by: Ankur Sharma <ankur.sharma@nutanix.com>
Regards,
Ankur
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 --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;
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(-)