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

On Fri, Jul 05, 2024 at 05:06:12PM +0200, Laurent Vivier wrote:
...
  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(a)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 
Ok.

...
 
 
  +/* 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? 
From memory:
  - No braces for one line blocks
  - snake_case instead of CamelCase
  - Don't use typedefs for structures or unions
  - Tabs instead of spaces

...
  ...
 
  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
  
-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson