[PATCH iproute2 0/5] Warning fixes

[PATCH iproute2 0/5] Warning fixes

[Stephen Hemminger]

Compiling iproute2 with -Wextra finds some possible confusing
things. Easy enough to fix.

Stephen Hemminger (5):
  dcb: fully initialize flag table
  fix fallthrough warnings
  ss: fix warning about empty if()
  ct: check for invalid proto
  ifstat: fix warning about conditional

 dcb/dcb_dcbx.c  | 16 ++++++++--------
 ip/xfrm_state.c |  1 +
 lib/utils.c     |  3 +--
 misc/ifstat.c   |  2 +-
 misc/ss.c       |  3 ++-
 tc/m_ct.c       |  4 +++-
 6 files changed, 16 insertions(+), 13 deletions(-)

-- 
2.39.2

[PATCH iproute2 1/5] dcb: fully initialize flag table

[Stephen Hemminger]

And make the flag table const since only used for lookup.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 dcb/dcb_dcbx.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/dcb/dcb_dcbx.c b/dcb/dcb_dcbx.c
index 244b671b893b..9e3dd2a0af87 100644
--- a/dcb/dcb_dcbx.c
+++ b/dcb/dcb_dcbx.c
@@ -42,12 +42,12 @@ struct dcb_dcbx_flag {
 	const char *key_json;
 };
 
