On 15.06.22 17:23, Anthony PERARD wrote:
Hello Anthony
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).
yes, will do
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");
+ 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?
I have checked, it works:
root@generic-armv8-xt-dom0:~# xl block-attach DomU /dev/loop0,raw,xvda3
[ 762.062874] xen-blkback: backend/vbd/3/51715: using 4 queues,
protocol 1 (arm-abi)
root@generic-armv8-xt-dom0:~# xl block-attach DomU
/dev/loop0,raw,xvda3,specification=virtio
libxl: error: libxl_disk.c:186:libxl__device_disk_setdefault: Domain
3:Hotplug is only supported for specification xen
libxl: error: libxl_device.c:1468:device_addrm_aocomplete: unable to add
device
Cheers,
--
Regards,
Oleksandr Tyshchenko
