[PATCH] iproute2: fix type incompatibility in ifstat.c

[PATCH] iproute2: fix type incompatibility in ifstat.c

[Stephen Gallagher]

Throughout ifstat.c, ifstat_ent.val is accessed as a long long unsigned
type, however it is defined as __u64. This works by coincidence on many
systems, however on ppc64le, __u64 is a long unsigned.

This patch makes the type definition consistent with all of the places
where it is accessed.

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>
---
 misc/ifstat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/misc/ifstat.c b/misc/ifstat.c
index 721f4914..767cedd4 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -58,7 +58,7 @@ struct ifstat_ent {
 	struct ifstat_ent	*next;
 	char			*name;
 	int			ifindex;
-	__u64			val[MAXS];
+	unsigned long long	val[MAXS];
 	double			rate[MAXS];
 	__u32			ival[MAXS];
 };
-- 
2.43.0

Re: [PATCH] iproute2: fix type incompatibility in ifstat.c

[Andrea Claudi]

On Tue, Feb 06, 2024 at 09:22:06AM -0500, Stephen Gallagher wrote:
> Throughout ifstat.c, ifstat_ent.val is accessed as a long long unsigned
> type, however it is defined as __u64. This works by coincidence on many
> systems, however on ppc64le, __u64 is a long unsigned.
> 
> This patch makes the type definition consistent with all of the places
> where it is accessed.
> 
> Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>
> ---
>  misc/ifstat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/misc/ifstat.c b/misc/ifstat.c
> index 721f4914..767cedd4 100644
> --- a/misc/ifstat.c
> +++ b/misc/ifstat.c
> @@ -58,7 +58,7 @@ struct ifstat_ent {
>  	struct ifstat_ent	*next;
>  	char			*name;
>  	int			ifindex;
> -	__u64			val[MAXS];
> +	unsigned long long	val[MAXS];
>  	double			rate[MAXS];
>  	__u32			ival[MAXS];
>  };
> -- 
> 2.43.0
> 

Hi Stephen, thanks for taking care of this.

FYI, patch directed to iproute2 or iproute2-next tree should:
- preferrably have [PATCH iproute2] in their subject
- be directed or cc'd to iproute2 maintainers Stephen Hemminger and
  David Ahern, and to the author of the fixed commit if possible.

This should include a Fixes: line on the commit changing val to __u64:
Fixes: 5a52102b7c8f ("ifstat: Add extended statistics to ifstat")
Stephen, David: do Stephen needs to resend this?

That said, patch looks good to me, so feel free to add my reviewed-by to
the following versions of this patch.

Reviewed-by: Andrea Claudi <aclaudi@redhat.com>

[PATCH iproute] Fix type incompatibility in ifstat.c

[Stephen Gallagher]

Fixes: 5a52102b7c8f ("ifstat: Add extended statistics to ifstat")

[PATCH] iproute2: fix type incompatibility in ifstat.c

[Stephen Gallagher]

Throughout ifstat.c, ifstat_ent.val is accessed as a long long unsigned
type, however it is defined as __u64. This works by coincidence on many
systems, however on ppc64le, __u64 is a long unsigned.

This patch makes the type definition consistent with all of the places
where it is accessed.

Fixes: 5a52102b7c8f ("ifstat: Add extended statistics to ifstat")

Reviewed-by: Andrea Claudi <aclaudi@redhat.com>

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>
---
 misc/ifstat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/misc/ifstat.c b/misc/ifstat.c
index 721f4914..767cedd4 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -58,7 +58,7 @@ struct ifstat_ent {
 	struct ifstat_ent	*next;
 	char			*name;
 	int			ifindex;
-	__u64			val[MAXS];
+	unsigned long long	val[MAXS];
 	double			rate[MAXS];
 	__u32			ival[MAXS];
 };
-- 
2.43.0

Re: [PATCH] iproute2: fix type incompatibility in ifstat.c

[Stephen Hemminger]

On Tue,  6 Feb 2024 09:22:06 -0500
Stephen Gallagher <sgallagh@redhat.com> wrote:

> Throughout ifstat.c, ifstat_ent.val is accessed as a long long unsigned
> type, however it is defined as __u64. This works by coincidence on many
> systems, however on ppc64le, __u64 is a long unsigned.
> 
> This patch makes the type definition consistent with all of the places
> where it is accessed.
> 
> Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>
> ---

Why not fix the use of unsigned long long to be __u64 instead?
That would make more sense.

Looking at ifstat it has other problems.
It doesn't check the sizes in the netlink response.

It really should be using 64 bit stat counters, not the legacy 32 bit
values. (ie IFLA_STATS64). Anyone want to take this on?X