-static struct dcb_dcbx_flag dcb_dcbx_flags[] = {
-	{DCB_CAP_DCBX_HOST, "host"},
-	{DCB_CAP_DCBX_LLD_MANAGED, "lld-managed", "lld_managed"},
-	{DCB_CAP_DCBX_VER_CEE, "cee"},
-	{DCB_CAP_DCBX_VER_IEEE, "ieee"},
-	{DCB_CAP_DCBX_STATIC, "static"},
+static const struct dcb_dcbx_flag dcb_dcbx_flags[] = {
+	{DCB_CAP_DCBX_HOST, "host", NULL },
+	{DCB_CAP_DCBX_LLD_MANAGED, "lld-managed", "lld_managed" },
+	{DCB_CAP_DCBX_VER_CEE, "cee", NULL },
+	{DCB_CAP_DCBX_VER_IEEE, "ieee", NULL },
+	{DCB_CAP_DCBX_STATIC, "static", NULL },
 };
 
 static void dcb_dcbx_print(__u8 dcbx)
@@ -60,7 +60,7 @@ static void dcb_dcbx_print(__u8 dcbx)
 
 		bit--;
 		for (i = 0; i < ARRAY_SIZE(dcb_dcbx_flags); i++) {
-			struct dcb_dcbx_flag *flag = &dcb_dcbx_flags[i];
+			const struct dcb_dcbx_flag *flag = &dcb_dcbx_flags[i];
 
 			if (flag->value == 1 << bit) {
 				print_bool(PRINT_JSON, flag->key_json ?: flag->key_fp,
@@ -123,7 +123,7 @@ static int dcb_cmd_dcbx_set(struct dcb *dcb, const char *dev, int argc, char **a
 		}
 
 		for (i = 0; i < ARRAY_SIZE(dcb_dcbx_flags); i++) {
-			struct dcb_dcbx_flag *flag = &dcb_dcbx_flags[i];
+			const struct dcb_dcbx_flag *flag = &dcb_dcbx_flags[i];
 
 			if (matches(*argv, flag->key_fp) == 0) {
 				dcbx |= flag->value;
-- 
2.39.2

Re: [PATCH iproute2 1/5] dcb: fully initialize flag table

[Petr Machata]

Stephen Hemminger <stephen@networkplumber.org> writes:

> And make the flag table const since only used for lookup.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Reviewed-by: Petr Machata <me@pmachata.org>

[PATCH iproute2 2/5] fix fallthrough warnings

[Stephen Hemminger]

In lib/utils.c comment for fallthrough was in wrong place
and one was missing in xfrm_state.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 ip/xfrm_state.c | 1 +
 lib/utils.c     | 3 +--
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
index a7b3d0e14156..9be65b2f8461 100644
--- a/ip/xfrm_state.c
+++ b/ip/xfrm_state.c
@@ -660,6 +660,7 @@ static int xfrm_state_modify(int cmd, unsigned int flags, int argc, char **argv)
 		case XFRM_MODE_BEET:
 			if (req.xsinfo.id.proto == IPPROTO_ESP)
 				break;
+			/* fallthrough */
 		default:
 			fprintf(stderr, "MODE value is invalid with XFRM-PROTO value \"%s\"\n",
 				strxf_xfrmproto(req.xsinfo.id.proto));
diff --git a/lib/utils.c b/lib/utils.c
index 01f3a5f7e4ea..b1f273054906 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -985,9 +985,8 @@ const char *rt_addr_n2a_r(int af, int len,
 			return inet_ntop(AF_INET6, &sa->sin6.sin6_addr,
 					 buf, buflen);
 		}
-
-		/* fallthrough */
 	}
+		/* fallthrough */
 	default:
 		return "???";
 	}
-- 
2.39.2

Re: [PATCH iproute2 2/5] fix fallthrough warnings

[Petr Machata]

Stephen Hemminger <stephen@networkplumber.org> writes:

> In lib/utils.c comment for fallthrough was in wrong place
> and one was missing in xfrm_state.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

That xfrm code is a bit too sprawling to understand what's going on, but
it's hard to imagine what else than an intentional fall-through would
make sense in that context. With that:

Reviewed-by: Petr Machata <me@pmachata.org>

[PATCH iproute2 3/5] ss: fix warning about empty if()

[Stephen Hemminger]

With all warnings enabled gcc wants brackets around the
empty if() clause. "Yes I really want an empty clause"

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 misc/ss.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/misc/ss.c b/misc/ss.c
index de02fccb539b..e9d813596b91 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -627,8 +627,9 @@ static void user_ent_hash_build_task(char *path, int pid, int tid)
 
 			fp = fopen(stat, "r");
 			if (fp) {
-				if (fscanf(fp, "%*d (%[^)])", task) < 1)
+				if (fscanf(fp, "%*d (%[^)])", task) < 1) {
 					; /* ignore */
+				}
 				fclose(fp);
 			}
 		}
-- 
2.39.2

Re: [PATCH iproute2 3/5] ss: fix warning about empty if()

[Petr Machata]

Stephen Hemminger <stephen@networkplumber.org> writes:

> With all warnings enabled gcc wants brackets around the
> empty if() clause. "Yes I really want an empty clause"
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  misc/ss.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/misc/ss.c b/misc/ss.c
> index de02fccb539b..e9d813596b91 100644
> --- a/misc/ss.c
> +++ b/misc/ss.c
> @@ -627,8 +627,9 @@ static void user_ent_hash_build_task(char *path, int pid, int tid)
>  
>  			fp = fopen(stat, "r");
>  			if (fp) {
> -				if (fscanf(fp, "%*d (%[^)])", task) < 1)
> +				if (fscanf(fp, "%*d (%[^)])", task) < 1) {
>  					; /* ignore */
> +				}
>  				fclose(fp);
>  			}
>  		}

Reviewed-by: Petr Machata <me@pmachata.org>

As an aside, this whole if business is necessary in the first place due
to __attribute__((warn_unused_result)) that fscanf apparently has on
some libc's. But ignoring fscanf failures is safe, because a) `task' is
pre-initialized at variable definition, so ignoring the result like this
should indeed be safe; and b), fp references some file in /proc and we
can rely on the format and that the scan will in fact not fail.

[PATCH iproute2 4/5] ct: check for invalid proto

[Stephen Hemminger]

Previously since proto was __u8 an invalid proto would
be allowed.  Gcc warns about never true conditional
since __u8 can never be negative.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 tc/m_ct.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tc/m_ct.c b/tc/m_ct.c
index 3e2491b3d192..8c471489778a 100644
--- a/tc/m_ct.c
+++ b/tc/m_ct.c
@@ -161,7 +161,8 @@ static int ct_parse_mark(char *str, struct nlmsghdr *n)
 static int ct_parse_helper(char *str, struct nlmsghdr *n)
 {
 	char f[32], p[32], name[32];
-	__u8 family, proto;
+	__u8 family;
+	int proto;
 
 	if (strlen(str) >= 32 ||
 	    sscanf(str, "%[^-]-%[^-]-%[^-]", f, p, name) != 3)
@@ -172,6 +173,7 @@ static int ct_parse_helper(char *str, struct nlmsghdr *n)
 		family = AF_INET6;
 	else
 		return -1;
+
 	proto = inet_proto_a2n(p);
 	if (proto < 0)
 		return -1;
-- 
2.39.2

Re: [PATCH iproute2 4/5] ct: check for invalid proto

[Petr Machata]

Stephen Hemminger <stephen@networkplumber.org> writes:

> Previously since proto was __u8 an invalid proto would
> be allowed.  Gcc warns about never true conditional
> since __u8 can never be negative.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Reviewed-by: Petr Machata <me@pmachata.org>

[PATCH iproute2 5/5] ifstat: fix warning about conditional

[Stephen Hemminger]

Gcc with warnings enabled complains because the conditional.
  if ((long)(a - b) < 0)
could be construed as never true.  Change to simple comparison.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 misc/ifstat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/misc/ifstat.c b/misc/ifstat.c
index 4ce5ca8af4e7..f6f9ba5027d3 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -608,7 +608,7 @@ static void update_db(int interval)
 				int i;
 
 				for (i = 0; i < MAXS; i++) {
-					if ((long)(h1->ival[i] - n->ival[i]) < 0) {
+					if (h1->ival[i] < n->ival[i]) {
 						memset(n->ival, 0, sizeof(n->ival));
 						break;
 					}
-- 
2.39.2

Re: [PATCH iproute2 5/5] ifstat: fix warning about conditional

[Petr Machata]

Stephen Hemminger <stephen@networkplumber.org> writes:

> Gcc with warnings enabled complains because the conditional.
>   if ((long)(a - b) < 0)
> could be construed as never true.

Hmm, yeah, it would never be true where sizeof(long) > 4.

>                                    Change to simple comparison.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Reviewed-by: Petr Machata <me@pmachata.org>

Re: [PATCH iproute2 0/5] Warning fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to iproute2/iproute2.git (main)
by Stephen Hemminger <stephen@networkplumber.org>:

On Wed, 28 Jun 2023 16:38:08 -0700 you wrote:
> Compiling iproute2 with -Wextra finds some possible confusing
> things. Easy enough to fix.
> 
> Stephen Hemminger (5):
>   dcb: fully initialize flag table
>   fix fallthrough warnings
>   ss: fix warning about empty if()
>   ct: check for invalid proto
>   ifstat: fix warning about conditional
> 
> [...]

Here is the summary with links:
  - [iproute2,1/5] dcb: fully initialize flag table
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=9ef3210bc7df
  - [iproute2,2/5] fix fallthrough warnings
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=2b41725d2a46
  - [iproute2,3/5] ss: fix warning about empty if()
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=27dce6de940a
  - [iproute2,4/5] ct: check for invalid proto
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=ac6d95a8465e
  - [iproute2,5/5] ifstat: fix warning about conditional
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=bbfccc11e1c4

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html