Re: [PATCH] xfstests: replace custom __u64 definition with uint64_t

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Dec 26, 2023 at 02:14:38PM +0530, Disha Goel wrote:
> On 21/12/23 10:42 pm, Darrick J. Wong wrote:
> 
> > On Thu, Dec 21, 2023 at 11:42:31AM +0530, Disha Goel wrote:
> > > In some distributions, __u64 is already defined in system header files,
> > > causing compilation errors when building xfstest.
> > > 
> > > 	# make
> > > 	    [CC]    ext4_resize
> > > 	ext4_resize.c:17:28: error: conflicting types for '__u64'
> > > 	 typedef unsigned long long __u64;
> > > 	                            ^~~~~
> > > 	In file included from /usr/include/asm/types.h:26:0,
> > > 	                 from /usr/include/linux/types.h:5,
> > > 	                 from /usr/include/linux/mount.h:4,
> > > 	                 from /usr/include/sys/mount.h:32,
> > > 	                 from ext4_resize.c:15:
> > > 	/usr/include/asm-generic/int-l64.h:30:23: note: previous declaration of '__u64' was here
> > > 	 typedef unsigned long __u64;
> > >                         ^~~~~
> > > 
> > > To address this issue, replace the custom definition of __u64 with the
> > > standard uint64_t type from <stdint.h>. uint64_t is part of the C99
> > > standard, offering a standardised approach for representing unsigned
> > > 64-bit integers. This modification enhances code consistency and
> > > ensures compatibility with standard types.
> > ...but isn't consistent with the definition in the kernel uapi headers.
> > ioctls explicitly use __u64, not uint64_t, and there are a few arches
> > (apparently) where __u64 ends up an unsigned long long but uint64_t ends
> > up an unsigned long, and the reverse.
> 
> Thanks for reviewing the patch. I will take care of this and address in v2.
> 
> > > Tested on various distributions on Power architecture, by successfully
> > > compiling xfstest. Additionally, verified the compatibility by running
> > > ext4/033 and ext4/056 tests, both of which use ext4_resize and
> > > observed successful test execution.
> > > 
> > >          # make
> > >              [CC]    detached_mounts_propagation
> > >              [CC]    ext4_resize
> > >              [CC]    t_readdir_3
> > > 
> > > Signed-off-by: Disha Goel <disgoel@linux.ibm.com>
> > > ---
> > >   src/ext4_resize.c | 6 ++----
> > >   1 file changed, 2 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/src/ext4_resize.c b/src/ext4_resize.c
> > > index 78b66432..6e60ee5b 100644
> > > --- a/src/ext4_resize.c
> > > +++ b/src/ext4_resize.c
> > > @@ -14,10 +14,8 @@
> > >   #include <sys/ioctl.h>
> > >   #include <sys/mount.h>
> > Why not #include <linux/ext4.h>, which gets you both the __u64 typedef
> > and EXT4_IOC_RESIZE_FS?
> > 
> > --D
> > 
> The #include <linux/ext4.h> got added in kernel 6.4 and distros which
> I'm testing does not have <linux/ext4.h> present in kernel headers.
> We can add #include <linux/types.h> which also gets the __u64 typedef.
> 
> Let me know your thoughts.

Ah, linux/ext4.h didn't exist until 6.4.  Heh.  Well, in the long run
fstests really ought to pull in kernel ioctl definitions whenever
possible.  So:

I suppose this program should #include <linux/types.h> to get the __u64
definition, after which this can go away:

> > > -typedef unsigned long long __u64;

And then you'd need to add some m4/autoconf AC_CHECK_HEADERS magic for
configure to detect when #include <linux/ext4.h> actually results in a
compilable program; and set some make variable accordingly.

Then this program can grow a:

#ifdef HAVE_LINUX_EXT4_H
# include <linux/ext4.h>
#endif

After which this existing bit of cpp magic:

#ifndef EXT4_IOC_RESIZE_FS
# define EXT4_IOC_RESIZE_FS			_IOW('f', 16, __u64)
#endif

should work for everyone.

--D

> > > -
> > >   #ifndef EXT4_IOC_RESIZE_FS
> > > -#define EXT4_IOC_RESIZE_FS		_IOW('f', 16, __u64)
> > > +#define EXT4_IOC_RESIZE_FS		_IOW('f', 16, uint64_t)
> > >   #endif
> > >   #define pr_error(fmt, ...) do { \
> > > @@ -31,7 +29,7 @@ static void usage(void)
> > >   int main(int argc, char **argv)
> > >   {
> > > -	__u64	new_size;
> > > +	uint64_t	new_size;
> > >   	int	error, fd;
> > >   	char	*mnt_dir = NULL, *tmp = NULL;
> > > -- 
> > > 2.39.1
> > > 
> > > 
>