Re: [PATCH v2 2/2] tools/virtio: fix build caused by virtio_ring changes

[Shunsuke Mie]

On 2023/04/11 3:40, Greg Kroah-Hartman wrote:
> On Mon, Apr 10, 2023 at 08:00:33AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Apr 10, 2023 at 08:28:45PM +0900, Shunsuke Mie wrote:
> > > Fix the build dependency for virtio_test. The virtio_ring that is used from
> > > the test requires container_of_const(). Change to use container_of.h kernel
> > > header directly and adapt related codes.
> > >
> > > Signed-off-by: Shunsuke Mie <mie@xxxxxxxxxx>
> > This is only for next right? That's where container_of_const
> > things are I think ...
> container_of_const() is in 6.2.
>
>
> > > ---
> > >   tools/include/linux/types.h   |  1 -
> > >   tools/virtio/linux/compiler.h |  2 ++
> > >   tools/virtio/linux/kernel.h   |  5 +----
> > >   tools/virtio/linux/module.h   |  1 -
> > >   tools/virtio/linux/uaccess.h  | 11 ++---------
> > >   5 files changed, 5 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/tools/include/linux/types.h b/tools/include/linux/types.h
> > > index 051fdeaf2670..f1896b70a8e5 100644
> > > --- a/tools/include/linux/types.h
> > > +++ b/tools/include/linux/types.h
> > > @@ -49,7 +49,6 @@ typedef __s8  s8;
> > >   #endif
> > >   
> > >   #define __force
> > > -#define __user
> Why is this needed?
>
> > >   #define __must_check
> > >   #define __cold
> > >   
> > > diff --git a/tools/virtio/linux/compiler.h b/tools/virtio/linux/compiler.h
> > > index 2c51bccb97bb..1f3a15b954b9 100644
> > > --- a/tools/virtio/linux/compiler.h
> > > +++ b/tools/virtio/linux/compiler.h
> > > @@ -2,6 +2,8 @@
> > >   #ifndef LINUX_COMPILER_H
> > >   #define LINUX_COMPILER_H
> > >   
> > > +#include "../../../include/linux/compiler_types.h"
> While I understand your need to not want to duplicate code, what in the
> world is this doing?  Why not use the in-kernel compiler.h instead?  Why
> are you copying loads of .h files into tools/virtio/?  What is this for
> and why not just use the real files so you don't have to even attempt to
> try to keep things in sync (hint, they will always be out of sync.)
>
> > > +
> > >   #define WRITE_ONCE(var, val) \
> > >   	(*((volatile typeof(val) *)(&(var))) = (val))
> > >   
> > > diff --git a/tools/virtio/linux/kernel.h b/tools/virtio/linux/kernel.h
> > > index 8b877167933d..6702008f7f5c 100644
> > > --- a/tools/virtio/linux/kernel.h
> > > +++ b/tools/virtio/linux/kernel.h
> > > @@ -10,6 +10,7 @@
> > >   #include <stdarg.h>
> > >   
> > >   #include <linux/compiler.h>
> > > +#include "../../../include/linux/container_of.h"
> Either do this for all .h files, or not, don't pick and choose.
>
>
>
> > >   #include <linux/log2.h>
> > >   #include <linux/types.h>
> > >   #include <linux/overflow.h>
> > > @@ -107,10 +108,6 @@ static inline void free_page(unsigned long addr)
> > >   	free((void *)addr);
> > >   }
> > >   
> > > -#define container_of(ptr, type, member) ({			\
> > > -	const typeof( ((type *)0)->member ) *__mptr = (ptr);	\
> > > -	(type *)( (char *)__mptr - offsetof(type,member) );})
> > > -
> > >   # ifndef likely
> > >   #  define likely(x)	(__builtin_expect(!!(x), 1))
> > >   # endif
> > > diff --git a/tools/virtio/linux/module.h b/tools/virtio/linux/module.h
> > > index 9dfa96fea2b2..5cf39167d47a 100644
> > > --- a/tools/virtio/linux/module.h
> > > +++ b/tools/virtio/linux/module.h
> > > @@ -4,4 +4,3 @@
> > >   #define MODULE_LICENSE(__MODULE_LICENSE_value) \
> > >   	static __attribute__((unused)) const char *__MODULE_LICENSE_name = \
> > >   		__MODULE_LICENSE_value
> > > -
> This change has nothing to do with what you said was happening in this
> patch :(
>
> Please be more careful.
>
> > > diff --git a/tools/virtio/linux/uaccess.h b/tools/virtio/linux/uaccess.h
> > > index 991dfb263998..cde2c344b260 100644
> > > --- a/tools/virtio/linux/uaccess.h
> > > +++ b/tools/virtio/linux/uaccess.h
> > > @@ -6,15 +6,10 @@
> > >   
> > >   extern void *__user_addr_min, *__user_addr_max;
> > >   
> > > -static inline void __chk_user_ptr(const volatile void *p, size_t size)
> > > -{
> > > -	assert(p >= __user_addr_min && p + size <= __user_addr_max);
> > > -}
> > > -
> What does this function have to do with container_of()?
>
>
> > >   #define put_user(x, ptr)					\
> > >   ({								\
> > >   	typeof(ptr) __pu_ptr = (ptr);				\
> > > -	__chk_user_ptr(__pu_ptr, sizeof(*__pu_ptr));		\
> > > +	__chk_user_ptr(__pu_ptr);		\
> Why are you trying to duplicate in-kernel .h files?
>
> This all doesn't look ok, sorry.
>
> greg k-h

Thank you. I'll modify problems you commented, but we have to discuss 
the design of tools/virtio.

Best regards,

Shunsuke.