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

Shunsuke Mie
2023-04-11T03:25:47Z

On 2023/04/11 4:44, Michael S. Tsirkin wrote:
> On Mon, Apr 10, 2023 at 08:40:34PM +0200, 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@igel.co.jp>
>>> This is only for next right? That's where container_of_const
>>> things are I think ...
>> container_of_const() is in 6.2.
>
> Absolutely but the patch switching virtio to that is not there.
>
>
>>>> ---
>>>> 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.)
> Because it's doing a very weird hack: building some kernel
> code into a userspace binary, used for just for testing.
> This is all not part of kernel build intentionally, no
> one is supposed to use this binary in production, but
> it was helpful in finding bugs in virtio core in the past
> so I keep it around.
Would it be possible to switch to in-kernel tests, such as KUnit? If it's
possible, we could use the kernel headers and implementations as they are,
and we could address the concerns that Greg raised I think.

Best regards,

Shunsuke