Also, the cpu_hits offload code is a wart. If it is of interest, it should
be more than one stat.

One last issue, is that change the size of this structure will break old
history files.

Re: [PATCH] iproute2: fix type incompatibility in ifstat.c

[Florian Weimer]

* Stephen Hemminger:

> On Tue,  6 Feb 2024 09:22:06 -0500
> Stephen Gallagher <sgallagh@redhat.com> wrote:
>
>> Throughout ifstat.c, ifstat_ent.val is accessed as a long long unsigned
>> type, however it is defined as __u64. This works by coincidence on many
>> systems, however on ppc64le, __u64 is a long unsigned.
>> 
>> This patch makes the type definition consistent with all of the places
>> where it is accessed.
>> 
>> Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>
>> ---

Patch was:

diff --git a/misc/ifstat.c b/misc/ifstat.c
index 721f4914..767cedd4 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -58,7 +58,7 @@ struct ifstat_ent {
 	struct ifstat_ent	*next;
 	char			*name;
 	int			ifindex;
-	__u64			val[MAXS];
+	unsigned long long	val[MAXS];
 	double			rate[MAXS];
 	__u32			ival[MAXS];
 };

> Why not fix the use of unsigned long long to be __u64 instead?
> That would make more sense.

You still won't be able to use %llu to print it.  I don't think the UAPI
headers provide anything like the <stdint.h> macros because the
assumption is that %llu is okay for printing __u64 on all architectures.

But we have this in POWER:

/*
 * This is here because we used to use l64 for 64bit powerpc
 * and we don't want to impact user mode with our change to ll64
 * in the kernel.
 *
 * However, some user programs are fine with this.  They can
 * flag __SANE_USERSPACE_TYPES__ to get int-ll64.h here.
 */
#if !defined(__SANE_USERSPACE_TYPES__) && defined(__powerpc64__) && !defined(__KERNEL__)
# include <asm-generic/int-l64.h>
#else
# include <asm-generic/int-ll64.h>
#endif

I didn't know some architectures are that … different.  Sadly this
wasn't fixed as part of the transition to powerpc64le.

I suppose iproute2 should build with -D__SANE_USERSPACE_TYPES__.

Thanks,
Florian

Re: [PATCH] iproute2: fix type incompatibility in ifstat.c

[Stephen Gallagher]

On Tue, Feb 6, 2024 at 10:12 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Tue,  6 Feb 2024 09:22:06 -0500
> Stephen Gallagher <sgallagh@redhat.com> wrote:
>
> > Throughout ifstat.c, ifstat_ent.val is accessed as a long long unsigned
> > type, however it is defined as __u64. This works by coincidence on many
> > systems, however on ppc64le, __u64 is a long unsigned.
> >
> > This patch makes the type definition consistent with all of the places
> > where it is accessed.
> >
> > Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>
> > ---
>
> Why not fix the use of unsigned long long to be __u64 instead?
> That would make more sense.
>


FWIW, that's the first path I tried, but it's accessed in dozens of
places, every one of which treats it as a 'long long unsigned'. That
led me to the belief that the specified type was just wrong (or at
minimum, in contravention of the consumers' expectations).

> Looking at ifstat it has other problems.
> It doesn't check the sizes in the netlink response.
>
> It really should be using 64 bit stat counters, not the legacy 32 bit
> values. (ie IFLA_STATS64). Anyone want to take this on?X

I definitely don't have the subject-matter expertise to do this,
sorry. Given that this issue is blocking our builds in Fedora/RHEL, I
was hoping we could try to solve the acute problem and leave
wider-ranging fixes to a separate effort.

> Also, the cpu_hits offload code is a wart. If it is of interest, it should
> be more than one stat.
>
> One last issue, is that change the size of this structure will break old
> history files.
>

I'm not sure I understand this; it looks to me like this is a private
structure. If it has API impact (as in, it's written directly to
output data meant to be re-read), shouldn't it be in a public header
and/or otherwise denoted as API?

Re: [PATCH] iproute2: fix type incompatibility in ifstat.c

[patchwork-bot+netdevbpf]

Hello:

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

On Tue,  6 Feb 2024 11:52:34 -0500 you wrote:
> Throughout ifstat.c, ifstat_ent.val is accessed as a long long unsigned
> type, however it is defined as __u64. This works by coincidence on many
> systems, however on ppc64le, __u64 is a long unsigned.
> 
> This patch makes the type definition consistent with all of the places
> where it is accessed.
> 
> [...]

Here is the summary with links:
  - iproute2: fix type incompatibility in ifstat.c
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=d9b886d745ad

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