Re: [PATCH V10 1/3] libxl: Add support for Virtio disk configuration

[Oleksandr <olekstysh@xxxxxxxxx>]

On 15.06.22 17:23, Anthony PERARD wrote:

Hello Anthony

> On Mon, Jun 13, 2022 at 09:05:20PM +0300, Oleksandr Tyshchenko wrote:
> > diff --git a/tools/libs/light/libxl_disk.c b/tools/libs/light/libxl_disk.c
> > index a5ca778..673b0d6 100644
> > --- a/tools/libs/light/libxl_disk.c
> > +++ b/tools/libs/light/libxl_disk.c
> > @@ -575,6 +660,41 @@ cleanup:
> >       return rc;
> >   }
> >   
> > +static int libxl__device_disk_get_path(libxl__gc *gc, uint32_t domid,
> > +                                       char **path)
> > +{
> > +    const char *xen_dir, *virtio_dir;
> > +    char *xen_path, *virtio_path;
> > +    int rc;
> > +
> > +    /* default path */
> > +    xen_path = GCSPRINTF("%s/device/%s",
> > +                         libxl__xs_libxl_path(gc, domid),
> > +                         libxl__device_kind_to_string(LIBXL__DEVICE_KIND_VBD));
> > +
> > +    rc = libxl__xs_read_checked(gc, XBT_NULL, xen_path, &xen_dir);
> > +    if (rc)
> > +        return rc;
> > +
> > +    virtio_path = GCSPRINTF("%s/device/%s",
> > +                            libxl__xs_libxl_path(gc, domid),
> > +                            
> > libxl__device_kind_to_string(LIBXL__DEVICE_KIND_VIRTIO_DISK));
> > +
> > +    rc = libxl__xs_read_checked(gc, XBT_NULL, virtio_path, &virtio_dir);
> > +    if (rc)
> > +        return rc;
> > +
> > +    if (xen_dir && virtio_dir) {
> > +        LOGD(ERROR, domid, "Invalid configuration, both xen and virtio paths are 
> > present");
> > +        return ERROR_INVAL;
> > +    } else if (virtio_dir)
> > +        *path = virtio_path;
> > +    else
> > +        *path = xen_path;
> Small coding style issue, could you use blocks {} on all part of the
> if...else, since you are using them on one of the block? This is
> described in tools/libs/light/CODING_STYLE (5. Block structure).
yes, will do


> > diff --git a/tools/xl/xl_block.c b/tools/xl/xl_block.c
> > index 70eed43..f2b0ff5 100644
> > --- a/tools/xl/xl_block.c
> > +++ b/tools/xl/xl_block.c
> > @@ -50,6 +50,11 @@ int main_blockattach(int argc, char **argv)
> >           return 0;
> >       }
> >   
> > +    if (disk.specification != LIBXL_DISK_SPECIFICATION_XEN) {
> > +        fprintf(stderr, "block-attach is only supported for specification 
> > xen\n");
> This check prevents a previously working `block-attach` command line
> from working.
>
>      # xl -Tvvv block-attach 0 /dev/vg/guest_disk,raw,hda
>      block-attach is only supported for specification xen
>
> At least, that works by adding ",specification=xen", but it should work
> without it as "xen" is the default (from the man page).
yes, you are right. thank you for pointing this out.


> Maybe the check is done too soon, or maybe a better place to do it would
> be in libxl.
>
> libxl__device_disk_setdefault() is called much later, while executing
> libxl_device_disk_add(), so `xl` can't use the default been done there
> to "disk.specification".
I got it.


> `xl block-attach` calls libxl_device_disk_add() which I think is only
> called for hotplug of disk. If I recall correctly, libxl__add_disks() is
> called instead at guest creation. So maybe it is possible to do
> something in libxl_device_disk_add(), but that a function defined by a
> macro, and the macro is using the same libxl__device_disk_add() that
> libxl_device_disk_add(). On the other hand, there is a "hotplug"
> parameter to libxl__device_disk_setdefault(), maybe that could be use?
Thank you for digging into the details here.

If I understood correctly your suggestion we simply can drop checks in 
main_blockattach() (and likely main_blockdetach() ?) and add it to 
libxl__device_disk_setdefault().

diff --git a/tools/libs/light/libxl_disk.c b/tools/libs/light/libxl_disk.c
index 9e82adb..96ace09 100644
--- a/tools/libs/light/libxl_disk.c
+++ b/tools/libs/light/libxl_disk.c
@@ -182,6 +182,11 @@ static int libxl__device_disk_setdefault(libxl__gc 
*gc, uint32_t domid,
         disk->transport = LIBXL_DISK_TRANSPORT_MMIO;
     }

+    if (hotplug && disk->specification != LIBXL_DISK_SPECIFICATION_XEN) {
+        LOGD(ERROR, domid, "Hotplug is only supported for specification 
xen");
+        return ERROR_FAIL;
+    }
+
     /* Force Qdisk backend for CDROM devices of guests with a device 
model. */
     if (disk->is_cdrom != 0 &&
         libxl__domain_type(gc, domid) == LIBXL_DOMAIN_TYPE_HVM) {


Is my understanding correct?


I have checked, it works:

root@generic-armv8-xt-dom0:~# xl block-attach DomU /dev/loop0,raw,xvda3
[  762.062874] xen-blkback: backend/vbd/3/51715: using 4 queues, 
protocol 1 (arm-abi)

root@generic-armv8-xt-dom0:~# xl block-attach DomU 
/dev/loop0,raw,xvda3,specification=virtio
libxl: error: libxl_disk.c:186:libxl__device_disk_setdefault: Domain 
3:Hotplug is only supported for specification xen
libxl: error: libxl_device.c:1468:device_addrm_aocomplete: unable to add 
device
> Cheers,
--
Regards,

Oleksandr Tyshchenko