Re: [PATCH 2/5] vhost-user: introduce virtio API

[Laurent Vivier <lvivier@redhat.com>]

On 24/06/2024 04:56, David Gibson wrote:
> On Fri, Jun 21, 2024 at 04:56:37PM +0200, Laurent Vivier wrote:
>> Add virtio.c and virtio.h that define the functions needed
>> to manage virtqueues.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>   Makefile |   4 +-
>>   util.h   |  11 ++
>>   virtio.c | 446 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   virtio.h | 123 +++++++++++++++
>>   4 files changed, 582 insertions(+), 2 deletions(-)
>>   create mode 100644 virtio.c
>>   create mode 100644 virtio.h
>>
...
>> diff --git a/virtio.c b/virtio.c
>> new file mode 100644
>> index 000000000000..50ec8b5119ed
>> --- /dev/null
>> +++ b/virtio.c
>> @@ -0,0 +1,446 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +
> 
> Needs an actual "Copyright" invocation as well as the SPDX stuff.
> Which, yes, is a bit fiddly given that it's largely taken from qemu.

I'm updating that

> 
>> +/* some parts copied from QEMU subprojects/libvhost-user/libvhost-user.c */
> 
> So, there are obvious stylistic differences between this and the rest
> of the passt code for that reason.  As I think I said on an earlier
> draft, I think we need to go fully one way or the other: either a)
> rewrite this entirely in passt style or b) change the whole thing so
> little that it's trivial to pull in new versions from qemu.  This
> seems to be somewhere in the middle.

As the orignal code in QEMU don't change a lot I chose to update it to match passt coding 
style. I'm adding the function headers. What else is missing?

...
>> diff --git a/virtio.h b/virtio.h
>> new file mode 100644
>> index 000000000000..61398bb432bc
>> --- /dev/null
>> +++ b/virtio.h
>> @@ -0,0 +1,123 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +//
>> +/* come parts copied from QEMU subprojects/libvhost-user/libvhost-user.h */
>> +
>> +#ifndef VIRTIO_H
>> +#define VIRTIO_H
>> +
>> +#include <stdbool.h>
>> +#include <linux/vhost_types.h>
>> +
>> +#define VIRTQUEUE_MAX_SIZE 1024
>> +
>> +#define vu_panic(vdev, ...)		\
>> +	do {				\
>> +		(vdev)->broken = true;	\
>> +		err( __VA_ARGS__ );	\
> 
> Wouldn't it be simpler to just use die() in place of vu_panic().  This
> is trying to keep the program running even if the vu device is broken,
> but if our channel to the guest is broken, I don't think passt is
> really worth saving.
> 

I agree.

Thanks,
Laurent