Re: [PATCH V9 1/2] libxl: Add support for Virtio disk configuration

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, Jun 01, 2022 at 08:57:40PM +0300, Oleksandr Tyshchenko wrote:
> diff --git a/tools/libs/light/libxl_disk.c b/tools/libs/light/libxl_disk.c
> index a5ca778..e90bc25 100644
> --- a/tools/libs/light/libxl_disk.c
> +++ b/tools/libs/light/libxl_disk.c
> @@ -163,6 +163,25 @@ static int libxl__device_disk_setdefault(libxl__gc *gc, 
> uint32_t domid,
>      rc = libxl__resolve_domid(gc, disk->backend_domname, 
> &disk->backend_domid);
>      if (rc < 0) return rc;
>  
> +    if (disk->specification == LIBXL_DISK_SPECIFICATION_UNKNOWN)
> +        disk->specification = LIBXL_DISK_SPECIFICATION_XEN;
> +
> +    if (disk->specification == LIBXL_DISK_SPECIFICATION_XEN &&
> +        disk->transport != LIBXL_DISK_TRANSPORT_UNKNOWN) {
> +        LOGD(ERROR, domid, "Transport is only supported for specification 
> virtio");
> +        return ERROR_FAIL;

Could you return ERROR_INVAL instead?

> +    }
> +
> +    /* Force transport mmio for specification virtio for now */
> +    if (disk->specification == LIBXL_DISK_SPECIFICATION_VIRTIO) {
> +        if (!(disk->transport == LIBXL_DISK_TRANSPORT_UNKNOWN ||
> +              disk->transport == LIBXL_DISK_TRANSPORT_MMIO)) {
> +            LOGD(ERROR, domid, "Unsupported transport for specification 
> virtio");
> +            return ERROR_FAIL;

Same here.

> +        }
> +        disk->transport = LIBXL_DISK_TRANSPORT_MMIO;
> +    }
> +
>      /* 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) {
> @@ -575,6 +660,41 @@ cleanup:
>      return rc;
>  }
>  
> +static int libxl__device_disk_get_path(libxl__gc *gc, uint32_t domid,
> +                                       char **path)
> +{
> +    const char *dir;
> +    int rc;
> +
> +    /*
> +     * As we don't know exactly what device kind to be used here, guess it
> +     * by checking the presence of the corresponding path in Xenstore.
> +     * First, try to read path for vbd device (default) and if not exists
> +     * read path for virtio_disk device. This will work as long as both Xen 
> PV
> +     * and Virtio disk devices are not assigned to the same guest.
> +     */
> +    *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, *path, &dir);
> +    if (rc)
> +        return rc;
> +
> +    if (dir)
> +        return 0;
> +
> +    *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, *path, &dir);
> +    if (rc)
> +        return rc;
> +
> +    return 0;

I still don't like this implementation of get_path() which return a
different answer depending on the environment which can change from one
call to the next. I think get_path() was introduced when the path for
the kind of device didn't correspond to the common path which other kind
of devices uses. And when get_path() is implemented, it always returns
the same answer (see libxl_pci.c for the only implementation).

I don't really know how to deal with a type of device that have two
different frontend kind at the moment. (But maybe there's something in
libxl_usb.c which could be useful as a potential example, but one of the
kind doesn't use xenstore so is probably easier to deal with.). So I
guess we are stuck with an implementation of get_path() which may return
a different answer depending on thing outside of libxl's full control.

So, could you at least make it much harder to have libxl's view of a
guest in a weird state? I mean:
- always check both xenstore paths
  -> if both exist, return an error
  -> if only one exist, return that one.
  -> default to the "vbd" kind, otherwise.

That would be better than the current implementation which returns the
"virtio" path by default.

Thanks,

-- 
Anthony PERARD