Re: [PATCH 1/6] dhcpv6: Use for loop instead of goto to avoid false positive cppcheck warning

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3807 bytes --]

On Thu, Nov 07, 2024 at 07:43:26PM +0100, Stefano Brivio wrote:
> cppcheck 2.16.0 reports:
> 
> dhcpv6.c:334:14: style: The comparison 'ia_type == 3' is always true. [knownConditionTrueFalse]
>  if (ia_type == OPT_IA_NA) {
>              ^
> dhcpv6.c:306:12: note: 'ia_type' is assigned value '3' here.
>  ia_type = OPT_IA_NA;
>            ^
> dhcpv6.c:334:14: note: The comparison 'ia_type == 3' is always true.
>  if (ia_type == OPT_IA_NA) {
>              ^
> 
> this is not really the case as we set ia_type to OPT_IA_TA and then
> jump back.
> 
> Anyway, there's no particular reason to use a goto here: add a trivial
> foreach() macro to go through elements of an array and use it instead.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  dhcpv6.c | 47 +++++++++++++++++++++--------------------------
>  util.h   |  3 +++
>  2 files changed, 24 insertions(+), 26 deletions(-)
> 
> diff --git a/dhcpv6.c b/dhcpv6.c
> index 14a5c7e..f2e7307 100644
> --- a/dhcpv6.c
> +++ b/dhcpv6.c
> @@ -296,47 +296,42 @@ static struct opt_hdr *dhcpv6_opt(const struct pool *p, size_t *offset,
>  static struct opt_hdr *dhcpv6_ia_notonlink(const struct pool *p,
>  					   struct in6_addr *la)
>  {
> +	int ia_types[2] = { OPT_IA_NA, OPT_IA_TA }, *ia_type;
> +	const struct opt_ia_addr *opt_addr;
>  	char buf[INET6_ADDRSTRLEN];
>  	struct in6_addr req_addr;
>  	const struct opt_hdr *h;
>  	struct opt_hdr *ia;
>  	size_t offset;
> -	int ia_type;
>  
> -	ia_type = OPT_IA_NA;
> -ia_ta:
> -	offset = 0;
> -	while ((ia = dhcpv6_opt(p, &offset, ia_type))) {
> -		if (ntohs(ia->l) < OPT_VSIZE(ia_na))
> -			return NULL;
> +	foreach(ia_type, ia_types) {
> +		offset = 0;
> +		while ((ia = dhcpv6_opt(p, &offset, *ia_type))) {
> +			if (ntohs(ia->l) < OPT_VSIZE(ia_na))
> +				return NULL;
>  
> -		offset += sizeof(struct opt_ia_na);
> +			offset += sizeof(struct opt_ia_na);
>  
> -		while ((h = dhcpv6_opt(p, &offset, OPT_IAAADR))) {
> -			const struct opt_ia_addr *opt_addr;
> +			while ((h = dhcpv6_opt(p, &offset, OPT_IAAADR))) {
> +				if (ntohs(h->l) != OPT_VSIZE(ia_addr))
> +					return NULL;
>  
> -			if (ntohs(h->l) != OPT_VSIZE(ia_addr))
> -				return NULL;
> +				opt_addr = (const struct opt_ia_addr *)h;
> +				req_addr = opt_addr->addr;
> +				if (!IN6_ARE_ADDR_EQUAL(la, &req_addr))
> +					goto err;
>  
> -			opt_addr = (const struct opt_ia_addr *)h;
> -			req_addr = opt_addr->addr;
> -			if (!IN6_ARE_ADDR_EQUAL(la, &req_addr)) {
> -				info("DHCPv6: requested address %s not on link",
> -				     inet_ntop(AF_INET6, &req_addr,
> -					       buf, sizeof(buf)));
> -				return ia;
> +				offset += sizeof(struct opt_ia_addr);
>  			}
> -
> -			offset += sizeof(struct opt_ia_addr);
>  		}
>  	}
>  
> -	if (ia_type == OPT_IA_NA) {
> -		ia_type = OPT_IA_TA;
> -		goto ia_ta;
> -	}
> -
>  	return NULL;
> +
> +err:
> +	info("DHCPv6: requested address %s not on link",
> +	     inet_ntop(AF_INET6, &req_addr, buf, sizeof(buf)));
> +	return ia;
>  }
>  
>  /**
> diff --git a/util.h b/util.h
> index 0bf396a..582ef57 100644
> --- a/util.h
> +++ b/util.h
> @@ -102,6 +102,9 @@
>  
>  #define ARRAY_SIZE(a)		((int)(sizeof(a) / sizeof((a)[0])))
>  
> +#define foreach(item, array)						\
> +	for ((item) = (array); (item) - (array) < ARRAY_SIZE(array); (item)++)
> +
>  #define IN_INTERVAL(a, b, x)	((x) >= (a) && (x) <= (b))
>  #define FD_PROTO(x, proto)						\
>  	(IN_INTERVAL(c->proto.fd_min, c->proto.fd_max, (x)))

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]