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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 10.06.22 18:12, Anthony PERARD wrote:

Hello Anthony

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


> > +    }
> > +
> > +    /* 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.
yes


> > +        }
> > +        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.
I think I got your idea, will try to do this way


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

making virtio by default wasn't the original intention, it was entirely 
involuntarily))

> Thanks,
--
Regards,

Oleksandr Tyshchenko