| Message ID | 1514242524-4319-2-git-send-email-rosenp@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [LEDE-DEV,1/2] firewall3: Replace strerror(errno) with %m | expand |
Citeren Rosen Penev <rosenp@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@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);
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);
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. Signed-off-by: Rosen Penev <rosenp@gmail.com> --- defaults.c | 2 +- ipsets.c | 2 +- iptables.c | 11 ++++++----- options.c | 2 +- ubus.c | 4 ++-- 5 files changed, 11 insertions(+), 10 deletions(-)