[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>
  • From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
  • Date: Wed, 15 Jun 2022 15:23:54 +0100
  • Authentication-results: esa4.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none
  • Cc: <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>, Wei Liu <wl@xxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Nick Rosbrook <rosbrookn@xxxxxxxxx>, "Juergen Gross" <jgross@xxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, "Julien Grall" <julien@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>
  • Delivery-date: Wed, 15 Jun 2022 14:24:24 +0000
  • Ironport-data: A9a23:JdRMu6NAKQGub/nvrR1al8FynXyQoLVcMsEvi/4bfWQNrUp01j1Vn GBND2nTM/qIYjf9c4hzbdvip05Qu5+GydU1HQto+SlhQUwRpJueD7x1DKtR0wB+jCHnZBg6h ynLQoCYdKjYdleF+lH1dOKJQUBUjclkfJKlYAL/En03FFYMpBsJ00o5wbZn29Mw2LBVPivW0 T/Mi5yHULOa82Yc3lI8s8pvfzs24ZweEBtB1rAPTagjUG32zhH5P7pGTU2FFFPqQ5E8IwKPb 72rIIdVXI/u10xF5tuNyt4Xe6CRK1LYFVDmZnF+A8BOjvXez8CbP2lS2Pc0MC9qZzu1c99Zx 9937o6vFUAVeZaQtadDTxdBTgxQIvgTkFPHCSDXXc27ykTHdz3nwul0DVFwNoodkgp1KTgQr 7pCcmlLN03dwbLtqF64YrAEasALJc/3PIQZqzd4wCvQF/oOSpHfWaTao9Rf2V/cg+gRQ6yEO pdIMFKDajz6TSFJFwYvDag5kaSKnUevcy1ntQ2s8P9fD2/7k1UqjemF3MDuUseRWcxfk0Kcp 2TH12f0GBcXMJqY0zXt2nCjnOjUhgvgRZkfUra/85ZCgkCXx2EVIA0bUx28u/bRokSzQc5FI koYvC8nt7Ev9VeDR8P4GRa/pRasgBkYXNZBFvwg3yuEwKHU/gWxC3ANS3hKb9lOnN87Q3km2 0GEm/vtBCdzq/uFRHSF7LCWoDiufy8PIgcqYisJThAZ8sLjiI42hxPLCN1kFcadidn4Gir5x TyQmzQvnLUYjcMN1KKT8EjOhnSnoZ2hZhQy/Q/NWWWm6Ct2YYekY8qj7l2zxelEBJaUSB+Gp ndspiSFxLlQV9fXznXLGbhTWuHyjxqYDNHCqVFlJcIz6GjqxzmYIa0Tvi1wPRpEHNlRLFcFf 3TvVRNtCI57ZSX3MPUuPNjvV6zG3oC7S427C6m8gs5mJ8EoKVTZpHwGiVu4hTiFraQ6rU0o1 X53m+6IBG1SN6loxSHeqww1ge5ynXBWKY8+qPnGI/WbPVm2Pif9pU8tagfmUwzAxPrsTP/p2 9heLdCW7B5UTffzZCLamaZKcw1XdyNiW8+s+5cGHgJmHuaBMDh5Y8I9PJt7I9A190irvrygE o6Btr9wlwOk2CyvxfSiYXF/crL/NatCQYYAFXV0Zz6AgiF7Ca72tft3X8ZmJtEPqb08pcOYu tFYIq1s9NwUEmSZk9ncBLGgxLFfmOOD3l/XZnP7PmFuIPaNhWXho7fZQ+cmzwFWZgLfiCf0i +fIOt/zKXbbezlfMQ==
  • Ironport-hdrordr: A9a23:EAL9yqnrnflVGYH2++qzFIhnprjpDfIs3DAbv31ZSRFFG/Fxl6 iV8sjz8SWE7Ar5OUtQ/OxoV5PsfZqxz/JICMwqTNCftWrdyQmVxeNZjbcKqgeIc0aVygce79 YCT0EXMqyXMbEQt6fHCWeDfOod/A==
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.