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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, Jun 15, 2022 at 07:32:54PM +0300, Oleksandr wrote:
> On 15.06.22 17:23, Anthony PERARD wrote:
> > On Mon, Jun 13, 2022 at 09:05:20PM +0300, Oleksandr Tyshchenko wrote:
> > > 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");

Maybe check for `specification == VIRTIO` instead, and report that
hotplug isn't supported in virtio's case.

> +        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?

Yes, that looks correct.

But I didn't look at the block-detach, and it seems that this part only
use generic functions to remove a disk. So there is no easy way to
prevent hotunplug from libxl. But `xl` does have access to a fully
initialised "disk" so can check the value of "specification", I guess
the check can stay in main_blockdetach().

Cheers,

-- 
Anthony PERARD