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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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).

> 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).

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".

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


Cheers,

-- 
Anthony PERARD