Re: [PATCH for-9.1 v3 08/11] contrib/vhost-user-blk: enable it on any POSIX system

[Stefano Garzarella]

On Thu, Apr 04, 2024 at 04:00:38PM +0200, Philippe Mathieu-Daudé wrote:
> Hi Stefano,

Hi Phil!

> On 4/4/24 14:23, Stefano Garzarella wrote:
> > Let's make the code more portable by using the "qemu/bswap.h" API
> > and adding defines from block/file-posix.c to support O_DIRECT in
> > other systems (e.g. macOS).
> >
> > vhost-user-server.c is a dependency, let's enable it for any POSIX
> > system.
> >
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > ---
> >  meson.build                             |  2 --
> >  contrib/vhost-user-blk/vhost-user-blk.c | 19 +++++++++++++++++--
> >  util/meson.build                        |  4 +++-
> >  3 files changed, 20 insertions(+), 5 deletions(-)
>
>
> > diff --git a/contrib/vhost-user-blk/vhost-user-blk.c 
> > b/contrib/vhost-user-blk/vhost-user-blk.c
> > index a8ab9269a2..462e584857 100644
> > --- a/contrib/vhost-user-blk/vhost-user-blk.c
> > +++ b/contrib/vhost-user-blk/vhost-user-blk.c
> > @@ -16,6 +16,7 @@
> >   */
> >  #include "qemu/osdep.h"
> > +#include "qemu/bswap.h"
> >  #include "standard-headers/linux/virtio_blk.h"
> >  #include "libvhost-user-glib.h"
>
>
> > @@ -267,13 +282,13 @@ static int vub_virtio_process_req(VubDev *vdev_blk,
> >      req->in = (struct virtio_blk_inhdr *)elem->in_sg[in_num - 1].iov_base;
> >      in_num--;
> > -    type = le32toh(req->out->type);
> > +    type = le32_to_cpu(req->out->type);
> >      switch (type & ~VIRTIO_BLK_T_BARRIER) {
> >      case VIRTIO_BLK_T_IN:
> >      case VIRTIO_BLK_T_OUT: {
> >          ssize_t ret = 0;
> >          bool is_write = type & VIRTIO_BLK_T_OUT;
> > -        req->sector_num = le64toh(req->out->sector);
> > +        req->sector_num = le64_to_cpu(req->out->sector);
> >          if (is_write) {
> >              ret  = vub_writev(req, &elem->out_sg[1], out_num);
> >          } else {
> Can we switch to the bswap API in a preliminary patch,

Sure, I tried to minimize the patches because it's already big,
but I can split this.

> converting all the source files?

What do you mean with "all the source files"?

"le64toh" is used here and in some subprojects (e.g. libvduse,
libvhost-user), where IIUC we can't use QEMU's bswap.h because we
don't want to put a dependency with the QEMU code.

BTW I'll check for other *toh() usage in QEMU code and change in the
preliminary patch you suggested to add.

Thanks for the review,
Stefano