[LEDE-DEV] [PATCH 2/2] firewall3: Fix errors reported by cppcheck.

[Arjen de Korte]

Citeren Rosen Penev <rosenp at gmail.com>:

> Mainly sign conversion errors with printf function (%d vs. %u).  
> Exact command line used was:
>
> cppcheck --enable=all --inconclusive --force . 2> err.txt && cat  
> err.txt | grep -v never | grep -v reduced
>
> Only errors that I felt comfortable with were fixed.

Generally, NACK to this one. Unless you can provide an instance where  
this actually fixes a bug, I have seen these kind of changes break  
things in unexpected ways. For instance, if the output is somehow  
parsed by an external program, comparing (unsigned)-1 and (signed)-1  
against an arbitrary value may result in different outcomes.

     (unsigned)-1 is greater than 100
     (signed)-1 is smaller than 100

Code relying on the above is bug-ugly, but fixing compiler warnings is  
certainly not worth taking any risk here, unless one can demonstrate  
it fixes an actual problem.

> Signed-off-by: Rosen Penev <rosenp at gmail.com>
> ---
>  defaults.c |  2 +-
>  ipsets.c   |  2 +-
>  iptables.c | 11 ++++++-----
>  options.c  |  2 +-
>  ubus.c     |  4 ++--
>  5 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/defaults.c b/defaults.c
> index f1be1dd..41c41a4 100644
> --- a/defaults.c
> +++ b/defaults.c
> @@ -331,7 +331,7 @@ set_default(const char *name, int set)
>  		return;
>  	}
>
> -	fprintf(f, "%u\n", set);
> +	fprintf(f, "%d\n", set);
>  	fclose(f);
>  }
>
> diff --git a/ipsets.c b/ipsets.c
> index 30c6463..9ce89aa 100644
> --- a/ipsets.c
> +++ b/ipsets.c
> @@ -321,7 +321,7 @@ fw3_load_ipsets(struct fw3_state *state, struct  
> uci_package *p,
>  static void
>  create_ipset(struct fw3_ipset *ipset, struct fw3_state *state)
>  {
> -	bool first = true;
> +	bool first;
>
>  	struct fw3_ipset_datatype *type;
>
> diff --git a/iptables.c b/iptables.c
> index d848239..70bfa59 100644
> --- a/iptables.c
> +++ b/iptables.c
> @@ -929,11 +929,12 @@ void
>  fw3_ipt_rule_mac(struct fw3_ipt_rule *r, struct fw3_mac *mac)
>  {
>  	char buf[sizeof("ff:ff:ff:ff:ff:ff\0")];
> -	uint8_t *addr = mac->mac.ether_addr_octet;
> +	uint8_t *addr;
>
>  	if (!mac)
>  		return;
>
> +	addr = mac->mac.ether_addr_octet;
>  	sprintf(buf, "%02x:%02x:%02x:%02x:%02x:%02x",
>  	        addr[0], addr[1], addr[2], addr[3], addr[4], addr[5]);
>
> @@ -981,12 +982,12 @@ fw3_ipt_rule_limit(struct fw3_ipt_rule *r,  
> struct fw3_limit *limit)
>
>  	fw3_ipt_rule_addarg(r, false, "-m", "limit");
>
> -	sprintf(buf, "%u/%s", limit->rate, fw3_limit_units[limit->unit]);
> +	sprintf(buf, "%d/%s", limit->rate, fw3_limit_units[limit->unit]);
>  	fw3_ipt_rule_addarg(r, limit->invert, "--limit", buf);
>
>  	if (limit->burst > 0)
>  	{
> -		sprintf(buf, "%u", limit->burst);
> +		sprintf(buf, "%d", limit->burst);
>  		fw3_ipt_rule_addarg(r, limit->invert, "--limit-burst", buf);
>  	}
>  }
> @@ -1089,7 +1090,7 @@ fw3_ipt_rule_time(struct fw3_ipt_rule *r,  
> struct fw3_time *time)
>  				if (p > buf)
>  					*p++ = ',';
>
> -				p += sprintf(p, "%u", i);
> +				p += sprintf(p, "%d", i);
>  			}
>  		}
>
> @@ -1105,7 +1106,7 @@ fw3_ipt_rule_time(struct fw3_ipt_rule *r,  
> struct fw3_time *time)
>  				if (p > buf)
>  					*p++ = ',';
>
> -				p += sprintf(p, "%u", i);
> +				p += sprintf(p, "%d", i);
>  			}
>  		}
>
> diff --git a/options.c b/options.c
> index f686cf0..8b2a8fd 100644
> --- a/options.c
> +++ b/options.c
> @@ -1145,7 +1145,7 @@ fw3_address_to_string(struct fw3_address  
> *address, bool allow_invert, bool as_ci
>  	}
>  	else
>  	{
> -		p += sprintf(p, "/%u", fw3_netmask2bitlen(address->family,
> +		p += sprintf(p, "/%d", fw3_netmask2bitlen(address->family,
>  		                                          &address->mask.v6));
>  	}
>
> diff --git a/ubus.c b/ubus.c
> index 5bb4f5d..1c823f1 100644
> --- a/ubus.c
> +++ b/ubus.c
> @@ -260,10 +260,10 @@ static void fw3_ubus_rules_add(struct blob_buf  
> *b, const char *service,
>  	}
>
>  	if (instance)
> -		snprintf(comment, sizeof(comment), "ubus:%s[%s] %s %d",
> +		snprintf(comment, sizeof(comment), "ubus:%s[%s] %s %u",
>  				service, instance, type ? type : "rule", n);
>  	else
> -		snprintf(comment, sizeof(comment), "ubus:%s %s %d",
> +		snprintf(comment, sizeof(comment), "ubus:%s %s %u",
>  				service, type ? type : "rule", n);
>
>  	blobmsg_add_string(b, "name", comment);