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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3077 bytes --]

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@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]