[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