Re: [PATCH v5 07/17] fanotify: encode file identifier for FAN_REPORT_FID

Re: [PATCH v5 07/17] fanotify: encode file identifier for FAN_REPORT_FID

[Amir Goldstein]

On Fri, Jan 11, 2019 at 10:11 AM kbuild test robot <lkp@intel.com> wrote:
>
> Hi Amir,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on linus/master]
> [also build test WARNING on v5.0-rc1]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Amir-Goldstein/fanotify-add-support-for-more-event-types/20190111-090241
> config: mips-allmodconfig (attached as .config)
> compiler: mips-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         GCC_VERSION=7.2.0 make.cross ARCH=mips
>
> All warnings (new ones prefixed by >>):
>
>    In file included from include/linux/kernel.h:14:0,
>                     from include/linux/list.h:9,
>                     from include/linux/preempt.h:11,
>                     from include/linux/spinlock.h:51,
>                     from include/linux/fdtable.h:11,
>                     from fs/notify/fanotify/fanotify.c:3:
>    fs/notify/fanotify/fanotify.c: In function 'fanotify_encode_fid':
>    include/linux/kern_levels.h:5:18: warning: format '%x' expects argument of type 'unsigned int', but argument 2 has type 'long int' [-Wformat=]

I'm confused.
__kernel_fsid_t val member is long[] on mips arch and int[] on other archs.
Which format specifier am I supposed to use to print it?

Thanks,
Amir.

Re: [PATCH v5 07/17] fanotify: encode file identifier for FAN_REPORT_FID

[Paul Burton]

Hi Amir,

On Fri, Jan 11, 2019 at 10:37:39AM +0200, Amir Goldstein wrote:
> On Fri, Jan 11, 2019 at 10:11 AM kbuild test robot <lkp@intel.com> wrote:
> >
> > Hi Amir,
> >
> > I love your patch! Perhaps something to improve:
> >
> > [auto build test WARNING on linus/master]
> > [also build test WARNING on v5.0-rc1]
> > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> >
> > url:    https://github.com/0day-ci/linux/commits/Amir-Goldstein/fanotify-add-support-for-more-event-types/20190111-090241
> > config: mips-allmodconfig (attached as .config)
> > compiler: mips-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
> > reproduce:
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # save the attached .config to linux build tree
> >         GCC_VERSION=7.2.0 make.cross ARCH=mips
> >
> > All warnings (new ones prefixed by >>):
> >
> >    In file included from include/linux/kernel.h:14:0,
> >                     from include/linux/list.h:9,
> >                     from include/linux/preempt.h:11,
> >                     from include/linux/spinlock.h:51,
> >                     from include/linux/fdtable.h:11,
> >                     from fs/notify/fanotify/fanotify.c:3:
> >    fs/notify/fanotify/fanotify.c: In function 'fanotify_encode_fid':
> >    include/linux/kern_levels.h:5:18: warning: format '%x' expects argument of type 'unsigned int', but argument 2 has type 'long int' [-Wformat=]
> 
> I'm confused.
> __kernel_fsid_t val member is long[] on mips arch and int[] on other archs.
> Which format specifier am I supposed to use to print it?

That's a good question.

Here's another: why on Earth do we have this custom __kernel_fsid_t
definition for MIPS at all..?

We only provide the MIPS definition when _MIPS_SZLONG == 32, ie. when
long is the same size as int & the generic definition of the struct
would have an identical memory layout anyway.

So I'm tempted to just delete this weird code - the only thing that
might break is if someone is doing something that really expects a long
& cares about getting an int of the same size. For example if anyone
prints the value with %lx or the like - essentially the opposite case to
yours.

I consider it pretty unlikely that anyone will be doing this in a
MIPS32-specific codepath such that they've been seeing the custom
__kernel_fsid_t up until now, but does anyone see a problem with this?

Thanks,
    Paul

---
diff --git a/arch/mips/include/uapi/asm/posix_types.h b/arch/mips/include/uapi/asm/posix_types.h
index 6aa49c10f88f..f0ccb5b90ce9 100644
--- a/arch/mips/include/uapi/asm/posix_types.h
+++ b/arch/mips/include/uapi/asm/posix_types.h
@@ -21,13 +21,6 @@
 typedef long		__kernel_daddr_t;
 #define __kernel_daddr_t __kernel_daddr_t

-#if (_MIPS_SZLONG == 32)
-typedef struct {
-	long	val[2];
-} __kernel_fsid_t;
-#define __kernel_fsid_t __kernel_fsid_t
-#endif
-
 #include <asm-generic/posix_types.h>

 #endif /* _ASM_POSIX_TYPES_H */