[PATCH v2 3/3] qemu: Setup host side of VDPA device for block copy
Setup the VDPA bits of the appropriate part of the image chain for block
copy.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 4
src/qemu/qemu_driver.c | 12
2 files changed, 16 insertions(+)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 41038fb994..42c12a5e9b 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -23,6 +23,7 @@
#include "qemu_domain.h"
#include "qemu_alias.h"
#include "qemu_security.h"
+#include "qemu_process.h"
#include "storage_source.h"
#include "viralloc.h"
@@ -3675,6 +3676,9 @@ qemuBlockPivot(virDomainObj *vm,
virQEMUCapsGet(priv->qemuCaps,
QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY) &&
virStorageSourceHasBacking(disk->mirror)) {
+if (qemuProcessPrepareHostStorageSourceChain(vm,
disk->mirror->backingStore) < 0)
+return -1;
+
if (!(chainattachdata =
qemuBuildStorageSourceChainAttachPrepareBlockdev(disk->mirror->backingStore)))
return -1;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 86da8da777..d00d2a27c6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14290,10 +14290,16 @@ qemuDomainBlockCopyCommon(virDomainObj *vm,
if (virQEMUCapsGet(priv->qemuCaps,
QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY)) {
g_autoptr(virStorageSource) terminator = virStorageSourceNew();
+if (qemuProcessPrepareHostStorageSource(vm, mirror) < 0)
+goto endjob;
+
if (!(data =
qemuBuildStorageSourceChainAttachPrepareBlockdevTop(mirror,
terminator)))
goto endjob;
} else {
+if (qemuProcessPrepareHostStorageSourceChain(vm, mirror) < 0)
+goto endjob;
+
if (!(data =
qemuBuildStorageSourceChainAttachPrepareBlockdev(mirror)))
goto endjob;
}
@@ -14308,6 +14314,9 @@ qemuDomainBlockCopyCommon(virDomainObj *vm,
if (mirror_shallow) {
/* if external backing store is populated we'll need to open it */
if (virStorageSourceHasBacking(mirror)) {
+if (qemuProcessPrepareHostStorageSourceChain(vm,
mirror->backingStore) < 0)
+goto endjob;
+
if (!(data =
qemuBuildStorageSourceChainAttachPrepareBlockdev(mirror->backingStore)))
goto endjob;
@@ -14321,6 +14330,9 @@ qemuDomainBlockCopyCommon(virDomainObj *vm,
mirrorBacking = mirror->backingStore;
}
+if (qemuProcessPrepareHostStorageSource(vm, mirror) < 0)
+goto endjob;
+
if (!(crdata =
qemuBuildStorageSourceChainAttachPrepareBlockdevTop(mirror,
mirrorBacking)))
goto endjob;
--
2.41.0
[PATCH v2 1/3] qemu: process: Extract host setup of disk device into helpers
Currently the code sets up only VDPA backends but will be used later in
hotplug code too.
This patch also uses normal forward iteration in the loop in
qemuProcessPrepareHostStorage as we don't need to remove disks from the
disk list at that point.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_process.c | 76 -
src/qemu/qemu_process.h | 7
2 files changed, 74 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 63c0c62a46..1ef032dbd2 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6777,6 +6777,69 @@ qemuProcessPrepareHostStorageSourceVDPA(virStorageSource
*src,
}
+/**
+ * See qemuProcessPrepareHostStorageSourceChain
+ */
+int
+qemuProcessPrepareHostStorageSource(virDomainObj *vm,
+virStorageSource *src)
+{
+/* connect to any necessary vdpa block devices */
+if (qemuProcessPrepareHostStorageSourceVDPA(src, vm->privateData) < 0)
+return -1;
+
+return 0;
+}
+
+
+/**
+ * qemuProcessPrepareHostStorageSourceChain:
+ *
+ * @vm: domain object
+ * @chain: source chain
+ *
+ * Prepare the host side of a disk for use with the VM. Note that this function
+ * accesses host resources.
+ */
+int
+qemuProcessPrepareHostStorageSourceChain(virDomainObj *vm,
+ virStorageSource *chain)
+{
+virStorageSource *n;
+
+for (n = chain; virStorageSourceIsBacking(n); n = n->backingStore) {
+if (qemuProcessPrepareHostStorageSource(vm, n) < 0)
+return -1;
+}
+
+return 0;
+}
+
+
+/**
+ * qemuProcessPrepareHostStorageDisk:
+ *
+ * @vm: domain object
+ * @disk: disk definition object
+ *
+ * Prepare the host side of a disk for use with the VM. Note that this function
+ * accesses host resources.
+ *
+ * Note that this function does not call qemuDomainDetermineDiskChain as that
is
+ * needed in qemuProcessPrepareHostStorage to remove disks based on the startup
+ * policy, thus other callers need to call it explicitly.
+ */
+int
+qemuProcessPrepareHostStorageDisk(virDomainObj *vm,
+ virDomainDiskDef *disk)
+{
+if (qemuProcessPrepareHostStorageSourceChain(vm, disk->src) < 0)
+return -1;
+
+return 0;
+}
+
+
static int
qemuProcessPrepareHostStorage(virQEMUDriver *driver,
virDomainObj *vm,
@@ -6813,16 +6876,11 @@ qemuProcessPrepareHostStorage(virQEMUDriver *driver,
return -1;
}
-/* connect to any necessary vdpa block devices */
-for (i = vm->def->ndisks; i > 0; i--) {
-size_t idx = i - 1;
-virDomainDiskDef *disk = vm->def->disks[idx];
-virStorageSource *src;
+for (i = 0; i < vm->def->ndisks; i++) {
+virDomainDiskDef *disk = vm->def->disks[i];
-for (src = disk->src; virStorageSourceIsBacking(src); src =
src->backingStore) {
-if (qemuProcessPrepareHostStorageSourceVDPA(src, vm->privateData)
< 0)
-return -1;
-}
+if (qemuProcessPrepareHostStorageDisk(vm, disk) < 0)
+return -1;
}
return 0;
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index ef05b46892..c1ea949215 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -133,6 +133,13 @@ int qemuProcessPrepareHost(virQEMUDriver *driver,
virDomainObj *vm,
unsigned int flags);
+int qemuProcessPrepareHostStorageSource(virDomainObj *vm,
+virStorageSource *src);
+int qemuProcessPrepareHostStorageSourceChain(virDomainObj *vm,
+ virStorageSource *chain);
+int qemuProcessPrepareHostStorageDisk(virDomainObj *vm,
+ virDomainDiskDef *disk);
+
int qemuProcessDeleteThreadContext(virDomainObj *vm);
int qemuProcessLaunch(virConnectPtr conn,
--
2.41.0
[PATCH v2 2/3] qemu: hotplug: Setup host side of VDPA device for disk hotplug
The code which opens the VDPA device and prepares it for FD passing was not called in the hotplug code path, preventing hotplug of VDPA disks with: error: internal error: argument key 'path' must not have null value Use the new helper qemuProcessPrepareHostStorageDisk to setup the VDPA definition. Closes: https://gitlab.com/libvirt/libvirt/-/issues/539 Signed-off-by: Peter Krempa --- src/qemu/qemu_hotplug.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1f7f5bdd26..fec7c4be4e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1007,6 +1007,9 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, if (qemuDomainDetermineDiskChain(driver, vm, disk, NULL) < 0) goto cleanup; +if (qemuProcessPrepareHostStorageDisk(vm, disk) < 0) +goto cleanup; + if (qemuHotplugAttachManagedPR(vm, disk->src, VIR_ASYNC_JOB_NONE) < 0) goto cleanup; -- 2.41.0
[PATCH v2 0/3] qemu: Fix hotplug and blockcopy to VDPA disks
Content-type: text/plain v2: - also deal with blockcopy (requires more helpers) Peter Krempa (3): qemu: process: Extract host setup of disk device into helpers qemu: hotplug: Setup host side of VDPA device for disk hotplug qemu: Setup host side of VDPA device for block copy src/qemu/qemu_block.c | 4 +++ src/qemu/qemu_driver.c | 12 +++ src/qemu/qemu_hotplug.c | 3 ++ src/qemu/qemu_process.c | 76 - src/qemu/qemu_process.h | 7 5 files changed, 93 insertions(+), 9 deletions(-) -- 2.41.0
Re: [PATCH 0/2] qemu: Fix hotplug of VDPA disks
On Thu, Oct 26, 2023 at 15:15:15 +0200, Peter Krempa wrote: > Peter Krempa (2): > qemu: process: Extract host setup of disk device into a helper > qemu: hotplug: Setup host side of VDPA device for disk hotplug > > src/qemu/qemu_hotplug.c | 3 +++ > src/qemu/qemu_process.c | 42 - > src/qemu/qemu_process.h | 3 +++ > 3 files changed, 39 insertions(+), 9 deletions(-) I'll be posting a v2 to properly address also block copy.
[PATCH 1/2] qemu: process: Extract host setup of disk device into a helper
Currently the code sets up only VDPA backends but will be used later in
hotplug code too.
This patch also uses normal forward iteration in the loop in
qemuProcessPrepareHostStorage as we don't need to remove disks from the
disk list at that point.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_process.c | 42 -
src/qemu/qemu_process.h | 3 +++
2 files changed, 36 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 63c0c62a46..b420ec283d 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6777,6 +6777,35 @@ qemuProcessPrepareHostStorageSourceVDPA(virStorageSource
*src,
}
+/**
+ * qemuProcessPrepareHostStorageDisk:
+ *
+ * @vm: domain object
+ * @disk: disk definition object
+ *
+ * Prepare the host side of a disk for use with the VM. Note that this function
+ * accesses host resources.
+ *
+ * Note that this function does not call qemuDomainDetermineDiskChain as that
is
+ * needed in qemuProcessPrepareHostStorage to remove disks based on the startup
+ * policy, thus other callers need to call it explicitly.
+ */
+int
+qemuProcessPrepareHostStorageDisk(virDomainObj *vm,
+ virDomainDiskDef *disk)
+{
+virStorageSource *n;
+
+for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) {
+/* connect to any necessary vdpa block devices */
+if (qemuProcessPrepareHostStorageSourceVDPA(n, vm->privateData) < 0)
+return -1;
+}
+
+return 0;
+}
+
+
static int
qemuProcessPrepareHostStorage(virQEMUDriver *driver,
virDomainObj *vm,
@@ -6813,16 +6842,11 @@ qemuProcessPrepareHostStorage(virQEMUDriver *driver,
return -1;
}
-/* connect to any necessary vdpa block devices */
-for (i = vm->def->ndisks; i > 0; i--) {
-size_t idx = i - 1;
-virDomainDiskDef *disk = vm->def->disks[idx];
-virStorageSource *src;
+for (i = 0; i < vm->def->ndisks; i++) {
+virDomainDiskDef *disk = vm->def->disks[i];
-for (src = disk->src; virStorageSourceIsBacking(src); src =
src->backingStore) {
-if (qemuProcessPrepareHostStorageSourceVDPA(src, vm->privateData)
< 0)
-return -1;
-}
+if (qemuProcessPrepareHostStorageDisk(vm, disk) < 0)
+return -1;
}
return 0;
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index ef05b46892..7b974259d3 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -133,6 +133,9 @@ int qemuProcessPrepareHost(virQEMUDriver *driver,
virDomainObj *vm,
unsigned int flags);
+int qemuProcessPrepareHostStorageDisk(virDomainObj *vm,
+ virDomainDiskDef *disk);
+
int qemuProcessDeleteThreadContext(virDomainObj *vm);
int qemuProcessLaunch(virConnectPtr conn,
--
2.41.0
[PATCH 2/2] qemu: hotplug: Setup host side of VDPA device for disk hotplug
The code which opens the VDPA device and prepares it for FD passing was not called in the hotplug code path, preventing hotplug of VDPA disks with: error: internal error: argument key 'path' must not have null value Use the new helper qemuProcessPrepareHostStorageDisk to setup the VDPA definition. Closes: https://gitlab.com/libvirt/libvirt/-/issues/539 Signed-off-by: Peter Krempa --- src/qemu/qemu_hotplug.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1f7f5bdd26..fec7c4be4e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1007,6 +1007,9 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, if (qemuDomainDetermineDiskChain(driver, vm, disk, NULL) < 0) goto cleanup; +if (qemuProcessPrepareHostStorageDisk(vm, disk) < 0) +goto cleanup; + if (qemuHotplugAttachManagedPR(vm, disk->src, VIR_ASYNC_JOB_NONE) < 0) goto cleanup; -- 2.41.0
[PATCH 0/2] qemu: Fix hotplug of VDPA disks
Peter Krempa (2): qemu: process: Extract host setup of disk device into a helper qemu: hotplug: Setup host side of VDPA device for disk hotplug src/qemu/qemu_hotplug.c | 3 +++ src/qemu/qemu_process.c | 42 - src/qemu/qemu_process.h | 3 +++ 3 files changed, 39 insertions(+), 9 deletions(-) -- 2.41.0
[PATCH] docs: formatdomain: Clarify that the SLIC ACPI table config is available for all modes
Move the docs for the element under a common section as it's not specific for direct kernel boot. In fact the original use was for Windows activation. Fixes: 72f652da63255c7f1a9914625cce617dde9128d0 Signed-off-by: Peter Krempa --- docs/formatdomain.rst | 29 + 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 03735e4593..459815d2b5 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -387,9 +387,6 @@ and full virtualized guests. /root/f8-i386-initrd console=ttyS0 ks=http://example.com/f8-i386/os/ /root/ppc.dtb - - /path/to/slic.dat - ... @@ -413,11 +410,6 @@ and full virtualized guests. The contents of this element specify the fully-qualified path to the (optional) device tree binary (dtb) image in the host OS. :since:`Since 1.0.4` -``acpi`` - The ``table`` element contains a fully-qualified path to the ACPI table. The - ``type`` attribute contains the ACPI table type (currently only ``slic`` is - supported) :since:`Since 1.3.5 (QEMU)` :since:`Since 5.9.0 (Xen)` - Container boot ~~ @@ -470,6 +462,27 @@ If you want to enable user namespace, set the ``idmap`` element. The ``uid`` and +Common element configuration +~ + +These options apply to any form of booting of the guest OS. + +:: + + ... + + ... + + /path/to/slic.dat + + + ... + +``acpi`` + The ``table`` element contains a fully-qualified path to the ACPI table. The + ``type`` attribute contains the ACPI table type (currently only ``slic`` is + supported) :since:`Since 1.3.5 (QEMU)` :since:`Since 5.9.0 (Xen)` + SMBIOS System Information - -- 2.41.0
[PATCH 15/22] qemuBlockStorageSourceGetBackendProps: Unify ordering of fields
Use the same ordering of the relevant fields as we do for the format
layer -blockdev so that later they can be refactored without test
fallout.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 2 +-
.../xml2json/dir-fat-cache.json | 6 ++---
.../file-backing_basic-cache-directsync.json | 24 +--
.../file-backing_basic-cache-none.json| 24 +--
.../file-backing_basic-cache-unsafe.json | 24 +--
.../file-backing_basic-cache-writeback.json | 24 +--
...file-backing_basic-cache-writethrough.json | 24 +--
.../xml2json/file-raw-aio_native.json | 6 ++---
...work-qcow2-backing-chain-cache-unsafe.json | 12 +-
.../blkdeviotune-group-num.x86_64-latest.args | 4 ++--
...blkdeviotune-max-length.x86_64-latest.args | 4 ++--
.../blkdeviotune-max.x86_64-latest.args | 4 ++--
.../blkdeviotune.x86_64-latest.args | 4 ++--
.../controller-order.x86_64-latest.args | 2 +-
.../disk-aio.x86_64-latest.args | 2 +-
.../disk-cache.x86_64-latest.args | 10
.../disk-error-policy-s390x.s390x-latest.args | 6 ++---
.../disk-error-policy.x86_64-latest.args | 6 ++---
.../disk-metadata-cache.x86_64-latest.args| 6 ++---
.../disk-network-nfs.x86_64-latest.args | 2 +-
...rk-tlsx509-nbd-hostname.x86_64-latest.args | 2 +-
...disk-network-tlsx509-nbd.x86_64-5.2.0.args | 2 +-
...isk-network-tlsx509-nbd.x86_64-latest.args | 2 +-
...isk-network-tlsx509-vxhs.x86_64-5.0.0.args | 6 ++---
.../disk-nvme.x86_64-latest.args | 2 +-
.../disk-shared.x86_64-latest.args| 6 ++---
.../disk-slices.x86_64-latest.args| 2 +-
.../disk-snapshot.x86_64-latest.args | 4 ++--
.../disk-transient.x86_64-latest.args | 2 +-
.../disk-vhostvdpa.x86_64-latest.args | 2 +-
.../name-escape.x86_64-latest.args| 2 +-
.../user-aliases.x86_64-latest.args | 4 ++--
...eo-bochs-display-device.x86_64-latest.args | 2 +-
...-device-pciaddr-default.x86_64-latest.args | 2 +-
...video-qxl-device-vgamem.x86_64-latest.args | 2 +-
.../video-qxl-device.x86_64-latest.args | 2 +-
...o-qxl-sec-device-vgamem.x86_64-latest.args | 2 +-
.../video-qxl-sec-device.x86_64-latest.args | 2 +-
...eo-ramfb-display-device.x86_64-latest.args | 2 +-
...video-vga-device-vgamem.x86_64-latest.args | 2 +-
.../video-vga-device.x86_64-latest.args | 2 +-
.../video-virtio-blob-off.x86_64-latest.args | 2 +-
.../video-virtio-blob-on.x86_64-latest.args | 2 +-
...video-virtio-gpu-device.x86_64-latest.args | 2 +-
...video-virtio-gpu-sdl-gl.x86_64-latest.args | 2 +-
...deo-virtio-gpu-spice-gl.x86_64-latest.args | 2 +-
.../video-virtio-gpu-virgl.x86_64-latest.args | 2 +-
.../video-virtio-vga.x86_64-latest.args | 2 +-
48 files changed, 132 insertions(+), 132 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 1fa9627444..689eb535cb 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1113,10 +1113,10 @@ qemuBlockStorageSourceGetBackendProps(virStorageSource
*src,
if (virJSONValueObjectAdd(,
"s:node-name", nodename,
- "A:cache", ,
"T:read-only", ro,
"T:auto-read-only", aro,
"S:discard", discardstr,
+ "A:cache", ,
NULL) < 0)
return NULL;
}
diff --git a/tests/qemublocktestdata/xml2json/dir-fat-cache.json
b/tests/qemublocktestdata/xml2json/dir-fat-cache.json
index 2a24175ef4..320dc77de1 100644
--- a/tests/qemublocktestdata/xml2json/dir-fat-cache.json
+++ b/tests/qemublocktestdata/xml2json/dir-fat-cache.json
@@ -14,10 +14,10 @@
"floppy": false,
"rw": false,
"node-name": "node-s",
+ "auto-read-only": true,
+ "discard": "unmap",
"cache": {
"direct": true,
"no-flush": false
- },
- "auto-read-only": true,
- "discard": "unmap"
+ }
}
diff --git
a/tests/qemublocktestdata/xml2json/file-backing_basic-cache-directsync.json
b/tests/qemublocktestdata/xml2json/file-backing_basic-cache-directsync.json
index 1d5ae09813..7a8d0686f6 100644
--- a/tests/qemublocktestdata/xml2json/file-backing_basic-cache-directsync.json
+++ b/tests/qemublocktestdata/xml2json/file-backing_basic-cache-directsync.json
@@ -13,12 +13,12 @@
"driver": "file",
"filename": "/var/lib/libvirt/images/a",
"node-name": "node-a-s",
+ "auto-read-only": true,
+ "discard&q
[PATCH 10/22] qemublocktest: Drop 'sheepdog' and 'vxhs' test cases
QEMU deprecated and removed support for those protocols, but due to a
logic bug in the tests it was not caught. Remove the test cases first.
Signed-off-by: Peter Krempa
---
tests/qemublocktest.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
index ce76150200..a89dddf002 100644
--- a/tests/qemublocktest.c
+++ b/tests/qemublocktest.c
@@ -1084,12 +1084,6 @@ mymain(void)
TEST_JSON_FORMAT_NET("\n"
" \n"
"\n");
-TEST_JSON_FORMAT_NET("\n"
- " \n"
- "\n");
-TEST_JSON_FORMAT_NET("\n"
- " \n"
- "\n");
#define TEST_DISK_TO_JSON_FULL(nme, fl) \
do { \
--
2.41.0
[PATCH 22/22] qemu: block: Remove unused flags QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_ flags
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP is no longer
referenced inside the code.
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY is passed from
various code paths to the qemuBlockStorageSourceGetBackendProps helper,
but it's no longer used.
Both thus can be removed.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 7 +--
src/qemu/qemu_block.h | 4 +---
tests/qemublocktest.c | 7 ++-
3 files changed, 4 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index a625e72a5d..382015f293 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1006,11 +1006,6 @@
qemuBlockStorageSourceAddBlockdevCommonProps(virJSONValue **props,
* use legacy formatting of attributes (for -drive / old qemus)
* QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY:
* omit any data which does not identify the image itself
- * QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY:
- * use the auto-read-only feature of qemu
- * QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP:
- * don't enable 'discard:unmap' option for passing through discards
- * (note that this is disabled also for _LEGACY and _TARGET_ONLY options)
* QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_EFFECTIVE_NODE:
* the 'protocol' node is used as the effective/top node of a
virStorageSource
*
@@ -1510,7 +1505,7 @@
qemuBlockStorageSourceAttachPrepareBlockdev(virStorageSource *src,
virStorageSource *backingStore)
{
g_autoptr(qemuBlockStorageSourceAttachData) data = NULL;
-unsigned int backendpropsflags =
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY;
+unsigned int backendpropsflags = 0;
data = g_new0(qemuBlockStorageSourceAttachData, 1);
diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h
index 9757108501..5c784a4386 100644
--- a/src/qemu/qemu_block.h
+++ b/src/qemu/qemu_block.h
@@ -66,9 +66,7 @@
qemuBlockStorageSourceSupportsConcurrentAccess(virStorageSource *src);
typedef enum {
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_LEGACY = 1 << 0,
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY = 1 << 1,
-QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY = 1 << 2,
-QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP = 1 << 3,
-QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_EFFECTIVE_NODE = 1 << 4,
+QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_EFFECTIVE_NODE = 1 << 2,
} qemuBlockStorageSourceBackendPropsFlags;
virJSONValue *
diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
index b8db1a2570..c581bd1748 100644
--- a/tests/qemublocktest.c
+++ b/tests/qemublocktest.c
@@ -285,9 +285,6 @@ testQemuDiskXMLToProps(const void *opaque)
for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) {
g_autofree char *backingstore = NULL;
-unsigned int backendpropsflagsnormal =
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY;
-unsigned int backendpropsflagstarget =
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY |
-
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY;
if (testQemuDiskXMLToJSONFakeSecrets(n) < 0)
return -1;
@@ -298,8 +295,8 @@ testQemuDiskXMLToProps(const void *opaque)
qemuDomainPrepareDiskSourceData(disk, n);
if (!(formatProps = qemuBlockStorageSourceGetFormatProps(n,
n->backingStore)) ||
-!(storageSrcOnlyProps = qemuBlockStorageSourceGetBackendProps(n,
backendpropsflagstarget)) ||
-!(storageProps = qemuBlockStorageSourceGetBackendProps(n,
backendpropsflagsnormal)) ||
+!(storageSrcOnlyProps = qemuBlockStorageSourceGetBackendProps(n,
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY)) ||
+!(storageProps = qemuBlockStorageSourceGetBackendProps(n, 0)) ||
!(backingstore = qemuBlockGetBackingStoreString(n, true))) {
if (!data->fail) {
VIR_TEST_VERBOSE("failed to generate qemu blockdev props");
--
2.41.0
[PATCH 20/22] qemuBlockStorageSourceGetBackendProps: Use qemuBlockStorageSourceAddBlockdevCommonProps
Use the qemuBlockStorageSourceAddBlockdevCommonProps helper when
formatting protocol layer both when it's used as backing for a format
node and when it's used as the effective node.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 29 +++--
1 file changed, 3 insertions(+), 26 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 7902ef31b3..5c8d107257 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1184,33 +1184,10 @@ qemuBlockStorageSourceGetBackendProps(virStorageSource
*src,
return NULL;
if (!onlytarget && !legacy) {
-const char *nodename = qemuBlockStorageSourceGetStorageNodename(src);
-
-if (flags & QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_EFFECTIVE_NODE) {
-if (qemuBlockStorageSourceAddBlockdevCommonProps(, src,
nodename, true) < 0)
-return NULL;
-} else {
-g_autoptr(virJSONValue) cache = NULL;
-const char *discardstr = "unmap";
-
-if (flags & QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP)
-discardstr = NULL;
-
-if (qemuBlockNodeNameValidate(nodename) < 0)
+if (qemuBlockStorageSourceAddBlockdevCommonProps(, src,
+
qemuBlockStorageSourceGetStorageNodename(src),
+ !!(flags &
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_EFFECTIVE_NODE)) < 0)
return NULL;
-
-if (qemuBlockStorageSourceGetBlockdevGetCacheProps(src, ) <
0)
-return NULL;
-
-if (virJSONValueObjectAdd(,
- "s:node-name", nodename,
- "T:read-only", ro,
- "T:auto-read-only", aro,
- "S:discard", discardstr,
- "A:cache", ,
- NULL) < 0)
-return NULL;
-}
}
return g_steal_pointer();
--
2.41.0
[PATCH 19/22] qemuBuildHostdevSCSIAttachPrepare: Use "effective node" mode for getting blockdev props
The resulting properties are identical, as the hostdev backend code doesn't set any of the extra properties. Signed-off-by: Peter Krempa --- src/qemu/qemu_command.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 98e7fa3d80..2674dd6959 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5066,8 +5066,7 @@ qemuBuildHostdevSCSIAttachPrepare(virDomainHostdevDef *hostdev, ret->storageNodeName = qemuBlockStorageSourceGetStorageNodename(src); *backendAlias = qemuBlockStorageSourceGetStorageNodename(src); -if (!(ret->storageProps = qemuBlockStorageSourceGetBackendProps(src, - QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP))) +if (!(ret->storageProps = qemuBlockStorageSourceGetBackendProps(src, QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_EFFECTIVE_NODE))) return NULL; if (qemuBuildStorageSourceAttachPrepareCommon(src, ret) < 0) -- 2.41.0
[PATCH 08/22] qemuBlockStorageSourceGetBlockdevGetCacheProps: Return the cache object rather than appending it
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 29 -
src/qemu/qemu_domain.c | 2 +-
2 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 0f47b5b37f..41038fb994 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -913,24 +913,20 @@ qemuBlockStorageSourceGetVhostVdpaProps(virStorageSource
*src)
static int
qemuBlockStorageSourceGetBlockdevGetCacheProps(virStorageSource *src,
- virJSONValue *props)
+ virJSONValue **cache)
{
-g_autoptr(virJSONValue) cacheobj = NULL;
bool direct = false;
bool noflush = false;
if (!qemuDomainDiskCachemodeFlags(src->cachemode, NULL, , ))
return 0;
-if (virJSONValueObjectAdd(,
+if (virJSONValueObjectAdd(cache,
"b:direct", direct,
"b:no-flush", noflush,
NULL) < 0)
return -1;
-if (virJSONValueObjectAppend(props, "cache", ) < 0)
-return -1;
-
return 0;
}
@@ -1109,10 +1105,13 @@ qemuBlockStorageSourceGetBackendProps(virStorageSource
*src,
return NULL;
if (!legacy) {
-if (qemuBlockStorageSourceGetBlockdevGetCacheProps(src, fileprops)
< 0)
+g_autoptr(virJSONValue) cache = NULL;
+
+if (qemuBlockStorageSourceGetBlockdevGetCacheProps(src, ) <
0)
return NULL;
if (virJSONValueObjectAdd(,
+ "A:cache", ,
"T:read-only", ro,
"T:auto-read-only", aro,
NULL) < 0)
@@ -1278,10 +1277,14 @@
qemuBlockStorageSourceGetBlockdevFormatCommonProps(virStorageSource *src)
int detectZeroesMode = virDomainDiskGetDetectZeroesMode(src->discard,
src->detect_zeroes);
g_autoptr(virJSONValue) props = NULL;
+g_autoptr(virJSONValue) cache = NULL;
if
(qemuBlockNodeNameValidate(qemuBlockStorageSourceGetFormatNodename(src)) < 0)
return NULL;
+if (qemuBlockStorageSourceGetBlockdevGetCacheProps(src, ) < 0)
+return NULL;
+
if (src->discard)
discard = virDomainDiskDiscardTypeToString(src->discard);
@@ -1297,12 +1300,10 @@
qemuBlockStorageSourceGetBlockdevFormatCommonProps(virStorageSource *src)
"b:read-only", src->readonly,
"S:discard", discard,
"S:detect-zeroes", detectZeroes,
+ "A:cache", ,
NULL) < 0)
return NULL;
-if (qemuBlockStorageSourceGetBlockdevGetCacheProps(src, props) < 0)
-return NULL;
-
return g_steal_pointer();
}
@@ -1439,10 +1440,14 @@ static virJSONValue *
qemuBlockStorageSourceGetBlockdevStorageSliceProps(virStorageSource *src)
{
g_autoptr(virJSONValue) props = NULL;
+g_autoptr(virJSONValue) cache = NULL;
if (qemuBlockNodeNameValidate(src->sliceStorage->nodename) < 0)
return NULL;
+if (qemuBlockStorageSourceGetBlockdevGetCacheProps(src, ) < 0)
+return NULL;
+
if (virJSONValueObjectAdd(,
"s:driver", "raw",
"s:node-name", src->sliceStorage->nodename,
@@ -1451,12 +1456,10 @@
qemuBlockStorageSourceGetBlockdevStorageSliceProps(virStorageSource *src)
"s:file",
qemuBlockStorageSourceGetStorageNodename(src),
"b:auto-read-only", true,
"s:discard", "unmap",
+ "A:cache", ,
NULL) < 0)
return NULL;
-if (qemuBlockStorageSourceGetBlockdevGetCacheProps(src, props) < 0)
-return NULL;
-
return g_steal_pointer();
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 0b36a49bdf..ae19ce884b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11442,7 +11442,7 @@ qemuDomainDiskCachemodeFlags(virDomainDiskCache
cachemode,
* directsync â false true false
* unsafe â true false true
*/
-switch ((virDomainDiskCache) cachemode) {
+switch (cachemode) {
case VIR_DOMAIN_DISK_CACHE_DISABLE: /* 'none' */
*writeback = true;
*direct = true;
--
2.41.0
[PATCH 13/22] qemuBlockStorageSourceGetBackendProps: Unify cases for '!onlytarget' and '!legacy'
At this point only a single code path (for formatting -drive for legacy
SD cards) uses the 'legacy' output and that code path doesn't populate
the node name. Thus we can unify the code block and simplify the JSON
formatters.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 40 ++--
1 file changed, 18 insertions(+), 22 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 41038fb994..ca68b9ab66 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1097,32 +1097,28 @@ qemuBlockStorageSourceGetBackendProps(virStorageSource
*src,
if (driver && virJSONValueObjectPrependString(fileprops, "driver", driver)
< 0)
return NULL;
-if (!onlytarget) {
-if
(qemuBlockNodeNameValidate(qemuBlockStorageSourceGetStorageNodename(src)) < 0 ||
-virJSONValueObjectAdd(,
- "S:node-name",
qemuBlockStorageSourceGetStorageNodename(src),
- NULL) < 0)
-return NULL;
+if (!onlytarget && !legacy) {
+g_autoptr(virJSONValue) cache = NULL;
+const char *discardstr = "unmap";
+const char *nodename = qemuBlockStorageSourceGetStorageNodename(src);
-if (!legacy) {
-g_autoptr(virJSONValue) cache = NULL;
+if (flags & QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP)
+discardstr = NULL;
-if (qemuBlockStorageSourceGetBlockdevGetCacheProps(src, ) <
0)
-return NULL;
+if (qemuBlockNodeNameValidate(nodename) < 0)
+return NULL;
-if (virJSONValueObjectAdd(,
- "A:cache", ,
- "T:read-only", ro,
- "T:auto-read-only", aro,
- NULL) < 0)
-return NULL;
+if (qemuBlockStorageSourceGetBlockdevGetCacheProps(src, ) < 0)
+return NULL;
-if (!(flags & QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP)
&&
-virJSONValueObjectAdd(,
- "s:discard", "unmap",
- NULL) < 0)
-return NULL;
-}
+if (virJSONValueObjectAdd(,
+ "s:node-name", nodename,
+ "A:cache", ,
+ "T:read-only", ro,
+ "T:auto-read-only", aro,
+ "S:discard", discardstr,
+ NULL) < 0)
+return NULL;
}
return g_steal_pointer();
--
2.41.0
[PATCH 14/22] virDomainDiskGetDetectZeroesMode: Return proper type
Change the return value type to 'virDomainDiskGetDetectZeroes'.
Signed-off-by: Peter Krempa
---
src/conf/domain_conf.c | 2 +-
src/conf/domain_conf.h | 2 +-
src/qemu/qemu_block.c | 4 ++--
src/qemu/qemu_command.c | 4 ++--
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 80f467ae7a..fa97def9f7 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -30592,7 +30592,7 @@ virDomainDiskTranslateSourcePool(virDomainDiskDef *def)
* don't change it in the XML for easier adjustments. This behaviour is
* documented.
*/
-int
+virDomainDiskDetectZeroes
virDomainDiskGetDetectZeroesMode(virDomainDiskDiscard discard,
virDomainDiskDetectZeroes detect_zeroes)
{
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 98f99721f0..7c0e017038 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -4406,7 +4406,7 @@ virDomainNetResolveActualType(virDomainNetDef *iface)
int virDomainDiskTranslateSourcePool(virDomainDiskDef *def);
-int
+virDomainDiskDetectZeroes
virDomainDiskGetDetectZeroesMode(virDomainDiskDiscard discard,
virDomainDiskDetectZeroes detect_zeroes);
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index ca68b9ab66..1fa9627444 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1270,8 +1270,8 @@
qemuBlockStorageSourceGetBlockdevFormatCommonProps(virStorageSource *src)
{
const char *detectZeroes = NULL;
const char *discard = NULL;
-int detectZeroesMode = virDomainDiskGetDetectZeroesMode(src->discard,
-
src->detect_zeroes);
+virDomainDiskDetectZeroes detectZeroesMode =
virDomainDiskGetDetectZeroesMode(src->discard,
+
src->detect_zeroes);
g_autoptr(virJSONValue) props = NULL;
g_autoptr(virJSONValue) cache = NULL;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index fd0f12f304..98e7fa3d80 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1680,8 +1680,8 @@ static char *
qemuBuildDriveStr(virDomainDiskDef *disk)
{
g_auto(virBuffer) opt = VIR_BUFFER_INITIALIZER;
-int detect_zeroes = virDomainDiskGetDetectZeroesMode(disk->discard,
- disk->detect_zeroes);
+virDomainDiskDetectZeroes detect_zeroes =
virDomainDiskGetDetectZeroesMode(disk->discard,
+
disk->detect_zeroes);
if (qemuBuildDriveSourceStr(disk, ) < 0)
return NULL;
--
2.41.0
[PATCH 16/22] qemu: block: Add helper to add common properties for -blockdev configuration
The new helper replaces qemuBlockStorageSourceGetBlockdevFormatCommonProps
and the two inline instances generating the common properties for a
blockdev layer.
The new helper is to be used for both the format layer and the storage
backing layer, thus a new parameter 'effective' switches between the
modes.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 128 +-
1 file changed, 88 insertions(+), 40 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 689eb535cb..aeb05a7f00 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -931,6 +931,90 @@
qemuBlockStorageSourceGetBlockdevGetCacheProps(virStorageSource *src,
}
+/**
+ * qemuBlockStorageSourceAddBlockdevCommonProps:
+ * @props: JSON object to append the props to
+ * @src: storage source
+ * @nodename: nodename to use for @src
+ * @effective: Whether props for the effective(topmost) layer are to be
formatted
+ *
+ * Add the common props (node name, read-only state, cache configuration,
discard)
+ * to a JSON object for a -blockdev definition. If @effective is true,
+ * the props are configured for the topmost layer used to access the data,
+ * otherwise the props are configured for the storage protocol backing a format
+ * layer.
+ */
+static int
+qemuBlockStorageSourceAddBlockdevCommonProps(virJSONValue **props,
+ virStorageSource *src,
+ const char *nodename,
+ bool effective)
+{
+virStorageType actualType = virStorageSourceGetActualType(src);
+g_autoptr(virJSONValue) cache = NULL;
+const char *detectZeroes = NULL;
+const char *discard = NULL;
+virTristateBool autoReadOnly = VIR_TRISTATE_BOOL_ABSENT;
+virTristateBool readOnly = VIR_TRISTATE_BOOL_ABSENT;
+
+if (qemuBlockNodeNameValidate(nodename) < 0)
+return -1;
+
+if (qemuBlockStorageSourceGetBlockdevGetCacheProps(src, ) < 0)
+return -1;
+
+if (effective) {
+virDomainDiskDetectZeroes dz =
virDomainDiskGetDetectZeroesMode(src->discard,
+
src->detect_zeroes);
+
+if (src->discard != VIR_DOMAIN_DISK_DISCARD_DEFAULT)
+discard = virDomainDiskDiscardTypeToString(src->discard);
+
+if (dz != VIR_DOMAIN_DISK_DETECT_ZEROES_DEFAULT)
+detectZeroes = virDomainDiskDetectZeroesTypeToString(dz);
+
+autoReadOnly = VIR_TRISTATE_BOOL_ABSENT;
+readOnly = virTristateBoolFromBool(src->readonly);
+} else {
+/* when passing a FD to qemu via the /dev/fdset mechanism qemu
+ * fetches the appropriate FD from the fdset by checking that it has
+ * the correct accessmode. Now with 'auto-read-only' in effect qemu
+ * wants to use a read-only FD first. If the user didn't pass multiple
+ * FDs the feature will not work regardless, so we'll not enable it. */
+if ((actualType == VIR_STORAGE_TYPE_FILE || actualType ==
VIR_STORAGE_TYPE_BLOCK) &&
+src->fdtuple && src->fdtuple->nfds == 1) {
+autoReadOnly = VIR_TRISTATE_BOOL_ABSENT;
+
+/* now we setup the normal readonly flag. If user requested write
access honour it */
+if (src->fdtuple->writable)
+readOnly = VIR_TRISTATE_BOOL_NO;
+else
+readOnly = virTristateBoolFromBool(src->readonly);
+} else {
+autoReadOnly = VIR_TRISTATE_BOOL_YES;
+}
+
+discard = "unmap";
+}
+
+/* currently unhandled global properties:
+ * '*force-share': 'bool'
+ */
+
+if (virJSONValueObjectAdd(props,
+ "s:node-name", nodename,
+ "T:read-only", readOnly,
+ "T:auto-read-only", autoReadOnly,
+ "S:discard", discard,
+ "S:detect-zeroes", detectZeroes,
+ "A:cache", ,
+ NULL) < 0)
+return -1;
+
+return 0;
+}
+
+
/**
* qemuBlockStorageSourceGetBackendProps:
* @src: disk source
@@ -1265,52 +1349,16 @@
qemuBlockStorageSourceGetFormatQcow2Props(virStorageSource *src,
}
-static virJSONValue *
-qemuBlockStorageSourceGetBlockdevFormatCommonProps(virStorageSource *src)
-{
-const char *detectZeroes = NULL;
-const char *discard = NULL;
-virDomainDiskDetectZeroes detectZeroesMode =
virDomainDiskGetDetectZeroesMode(src->discard,
-
src->detect_zeroes);
-g_autoptr(virJSONValue) props = NULL;
-g_autoptr(virJSONValue) cache = NULL;
-
-if
(qemuBlockNodeNameValidate(qemuBlo
[PATCH 07/22] qemuDomainDiskCachemodeFlags: Simplify usage
Return whether a relevant cachemode was presented rather than returning
an error, so that callers can be simplified. Use the proper enum type as
argument rather than typecasting in the switch statement.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 5 +
src/qemu/qemu_command.c | 15 ++-
src/qemu/qemu_domain.c | 31 +--
src/qemu/qemu_domain.h | 4 ++--
4 files changed, 26 insertions(+), 29 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 3be12b47e3..0f47b5b37f 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -919,12 +919,9 @@
qemuBlockStorageSourceGetBlockdevGetCacheProps(virStorageSource *src,
bool direct = false;
bool noflush = false;
-if (src->cachemode == VIR_DOMAIN_DISK_CACHE_DEFAULT)
+if (!qemuDomainDiskCachemodeFlags(src->cachemode, NULL, , ))
return 0;
-if (qemuDomainDiskCachemodeFlags(src->cachemode, NULL, , )
< 0)
-return -1;
-
if (virJSONValueObjectAdd(,
"b:direct", direct,
"b:no-flush", noflush,
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index dbef8d0068..fd0f12f304 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1889,16 +1889,13 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
wwn = virJSONValueNewNumberUlong(w);
}
-if (disk->cachemode != VIR_DOMAIN_DISK_CACHE_DEFAULT) {
-/* VIR_DOMAIN_DISK_DEVICE_LUN translates into 'scsi-block'
- * where any caching setting makes no sense. */
-if (disk->device != VIR_DOMAIN_DISK_DEVICE_LUN) {
-bool wb;
-
-if (qemuDomainDiskCachemodeFlags(disk->cachemode, , NULL,
- NULL) < 0)
-return NULL;
+/* 'write-cache' component of disk->cachemode is set on device level.
+ * VIR_DOMAIN_DISK_DEVICE_LUN translates into 'scsi-block' where any
+ * caching setting makes no sense. */
+if (disk->device != VIR_DOMAIN_DISK_DEVICE_LUN) {
+bool wb;
+if (qemuDomainDiskCachemodeFlags(disk->cachemode, , NULL, NULL)) {
writeCache = virTristateSwitchFromBool(wb);
}
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 995aa3f79c..0b36a49bdf 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11406,13 +11406,18 @@ qemuDomainPrepareHostdev(virDomainHostdevDef *hostdev,
/**
* qemuDomainDiskCachemodeFlags:
+ * @cachemode: aggregated cache mode
+ * @writeback: populated with 'writeback' component of @cachemode (may be NULL)
+ * @direct: populated with 'direct' component of @cachemode (may be NULL)
+ * @noflush: populated with 'noflush' component of @cachemode (may be NULL)
*
- * Converts disk cachemode to the cache mode options for qemu. Returns -1 for
- * invalid @cachemode values and fills the flags and returns 0 on success.
- * Flags may be NULL.
+ * Converts disk @cachemode to the cache mode options for qemu according to the
+ * table below.
+ *
+ * Returns true if @cachemode is a relevant cache mode setting.
*/
-int
-qemuDomainDiskCachemodeFlags(int cachemode,
+bool
+qemuDomainDiskCachemodeFlags(virDomainDiskCache cachemode,
bool *writeback,
bool *direct,
bool *noflush)
@@ -11442,40 +11447,38 @@ qemuDomainDiskCachemodeFlags(int cachemode,
*writeback = true;
*direct = true;
*noflush = false;
-break;
+return true;
case VIR_DOMAIN_DISK_CACHE_WRITETHRU:
*writeback = false;
*direct = false;
*noflush = false;
-break;
+return true;
case VIR_DOMAIN_DISK_CACHE_WRITEBACK:
*writeback = true;
*direct = false;
*noflush = false;
-break;
+return true;
case VIR_DOMAIN_DISK_CACHE_DIRECTSYNC:
*writeback = false;
*direct = true;
*noflush = false;
-break;
+return true;
case VIR_DOMAIN_DISK_CACHE_UNSAFE:
*writeback = true;
*direct = false;
*noflush = true;
-break;
+return true;
case VIR_DOMAIN_DISK_CACHE_DEFAULT:
case VIR_DOMAIN_DISK_CACHE_LAST:
-default:
-virReportEnumRangeError(virDomainDiskCache, cachemode);
-return -1;
+return false;
}
-return 0;
+return false;
}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index a3fc6acaaa..1e56e50672 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -1008,8 +1008,8 @@ qemuDomainPrepareDiskSource(virDomainDiskDef *disk,
qemuDomainObjPrivate *priv,
virQEMUDriverConfig *cfg);
-int
-qemuDomainDiskCachemodeFlags(int cachemode,
+bool
+qemu
[PATCH 18/22] qemuBlockStorageSourceGetBackendProps: Introduce QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_EFFECTIVE_NODE
Introduce a mode where the protocol layer -blockdev will be formatted
so that it can be used as the effective node (used to access data from
the device). For this new mode we'll use
qemuBlockStorageSourceAddBlockdevCommonProps.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 40
src/qemu/qemu_block.h | 1 +
2 files changed, 25 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index dc6f34530b..7902ef31b3 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1030,6 +1030,8 @@ qemuBlockStorageSourceAddBlockdevCommonProps(virJSONValue
**props,
* QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP:
* don't enable 'discard:unmap' option for passing through discards
* (note that this is disabled also for _LEGACY and _TARGET_ONLY options)
+ * QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_EFFECTIVE_NODE:
+ * the 'protocol' node is used as the effective/top node of a
virStorageSource
*
* Creates a JSON object describing the underlying storage or protocol of a
* storage source. Returns NULL on error and reports an appropriate error
message.
@@ -1182,27 +1184,33 @@ qemuBlockStorageSourceGetBackendProps(virStorageSource
*src,
return NULL;
if (!onlytarget && !legacy) {
-g_autoptr(virJSONValue) cache = NULL;
-const char *discardstr = "unmap";
const char *nodename = qemuBlockStorageSourceGetStorageNodename(src);
-if (flags & QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP)
-discardstr = NULL;
+if (flags & QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_EFFECTIVE_NODE) {
+if (qemuBlockStorageSourceAddBlockdevCommonProps(, src,
nodename, true) < 0)
+return NULL;
+} else {
+g_autoptr(virJSONValue) cache = NULL;
+const char *discardstr = "unmap";
-if (qemuBlockNodeNameValidate(nodename) < 0)
-return NULL;
+if (flags & QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP)
+discardstr = NULL;
-if (qemuBlockStorageSourceGetBlockdevGetCacheProps(src, ) < 0)
-return NULL;
+if (qemuBlockNodeNameValidate(nodename) < 0)
+return NULL;
-if (virJSONValueObjectAdd(,
- "s:node-name", nodename,
- "T:read-only", ro,
- "T:auto-read-only", aro,
- "S:discard", discardstr,
- "A:cache", ,
- NULL) < 0)
-return NULL;
+if (qemuBlockStorageSourceGetBlockdevGetCacheProps(src, ) <
0)
+return NULL;
+
+if (virJSONValueObjectAdd(,
+ "s:node-name", nodename,
+ "T:read-only", ro,
+ "T:auto-read-only", aro,
+ "S:discard", discardstr,
+ "A:cache", ,
+ NULL) < 0)
+return NULL;
+}
}
return g_steal_pointer();
diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h
index 7bb83d8d44..9757108501 100644
--- a/src/qemu/qemu_block.h
+++ b/src/qemu/qemu_block.h
@@ -68,6 +68,7 @@ typedef enum {
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY = 1 << 1,
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY = 1 << 2,
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP = 1 << 3,
+QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_EFFECTIVE_NODE = 1 << 4,
} qemuBlockStorageSourceBackendPropsFlags;
virJSONValue *
--
2.41.0
[PATCH 11/22] qemublocktest: Fix logical bug in TEST_JSON_FORMAT macro
Condition handling failure of the first virTestRun was lacking the 'ret = -1' line thus the subsequent line was taken as it's body rendering the first invocation useless. Signed-off-by: Peter Krempa --- tests/qemublocktest.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index a89dddf002..161fd84871 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -1015,6 +1015,7 @@ mymain(void) xmljsonxmldata.legacy = true; \ if (virTestRun(virTestCounterNext(), testBackingXMLjsonXML, \ ) < 0) \ +ret = -1; \ xmljsonxmldata.legacy = false; \ if (virTestRun(virTestCounterNext(), testBackingXMLjsonXML, \ ) < 0) \ -- 2.41.0
[PATCH 17/22] qemu: block: Use qemuBlockStorageSourceAddBlockdevCommonProps for storage slice
Use the new helper in qemuBlockStorageSourceGetBlockdevStorageSliceProps
to format the common bits.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 17 ++---
.../disk-slices.x86_64-latest.args | 6 +++---
2 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index aeb05a7f00..dc6f34530b 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1484,26 +1484,21 @@ static virJSONValue *
qemuBlockStorageSourceGetBlockdevStorageSliceProps(virStorageSource *src)
{
g_autoptr(virJSONValue) props = NULL;
-g_autoptr(virJSONValue) cache = NULL;
-
-if (qemuBlockNodeNameValidate(src->sliceStorage->nodename) < 0)
-return NULL;
-
-if (qemuBlockStorageSourceGetBlockdevGetCacheProps(src, ) < 0)
-return NULL;
if (virJSONValueObjectAdd(,
"s:driver", "raw",
- "s:node-name", src->sliceStorage->nodename,
"U:offset", src->sliceStorage->offset,
"U:size", src->sliceStorage->size,
"s:file",
qemuBlockStorageSourceGetStorageNodename(src),
- "b:auto-read-only", true,
- "s:discard", "unmap",
- "A:cache", ,
NULL) < 0)
return NULL;
+if (qemuBlockStorageSourceAddBlockdevCommonProps(,
+ src,
+
src->sliceStorage->nodename,
+ false) < 0)
+return NULL;
+
return g_steal_pointer();
}
diff --git a/tests/qemuxml2argvdata/disk-slices.x86_64-latest.args
b/tests/qemuxml2argvdata/disk-slices.x86_64-latest.args
index deaa094379..363f0c0361 100644
--- a/tests/qemuxml2argvdata/disk-slices.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/disk-slices.x86_64-latest.args
@@ -31,14 +31,14 @@
XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
-blockdev
'{"node-name":"libvirt-6-format","read-only":false,"driver":"raw","offset":0,"size":321,"file":"libvirt-6-storage"}'
\
-device
'{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x2","drive":"libvirt-6-format","id":"virtio-disk0","bootindex":1}'
\
-blockdev
'{"driver":"file","filename":"/var/lib/libvirt/images/raw.img","node-name":"libvirt-5-storage","auto-read-only":true,"discard":"unmap"}'
\
--blockdev
'{"driver":"raw","node-name":"libvirt-5-slice-sto","offset":9876,"size":123456789,"file":"libvirt-5-storage","auto-read-only":true,"discard":"unmap"}'
\
+-blockdev
'{"driver":"raw","offset":9876,"size":123456789,"file":"libvirt-5-storage","node-name":"libvirt-5-slice-sto","auto-read-only":true,"discard":"unmap"}'
\
-blockdev
'{"node-name":"libvirt-5-format","read-only":true,"driver":"qcow2","file":"libvirt-5-slice-sto","backing":null}'
\
-blockdev
'{"driver":"file","filename":"/var/lib/libvirt/images/overlay.qcow2","node-name":"libvirt-4-storage","auto-read-only":true,"discard":"unmap"}'
\
-blockdev
'{"node-name":"libvirt-4-format","read-only":false,"driver":"qcow2","file":"libvirt-4-storage","backing":"libvirt-5-format"}'
\
-device
'{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x3","drive":"libvirt-4-format","id":"virtio-disk1"}'
\
-object
'{"qom-type":"secret","id":"libvirt-3-format-encryption-secret0","data":"9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1","keyid":"masterKey0","iv":"AAECAwQFBgcICQoLDA0ODw==","format":"base64"}'
\
-blockdev
'{"driver":"file","filename":"/var/lib/libvirt/images/luks.img","node-name":"libvirt-3-storage","auto-re
[PATCH 00/22] qemu: Refactor blockdev protocol JSON generators ('raw' driver removal part 2)
Second part cleans up tests and various bits and refactors the protocol blockdev JSON configuration generator to be reusable. This still doesn't change the configuration of the blockdevs, just reorders some arguments. Peter Krempa (22): qemuxml2(argv|xml)test: Add network backed disk type='sd' qemu: migration: No longer avoid 'auto-read-only' option for migration qemuBuildDriveSourceStr: Absorb only use of qemuDiskSourceGetProps qemuBlockStorageSourceGetBackendProps: Remove unnecessary indent for non-nbdkit code path conf: Move definition of some disk type enums to a common header virStorageSource: Use proper type for shadow copies of iomode/cachemode/discard/detect_zeroes qemuDomainDiskCachemodeFlags: Simplify usage qemuBlockStorageSourceGetBlockdevGetCacheProps: Return the cache object rather than appending it qemublocktest: Use "target only" mode in 'testJSONtoJSON' and 'testBackingXMLjsonXML' qemublocktest: Drop 'sheepdog' and 'vxhs' test cases qemublocktest: Fix logical bug in TEST_JSON_FORMAT macro qemublocktest: testBackingXMLjsonXML: Drop 'legacy' mode qemuBlockStorageSourceGetBackendProps: Unify cases for '!onlytarget' and '!legacy' virDomainDiskGetDetectZeroesMode: Return proper type qemuBlockStorageSourceGetBackendProps: Unify ordering of fields qemu: block: Add helper to add common properties for -blockdev configuration qemu: block: Use qemuBlockStorageSourceAddBlockdevCommonProps for storage slice qemuBlockStorageSourceGetBackendProps: Introduce QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_EFFECTIVE_NODE qemuBuildHostdevSCSIAttachPrepare: Use "effective node" mode for getting blockdev props qemuBlockStorageSourceGetBackendProps: Use qemuBlockStorageSourceAddBlockdevCommonProps qemuBlockStorageSourceGetBackendProps: Remove unused logic for (auto-)read-only flags qemu: block: Remove unused flags QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_ flags src/conf/domain_conf.c| 2 +- src/conf/domain_conf.h| 39 +- src/conf/storage_source_conf.h| 8 +- src/conf/virconftypes.h | 39 +- src/qemu/qemu_block.c | 349 +- src/qemu/qemu_block.h | 6 +- src/qemu/qemu_command.c | 63 ++-- src/qemu/qemu_domain.c| 33 +- src/qemu/qemu_domain.h| 4 +- src/qemu/qemu_migration.c | 5 +- tests/qemublocktest.c | 41 +- .../jsontojson/curl-libguestfs-out.json | 5 +- .../ssh-passthrough-libguestfs-out.json | 4 +- .../xml2json/dir-fat-cache.json | 6 +- .../file-backing_basic-cache-directsync.json | 24 +- .../file-backing_basic-cache-none.json| 24 +- .../file-backing_basic-cache-unsafe.json | 24 +- .../file-backing_basic-cache-writeback.json | 24 +- ...file-backing_basic-cache-writethrough.json | 24 +- .../xml2json/file-raw-aio_native.json | 6 +- ...work-qcow2-backing-chain-cache-unsafe.json | 12 +- .../blkdeviotune-group-num.x86_64-latest.args | 4 +- ...blkdeviotune-max-length.x86_64-latest.args | 4 +- .../blkdeviotune-max.x86_64-latest.args | 4 +- .../blkdeviotune.x86_64-latest.args | 4 +- .../controller-order.x86_64-latest.args | 2 +- .../disk-aio.x86_64-latest.args | 2 +- .../disk-arm-virtio-sd.aarch64-latest.args| 2 + tests/qemuxml2argvdata/disk-arm-virtio-sd.xml | 16 + .../disk-cache.x86_64-latest.args | 10 +- .../disk-error-policy-s390x.s390x-latest.args | 6 +- .../disk-error-policy.x86_64-latest.args | 6 +- .../disk-metadata-cache.x86_64-latest.args| 6 +- .../disk-network-nfs.x86_64-latest.args | 2 +- ...rk-tlsx509-nbd-hostname.x86_64-latest.args | 2 +- ...disk-network-tlsx509-nbd.x86_64-5.2.0.args | 2 +- ...isk-network-tlsx509-nbd.x86_64-latest.args | 2 +- ...isk-network-tlsx509-vxhs.x86_64-5.0.0.args | 6 +- .../disk-nvme.x86_64-latest.args | 2 +- .../disk-shared.x86_64-latest.args| 6 +- .../disk-slices.x86_64-latest.args| 8 +- .../disk-snapshot.x86_64-latest.args | 4 +- .../disk-transient.x86_64-latest.args | 2 +- .../disk-vhostvdpa.x86_64-latest.args | 2 +- .../name-escape.x86_64-latest.args| 2 +- .../user-aliases.x86_64-latest.args | 4 +- ...eo-bochs-display-device.x86_64-latest.args | 2 +- ...-device-pciaddr-default.x86_64-latest.args | 2 +- ...video-qxl-device-vgamem.x86_64-latest.args | 2 +- .../video-qxl-device.x86_64-latest.args | 2 +- ...o-qxl-sec-device-vgamem.x86_64-latest.args | 2 +- .../video-qxl-sec-device.x86_64-latest.args | 2 +- ...eo-ramfb-display-device.x86_64-latest.args | 2 +- ...video-vga-
[PATCH 12/22] qemublocktest: testBackingXMLjsonXML: Drop 'legacy' mode
Legacy mode used to be needed for use with -drive, which was almost
completely deleted. We now have qemuxml2argvtest test cases checking a
few cases and the rest uses the modern mode only. Thus we don't need to
test the legacy mode any more.
Signed-off-by: Peter Krempa
---
tests/qemublocktest.c | 25 -
1 file changed, 8 insertions(+), 17 deletions(-)
diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
index 161fd84871..b8db1a2570 100644
--- a/tests/qemublocktest.c
+++ b/tests/qemublocktest.c
@@ -61,8 +61,6 @@ testBackingXMLjsonXML(const void *args)
g_auto(virBuffer) debug = VIR_BUFFER_INITIALIZER;
unsigned int backendpropsflags =
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY;
-if (data->legacy)
-backendpropsflags |= QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_LEGACY;
xmlsrc = virStorageSourceNew();
xmlsrc->type = data->type;
@@ -81,17 +79,15 @@ testBackingXMLjsonXML(const void *args)
return -1;
}
-if (!data->legacy) {
-if (testQEMUSchemaValidate(backendprops, data->schemaroot,
- data->schema, false, ) < 0) {
-g_autofree char *debugmsg = virBufferContentAndReset();
-g_autofree char *debugprops = virJSONValueToString(backendprops,
true);
+if (testQEMUSchemaValidate(backendprops, data->schemaroot,
+ data->schema, false, ) < 0) {
+g_autofree char *debugmsg = virBufferContentAndReset();
+g_autofree char *debugprops = virJSONValueToString(backendprops, true);
-VIR_TEST_VERBOSE("json does not conform to QAPI schema");
-VIR_TEST_DEBUG("json:\n%s\ndoes not match schema. Debug output:\n
%s",
- debugprops, NULLSTR(debugmsg));
-return -1;
-}
+VIR_TEST_VERBOSE("json does not conform to QAPI schema");
+VIR_TEST_DEBUG("json:\n%s\ndoes not match schema. Debug output:\n %s",
+ debugprops, NULLSTR(debugmsg));
+return -1;
}
if (virJSONValueObjectAdd(, "a:file", , NULL) < 0)
@@ -1012,11 +1008,6 @@ mymain(void)
do { \
xmljsonxmldata.type = tpe; \
xmljsonxmldata.xml = xmlstr; \
-xmljsonxmldata.legacy = true; \
-if (virTestRun(virTestCounterNext(), testBackingXMLjsonXML, \
- ) < 0) \
-ret = -1; \
-xmljsonxmldata.legacy = false; \
if (virTestRun(virTestCounterNext(), testBackingXMLjsonXML, \
) < 0) \
ret = -1; \
--
2.41.0
[PATCH 06/22] virStorageSource: Use proper type for shadow copies of iomode/cachemode/discard/detect_zeroes
The aforementioned fields in virStorageSource struct are copies of the
disk properties, but were not converted to the proper type yet.
Signed-off-by: Peter Krempa
---
src/conf/storage_source_conf.h | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h
index 2db780611e..5e7d127453 100644
--- a/src/conf/storage_source_conf.h
+++ b/src/conf/storage_source_conf.h
@@ -397,10 +397,10 @@ struct _virStorageSource {
/* Libvirt currently stores the following properties in virDomainDiskDef.
* These instances are currently just copies from the parent definition and
* are not mapped back to the XML */
-int iomode; /* enum virDomainDiskIo */
-int cachemode; /* enum virDomainDiskCache */
-int discard; /* enum virDomainDiskDiscard */
-int detect_zeroes; /* enum virDomainDiskDetectZeroes */
+virDomainDiskIo iomode;
+virDomainDiskCache cachemode;
+virDomainDiskDiscard discard;
+virDomainDiskDetectZeroes detect_zeroes;
virTristateSwitch discard_no_unref;
bool floppyimg; /* set to true if the storage source is going to be used
--
2.41.0
[PATCH 21/22] qemuBlockStorageSourceGetBackendProps: Remove unused logic for (auto-)read-only flags
The code was refactored to format the 'read-only' and 'auto-read-only'
flags via the common helper, so the logic determining their values can
be removed.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 36 +++-
1 file changed, 3 insertions(+), 33 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 5c8d107257..a625e72a5d 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -806,9 +806,7 @@ qemuBlockStorageSourceGetSshProps(virStorageSource *src)
static virJSONValue *
qemuBlockStorageSourceGetFileProps(virStorageSource *src,
- bool onlytarget,
- virTristateBool *autoReadOnly,
- virTristateBool *readOnly)
+ bool onlytarget)
{
const char *path = src->path;
const char *iomode = NULL;
@@ -824,25 +822,8 @@ qemuBlockStorageSourceGetFileProps(virStorageSource *src,
if (src->iomode != VIR_DOMAIN_DISK_IO_DEFAULT)
iomode = virDomainDiskIoTypeToString(src->iomode);
-if (srcpriv && srcpriv->fdpass) {
+if (srcpriv && srcpriv->fdpass)
path = qemuFDPassGetPath(srcpriv->fdpass);
-
-/* when passing a FD to qemu via the /dev/fdset mechanism qemu
- * fetches the appropriate FD from the fdset by checking that it
has
- * the correct accessmode. Now with 'auto-read-only' in effect qemu
- * wants to use a read-only FD first. If the user didn't pass
multiple
- * FDs the feature will not work regardless, so we'll disable it.
*/
-if (src->fdtuple->nfds == 1) {
-*autoReadOnly = VIR_TRISTATE_BOOL_ABSENT;
-
-/* now we setup the normal readonly flag. If user requested
write
- * access honour it */
-if (src->fdtuple->writable)
-*readOnly = VIR_TRISTATE_BOOL_NO;
-else
-*readOnly = virTristateBoolFromBool(src->readonly);
-}
-}
}
ignore_value(virJSONValueObjectAdd(,
@@ -1043,20 +1024,9 @@ qemuBlockStorageSourceGetBackendProps(virStorageSource
*src,
virStorageType actualType = virStorageSourceGetActualType(src);
g_autoptr(virJSONValue) fileprops = NULL;
const char *driver = NULL;
-virTristateBool aro = VIR_TRISTATE_BOOL_ABSENT;
-virTristateBool ro = VIR_TRISTATE_BOOL_ABSENT;
bool onlytarget = flags &
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY;
bool legacy = flags & QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_LEGACY;
-if (flags & QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY) {
-aro = VIR_TRISTATE_BOOL_YES;
-} else {
-if (src->readonly)
-ro = VIR_TRISTATE_BOOL_YES;
-else
-ro = VIR_TRISTATE_BOOL_NO;
-}
-
switch (actualType) {
case VIR_STORAGE_TYPE_BLOCK:
case VIR_STORAGE_TYPE_FILE:
@@ -1069,7 +1039,7 @@ qemuBlockStorageSourceGetBackendProps(virStorageSource
*src,
driver = "file";
}
-if (!(fileprops = qemuBlockStorageSourceGetFileProps(src, onlytarget,
, )))
+if (!(fileprops = qemuBlockStorageSourceGetFileProps(src, onlytarget)))
return NULL;
break;
--
2.41.0
[PATCH 05/22] conf: Move definition of some disk type enums to a common header
Certain disk config fields are mirrored between the disk and storage
source definitions. We nee
Signed-off-by: Peter Krempa
---
src/conf/domain_conf.h | 37 -
src/conf/virconftypes.h | 39 ++-
2 files changed, 38 insertions(+), 38 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 9e6dd930fa..98f99721f0 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -413,17 +413,6 @@ typedef enum {
VIR_DOMAIN_DISK_BUS_LAST
} virDomainDiskBus;
-typedef enum {
-VIR_DOMAIN_DISK_CACHE_DEFAULT,
-VIR_DOMAIN_DISK_CACHE_DISABLE,
-VIR_DOMAIN_DISK_CACHE_WRITETHRU,
-VIR_DOMAIN_DISK_CACHE_WRITEBACK,
-VIR_DOMAIN_DISK_CACHE_DIRECTSYNC,
-VIR_DOMAIN_DISK_CACHE_UNSAFE,
-
-VIR_DOMAIN_DISK_CACHE_LAST
-} virDomainDiskCache;
-
typedef enum {
VIR_DOMAIN_DISK_ERROR_POLICY_DEFAULT,
VIR_DOMAIN_DISK_ERROR_POLICY_STOP,
@@ -451,32 +440,6 @@ typedef enum {
VIR_DOMAIN_DISK_TRANS_LAST
} virDomainDiskGeometryTrans;
-typedef enum {
-VIR_DOMAIN_DISK_IO_DEFAULT = 0,
-VIR_DOMAIN_DISK_IO_NATIVE,
-VIR_DOMAIN_DISK_IO_THREADS,
-VIR_DOMAIN_DISK_IO_URING,
-
-VIR_DOMAIN_DISK_IO_LAST
-} virDomainDiskIo;
-
-typedef enum {
-VIR_DOMAIN_DISK_DISCARD_DEFAULT = 0,
-VIR_DOMAIN_DISK_DISCARD_UNMAP,
-VIR_DOMAIN_DISK_DISCARD_IGNORE,
-
-VIR_DOMAIN_DISK_DISCARD_LAST
-} virDomainDiskDiscard;
-
-typedef enum {
-VIR_DOMAIN_DISK_DETECT_ZEROES_DEFAULT = 0,
-VIR_DOMAIN_DISK_DETECT_ZEROES_OFF,
-VIR_DOMAIN_DISK_DETECT_ZEROES_ON,
-VIR_DOMAIN_DISK_DETECT_ZEROES_UNMAP,
-
-VIR_DOMAIN_DISK_DETECT_ZEROES_LAST
-} virDomainDiskDetectZeroes;
-
typedef enum {
VIR_DOMAIN_DISK_MODEL_DEFAULT = 0,
VIR_DOMAIN_DISK_MODEL_VIRTIO,
diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h
index e07f967814..26cb966194 100644
--- a/src/conf/virconftypes.h
+++ b/src/conf/virconftypes.h
@@ -1,5 +1,5 @@
/*
- * virconftypes.h: struct typedefs to avoid circular inclusion
+ * virconftypes.h: struct and enum type definitions to avoid circular inclusion
*
* Copyright (C) 2006-2019 Red Hat, Inc.
* Copyright (C) 2006-2008 Daniel P. Berrange
@@ -261,3 +261,40 @@ typedef struct _virDomainXMLOption virDomainXMLOption;
typedef struct _virDomainXMLPrivateDataCallbacks
virDomainXMLPrivateDataCallbacks;
typedef struct _virDomainXenbusControllerOpts virDomainXenbusControllerOpts;
+
+typedef enum {
+VIR_DOMAIN_DISK_IO_DEFAULT = 0,
+VIR_DOMAIN_DISK_IO_NATIVE,
+VIR_DOMAIN_DISK_IO_THREADS,
+VIR_DOMAIN_DISK_IO_URING,
+
+VIR_DOMAIN_DISK_IO_LAST
+} virDomainDiskIo;
+
+typedef enum {
+VIR_DOMAIN_DISK_CACHE_DEFAULT = 0,
+VIR_DOMAIN_DISK_CACHE_DISABLE,
+VIR_DOMAIN_DISK_CACHE_WRITETHRU,
+VIR_DOMAIN_DISK_CACHE_WRITEBACK,
+VIR_DOMAIN_DISK_CACHE_DIRECTSYNC,
+VIR_DOMAIN_DISK_CACHE_UNSAFE,
+
+VIR_DOMAIN_DISK_CACHE_LAST
+} virDomainDiskCache;
+
+typedef enum {
+VIR_DOMAIN_DISK_DISCARD_DEFAULT = 0,
+VIR_DOMAIN_DISK_DISCARD_UNMAP,
+VIR_DOMAIN_DISK_DISCARD_IGNORE,
+
+VIR_DOMAIN_DISK_DISCARD_LAST
+} virDomainDiskDiscard;
+
+typedef enum {
+VIR_DOMAIN_DISK_DETECT_ZEROES_DEFAULT = 0,
+VIR_DOMAIN_DISK_DETECT_ZEROES_OFF,
+VIR_DOMAIN_DISK_DETECT_ZEROES_ON,
+VIR_DOMAIN_DISK_DETECT_ZEROES_UNMAP,
+
+VIR_DOMAIN_DISK_DETECT_ZEROES_LAST
+} virDomainDiskDetectZeroes;
--
2.41.0
[PATCH 09/22] qemublocktest: Use "target only" mode in 'testJSONtoJSON' and 'testBackingXMLjsonXML'
Both tests pass a disk source definition which didn't go through the
preparation steps and thus contains only the target information that
were originally present, thus we should be using the
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY flag.
For the same reason QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY
used in 'testJSONtoJSON' doesn't make sense.
Signed-off-by: Peter Krempa
---
tests/qemublocktest.c| 4 ++--
tests/qemublocktestdata/jsontojson/curl-libguestfs-out.json | 5 ++---
.../jsontojson/ssh-passthrough-libguestfs-out.json | 4 +---
3 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
index addd646071..ce76150200 100644
--- a/tests/qemublocktest.c
+++ b/tests/qemublocktest.c
@@ -59,7 +59,7 @@ testBackingXMLjsonXML(const void *args)
g_autoptr(virStorageSource) xmlsrc = NULL;
g_autoptr(virStorageSource) jsonsrc = NULL;
g_auto(virBuffer) debug = VIR_BUFFER_INITIALIZER;
-unsigned int backendpropsflags = 0;
+unsigned int backendpropsflags =
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY;
if (data->legacy)
backendpropsflags |= QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_LEGACY;
@@ -157,7 +157,7 @@ testJSONtoJSON(const void *args)
}
if (!(jsonsrcout = qemuBlockStorageSourceGetBackendProps(src,
-
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY))) {
+
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY))) {
fprintf(stderr, "failed to format disk source json\n");
return -1;
}
diff --git a/tests/qemublocktestdata/jsontojson/curl-libguestfs-out.json
b/tests/qemublocktestdata/jsontojson/curl-libguestfs-out.json
index e130c7bd3c..8cfacb5bc8 100644
--- a/tests/qemublocktestdata/jsontojson/curl-libguestfs-out.json
+++ b/tests/qemublocktestdata/jsontojson/curl-libguestfs-out.json
@@ -2,8 +2,7 @@
"driver": "https",
"url": "https://test.host:443/whatever.img;,
"sslverify": false,
+ "cookie": "some_cookie=\"some_value_or_whatever\"",
"timeout": 2000,
- "readahead": 65536,
- "auto-read-only": true,
- "discard": "unmap"
+ "readahead": 65536
}
diff --git
a/tests/qemublocktestdata/jsontojson/ssh-passthrough-libguestfs-out.json
b/tests/qemublocktestdata/jsontojson/ssh-passthrough-libguestfs-out.json
index 1f6032deb4..7855917e2e 100644
--- a/tests/qemublocktestdata/jsontojson/ssh-passthrough-libguestfs-out.json
+++ b/tests/qemublocktestdata/jsontojson/ssh-passthrough-libguestfs-out.json
@@ -8,7 +8,5 @@
"user": "testuser",
"host-key-check": {
"mode": "none"
- },
- "auto-read-only": true,
- "discard": "unmap"
+ }
}
--
2.41.0
[PATCH 02/22] qemu: migration: No longer avoid 'auto-read-only' option for migration
The 'auto-read-only' blockdev option is available in all supported qemu
versions so we can remove the migration hack which disabled it.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 8 ++--
src/qemu/qemu_block.h | 3 +--
src/qemu/qemu_command.c | 2 +-
src/qemu/qemu_migration.c | 5 +
4 files changed, 5 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 0d252552de..e706bb7369 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1513,14 +1513,10 @@
qemuBlockStorageSourceAttachDataFree(qemuBlockStorageSourceAttachData *data)
*/
qemuBlockStorageSourceAttachData *
qemuBlockStorageSourceAttachPrepareBlockdev(virStorageSource *src,
-virStorageSource *backingStore,
-bool autoreadonly)
+virStorageSource *backingStore)
{
g_autoptr(qemuBlockStorageSourceAttachData) data = NULL;
-unsigned int backendpropsflags = 0;
-
-if (autoreadonly)
-backendpropsflags |=
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY;
+unsigned int backendpropsflags =
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY;
data = g_new0(qemuBlockStorageSourceAttachData, 1);
diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h
index 7008a4e7da..7bb83d8d44 100644
--- a/src/qemu/qemu_block.h
+++ b/src/qemu/qemu_block.h
@@ -134,8 +134,7 @@
G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuBlockStorageSourceAttachData,
qemuBlockStorageSourceAttachData *
qemuBlockStorageSourceAttachPrepareBlockdev(virStorageSource *src,
-virStorageSource *backingStore,
-bool autoreadonly);
+virStorageSource *backingStore);
qemuBlockStorageSourceAttachData *
qemuBlockStorageSourceDetachPrepare(virStorageSource *src);
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d40d3a4e13..ba21976956 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -10937,7 +10937,7 @@
qemuBuildStorageSourceChainAttachPrepareBlockdevOne(qemuBlockStorageSourceChainD
{
g_autoptr(qemuBlockStorageSourceAttachData) elem = NULL;
-if (!(elem = qemuBlockStorageSourceAttachPrepareBlockdev(src,
backingStore, true)))
+if (!(elem = qemuBlockStorageSourceAttachPrepareBlockdev(src,
backingStore)))
return -1;
if (qemuBuildStorageSourceAttachPrepareCommon(src, elem) < 0)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 76da981d08..ac58aa1a8c 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1045,11 +1045,8 @@ qemuMigrationSrcNBDStorageCopyBlockdev(virDomainObj *vm,
tlsAlias, tlsHostname)))
return -1;
-/* Migration via blockdev-mirror was supported sooner than the
auto-read-only
- * feature was added to qemu */
if (!(data = qemuBlockStorageSourceAttachPrepareBlockdev(copysrc,
-
copysrc->backingStore,
- false)))
+
copysrc->backingStore)))
return -1;
if (qemuDomainObjEnterMonitorAsync(vm, VIR_ASYNC_JOB_MIGRATION_OUT) < 0)
--
2.41.0
[PATCH 01/22] qemuxml2(argv|xml)test: Add network backed disk type='sd'
Add a few examples of SD cards backed with network storage to capture
the current state as the formatter code is about to be refactored.
Signed-off-by: Peter Krempa
---
.../disk-arm-virtio-sd.aarch64-latest.args | 2 ++
tests/qemuxml2argvdata/disk-arm-virtio-sd.xml| 16
.../disk-arm-virtio-sd.aarch64-latest.xml| 16
3 files changed, 34 insertions(+)
diff --git a/tests/qemuxml2argvdata/disk-arm-virtio-sd.aarch64-latest.args
b/tests/qemuxml2argvdata/disk-arm-virtio-sd.aarch64-latest.args
index 5d841604fb..00fdd1759e 100644
--- a/tests/qemuxml2argvdata/disk-arm-virtio-sd.aarch64-latest.args
+++ b/tests/qemuxml2argvdata/disk-arm-virtio-sd.aarch64-latest.args
@@ -33,6 +33,8 @@
XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-armtest/.config \
-dtb /arm.dtb \
-usb \
-drive file=/arm-sd.qcow2,format=qcow2,if=sd,index=0 \
+-drive
file.driver=nbd,file.server.type=inet,file.server.host=localhost,file.server.port=10809,file.export=export,format=qcow2,if=sd,index=1
\
+-drive
file.driver=gluster,file.volume=Volume3,file.path=Image.qcow2,file.server.0.type=inet,file.server.0.host=example.org,file.server.0.port=6000,file.server.1.type=inet,file.server.1.host=example.org,file.server.1.port=24007,file.server.2.type=unix,file.server.2.path=/path/to/sock,file.debug=4,format=qcow2,if=sd,index=2
\
-blockdev
'{"driver":"file","filename":"/arm-virtio.qcow2","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}'
\
-blockdev
'{"node-name":"libvirt-1-format","read-only":false,"driver":"qcow2","file":"libvirt-1-storage"}'
\
-device
'{"driver":"virtio-blk-device","drive":"libvirt-1-format","id":"virtio-disk0"}'
\
diff --git a/tests/qemuxml2argvdata/disk-arm-virtio-sd.xml
b/tests/qemuxml2argvdata/disk-arm-virtio-sd.xml
index 45a2192d48..9315c12c76 100644
--- a/tests/qemuxml2argvdata/disk-arm-virtio-sd.xml
+++ b/tests/qemuxml2argvdata/disk-arm-virtio-sd.xml
@@ -26,6 +26,22 @@
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/qemuxml2xmloutdata/disk-arm-virtio-sd.aarch64-latest.xml
b/tests/qemuxml2xmloutdata/disk-arm-virtio-sd.aarch64-latest.xml
index 962dc8f367..b9d412d3fb 100644
--- a/tests/qemuxml2xmloutdata/disk-arm-virtio-sd.aarch64-latest.xml
+++ b/tests/qemuxml2xmloutdata/disk-arm-virtio-sd.aarch64-latest.xml
@@ -30,6 +30,22 @@
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
--
2.41.0
[PATCH 04/22] qemuBlockStorageSourceGetBackendProps: Remove unnecessary indent for non-nbdkit code path
Formatting of the 'nbdkit' driven backend breaks out of the switch
statement so we don't need to have an unnecessary block and indentation
level for the case when nbdkit is not in use.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 132 +-
1 file changed, 66 insertions(+), 66 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index e706bb7369..3be12b47e3 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1028,75 +1028,75 @@ qemuBlockStorageSourceGetBackendProps(virStorageSource
*src,
return NULL;
case VIR_STORAGE_TYPE_NETWORK:
-/* prefer using nbdkit for sources that are supported */
+/* prefer using nbdkit if configured for sources that are supported */
if ((fileprops = qemuBlockStorageSourceGetNbdkitProps(src))) {
driver = "nbd";
break;
-} else {
-switch ((virStorageNetProtocol) src->protocol) {
-case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
-driver = "gluster";
-if (!(fileprops =
qemuBlockStorageSourceGetGlusterProps(src, onlytarget)))
-return NULL;
-break;
-
-case VIR_STORAGE_NET_PROTOCOL_VXHS:
-driver = "vxhs";
-if (!(fileprops = qemuBlockStorageSourceGetVxHSProps(src,
onlytarget)))
-return NULL;
-break;
-
-case VIR_STORAGE_NET_PROTOCOL_HTTP:
-case VIR_STORAGE_NET_PROTOCOL_HTTPS:
-case VIR_STORAGE_NET_PROTOCOL_FTP:
-case VIR_STORAGE_NET_PROTOCOL_FTPS:
-case VIR_STORAGE_NET_PROTOCOL_TFTP:
-driver = virStorageNetProtocolTypeToString(src->protocol);
-if (!(fileprops = qemuBlockStorageSourceGetCURLProps(src,
onlytarget)))
-return NULL;
-break;
-
-case VIR_STORAGE_NET_PROTOCOL_ISCSI:
-driver = "iscsi";
-if (!(fileprops = qemuBlockStorageSourceGetISCSIProps(src,
onlytarget)))
-return NULL;
-break;
-
-case VIR_STORAGE_NET_PROTOCOL_NBD:
-driver = "nbd";
-if (!(fileprops = qemuBlockStorageSourceGetNBDProps(src,
onlytarget)))
-return NULL;
-break;
-
-case VIR_STORAGE_NET_PROTOCOL_RBD:
-driver = "rbd";
-if (!(fileprops = qemuBlockStorageSourceGetRBDProps(src,
onlytarget)))
-return NULL;
-break;
-
-case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
-driver = "sheepdog";
-if (!(fileprops =
qemuBlockStorageSourceGetSheepdogProps(src)))
-return NULL;
-break;
-
-case VIR_STORAGE_NET_PROTOCOL_SSH:
-driver = "ssh";
-if (!(fileprops = qemuBlockStorageSourceGetSshProps(src)))
-return NULL;
-break;
-
-case VIR_STORAGE_NET_PROTOCOL_NFS:
-driver = "nfs";
-if (!(fileprops = qemuBlockStorageSourceGetNFSProps(src)))
-return NULL;
-break;
-
-case VIR_STORAGE_NET_PROTOCOL_NONE:
-case VIR_STORAGE_NET_PROTOCOL_LAST:
-virReportEnumRangeError(virStorageNetProtocol,
src->protocol);
-return NULL;
-}
+}
+
+switch ((virStorageNetProtocol) src->protocol) {
+case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
+driver = "gluster";
+if (!(fileprops = qemuBlockStorageSourceGetGlusterProps(src,
onlytarget)))
+return NULL;
+break;
+
+case VIR_STORAGE_NET_PROTOCOL_VXHS:
+driver = "vxhs";
+if (!(fileprops = qemuBlockStorageSourceGetVxHSProps(src,
onlytarget)))
+return NULL;
+break;
+
+case VIR_STORAGE_NET_PROTOCOL_HTTP:
+case VIR_STORAGE_NET_PROTOCOL_HTTPS:
+case VIR_STORAGE_NET_PROTOCOL_FTP:
+case VIR_STORAGE_NET_PROTOCOL_FTPS:
+case VIR_STORAGE_NET_PROTOCOL_TFTP:
+driver = virStorageNetProtocolTypeToString(src->protocol);
+if (!(fileprops = qemuBlockStorageSourceGetCURLProps(src,
onlytarget)))
+return NULL;
+break;
+
+case VIR_STORAGE_NET_PROTOCOL_ISCSI:
+driver = "iscsi";
+if (!(fileprops = qemuBlockStorageSourceGetISCSIProps(src,
onlytarget)))
+r
[PATCH 03/22] qemuBuildDriveSourceStr: Absorb only use of qemuDiskSourceGetProps
'qemuBuildDriveSourceStr' used to build the legacy -drive commandline
for SD cards is the only user of qemuDiskSourceGetProps. Move the helper
directly inline.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_command.c | 39 +++
1 file changed, 11 insertions(+), 28 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ba21976956..dbef8d0068 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1572,30 +1572,6 @@ qemuDiskBusIsSD(int bus)
}
-/**
- * qemuDiskSourceGetProps:
- * @src: disk source struct
- *
- * Returns the disk source struct wrapped so that it can be used as disk source
- * directly by converting it from json.
- */
-static virJSONValue *
-qemuDiskSourceGetProps(virStorageSource *src)
-{
-g_autoptr(virJSONValue) props = NULL;
-virJSONValue *ret = NULL;
-
-if (!(props = qemuBlockStorageSourceGetBackendProps(src,
-
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_LEGACY)))
-return NULL;
-
-if (virJSONValueObjectAdd(, "a:file", , NULL) < 0)
-return NULL;
-
-return ret;
-}
-
-
static int
qemuBuildDriveSourceStr(virDomainDiskDef *disk,
virBuffer *buf)
@@ -1603,7 +1579,6 @@ qemuBuildDriveSourceStr(virDomainDiskDef *disk,
virStorageType actualType = virStorageSourceGetActualType(disk->src);
qemuDomainStorageSourcePrivate *srcpriv =
QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
qemuDomainSecretInfo **encinfo = NULL;
-g_autoptr(virJSONValue) srcprops = NULL;
bool rawluks = false;
if (srcpriv)
@@ -1624,14 +1599,22 @@ qemuBuildDriveSourceStr(virDomainDiskDef *disk,
virQEMUBuildBufferEscapeComma(buf, disk->src->path);
break;
-case VIR_STORAGE_TYPE_NETWORK:
-if (!(srcprops = qemuDiskSourceGetProps(disk->src)))
+case VIR_STORAGE_TYPE_NETWORK: {
+g_autoptr(virJSONValue) props = NULL;
+g_autoptr(virJSONValue) wrap = NULL;
+
+if (!(props = qemuBlockStorageSourceGetBackendProps(disk->src,
+
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_LEGACY)))
return -1;
-if (virQEMUBuildCommandLineJSON(srcprops, buf, NULL,
+if (virJSONValueObjectAdd(, "a:file", , NULL) < 0)
+return -1;
+
+if (virQEMUBuildCommandLineJSON(wrap, buf, NULL,
virQEMUBuildCommandLineJSONArrayNumbered) < 0)
return -1;
break;
+}
case VIR_STORAGE_TYPE_VOLUME:
case VIR_STORAGE_TYPE_NVME:
--
2.41.0
[PATCH 13/31] qemuDomainGetStatsBlockExportDisk: Use 'storage' node name accessors
In all cases we want to probe stats from the 'storage' layer as we're
interested in the 'threshold' value, which we set there.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f3c9730cd8..e2fe2016af 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17349,7 +17349,7 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef
*disk,
if (QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName) {
frontendalias = QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName;
backendalias = n->nodeformat;
-backendstoragealias = n->nodestorage;
+backendstoragealias = qemuBlockStorageSourceGetStorageNodename(n);
} else {
/* alias may be NULL if the VM is not running */
if (disk->info.alias &&
@@ -17408,7 +17408,7 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef
*disk,
stats) < 0)
return -1;
-if
(qemuDomainGetStatsBlockExportBackendStorage(disk->mirror->nodestorage,
+if
(qemuDomainGetStatsBlockExportBackendStorage(qemuBlockStorageSourceGetStorageNodename(disk->mirror),
stats, *recordnr,
params) < 0)
return -1;
@@ -17437,7 +17437,7 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef
*disk,
stats) < 0)
return -1;
-if
(qemuDomainGetStatsBlockExportBackendStorage(backupdisk->store->nodestorage,
+if
(qemuDomainGetStatsBlockExportBackendStorage(qemuBlockStorageSourceGetStorageNodename(backupdisk->store),
stats,
*recordnr,
params) <
0)
return -1;
--
2.41.0
[PATCH 17/31] qemu: block: Add accessors for storage source effective nodename
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 14 ++
src/qemu/qemu_block.h | 3 +++
2 files changed, 17 insertions(+)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index cba1fb1c1e..10c2c0104b 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -86,6 +86,20 @@ qemuBlockStorageSourceSetFormatNodename(virStorageSource
*src,
}
+/**
+ * qemuBlockStorageSourceGetEffectiveNodename:
+ * @src: virStorageSource to get the effective nodename of
+ *
+ * Gets the nodename that exposes the guest visible data. This function always
+ * returns a name.
+ */
+const char *
+qemuBlockStorageSourceGetEffectiveNodename(virStorageSource *src)
+{
+return src->nodeformat;
+}
+
+
/**
* qemuBlockStorageSourceGetEffectiveStorageNodename:
* @src: virStorageSource to get the effective nodename of
diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h
index 6ed0aa85b2..7008a4e7da 100644
--- a/src/qemu/qemu_block.h
+++ b/src/qemu/qemu_block.h
@@ -43,6 +43,9 @@ qemuBlockStorageSourceGetStorageNodename(virStorageSource
*src);
const char *
qemuBlockStorageSourceGetFormatNodename(virStorageSource *src);
+const char *
+qemuBlockStorageSourceGetEffectiveNodename(virStorageSource *src);
+
typedef struct qemuBlockNodeNameBackingChainData
qemuBlockNodeNameBackingChainData;
struct qemuBlockNodeNameBackingChainData {
--
2.41.0
[PATCH 11/31] qemu: Refactor storage backend attach/detach setup code to use 'storage' nodename accessors
Refactor the code settin up data structures used to attach/detach disks
and SCSI hostdevs.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 7 ---
src/qemu/qemu_command.c | 6 +++---
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 1fc36569a9..7355cb0b5e 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1484,7 +1484,7 @@
qemuBlockStorageSourceAttachPrepareBlockdev(virStorageSource *src,
backendpropsflags)))
return NULL;
-data->storageNodeName = src->nodestorage;
+data->storageNodeName = qemuBlockStorageSourceGetStorageNodename(src);
data->formatNodeName = src->nodeformat;
if (qemuBlockStorageSourceNeedsStorageSliceLayer(src)) {
@@ -1705,7 +1705,7 @@ qemuBlockStorageSourceDetachPrepare(virStorageSource *src)
data->formatNodeName = src->nodeformat;
data->formatAttached = true;
-data->storageNodeName = src->nodestorage;
+data->storageNodeName = qemuBlockStorageSourceGetStorageNodename(src);
data->storageAttached = true;
/* 'raw' format doesn't need the extra 'raw' layer when slicing, thus
@@ -1899,7 +1899,8 @@ qemuBlockStorageSourceDetachOneBlockdev(virDomainObj *vm,
ret = qemuMonitorBlockdevDel(qemuDomainGetMonitor(vm), src->nodeformat);
if (ret == 0)
-ret = qemuMonitorBlockdevDel(qemuDomainGetMonitor(vm),
src->nodestorage);
+ret = qemuMonitorBlockdevDel(qemuDomainGetMonitor(vm),
+
qemuBlockStorageSourceGetStorageNodename(src));
qemuDomainObjExitMonitor(vm);
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 8a7b80719f..40de712c61 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5049,7 +5049,7 @@ qemuBuildHostdevSCSIDetachPrepare(virDomainHostdevDef
*hostdev,
}
srcpriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
-ret->storageNodeName = src->nodestorage;
+ret->storageNodeName = qemuBlockStorageSourceGetStorageNodename(src);
ret->storageAttached = true;
if (srcpriv && srcpriv->secinfo)
@@ -5083,8 +5083,8 @@ qemuBuildHostdevSCSIAttachPrepare(virDomainHostdevDef
*hostdev,
return NULL;
}
-ret->storageNodeName = src->nodestorage;
-*backendAlias = src->nodestorage;
+ret->storageNodeName = qemuBlockStorageSourceGetStorageNodename(src);
+*backendAlias = qemuBlockStorageSourceGetStorageNodename(src);
if (!(ret->storageProps = qemuBlockStorageSourceGetBackendProps(src,
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP)))
--
2.41.0
[PATCH 08/31] qemu: domain: Convert the status XML code for 'storage' nodenames to new accessors
Use the new accessors in the private XML formatters and parsers and the
recovery code.
Specifically in all instances we use the proper (not effective) storage
nodename. In the virStorageSource private data it is what we need to
store. In blockjobs status XML it simply serves us to find the
appropriate 'virStorageSource' struct so using the storage layer node
name is simpler.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_domain.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f891defa91..78dc99d243 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2000,7 +2000,7 @@ qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt,
int enccount;
xmlNodePtr nbdkitnode = NULL;
-src->nodestorage =
virXPathString("string(./nodenames/nodename[@type='storage']/@name)", ctxt);
+qemuBlockStorageSourceSetStorageNodename(src,
virXPathString("string(./nodenames/nodename[@type='storage']/@name)", ctxt));
src->nodeformat =
virXPathString("string(./nodenames/nodename[@type='format']/@name)", ctxt);
src->tlsAlias = virXPathString("string(./objects/TLSx509/@alias)", ctxt);
@@ -2111,7 +2111,7 @@ qemuStorageSourcePrivateDataFormat(virStorageSource *src,
g_auto(virBuffer) objectsChildBuf = VIR_BUFFER_INIT_CHILD(buf);
g_auto(virBuffer) fdsetsChildBuf = VIR_BUFFER_INIT_CHILD(buf);
-virBufferEscapeString(, "\n", src->nodestorage);
+virBufferEscapeString(, "\n", qemuBlockStorageSourceGetStorageNodename(src));
virBufferEscapeString(, "\n", src->nodeformat);
if (src->sliceStorage)
@@ -2288,13 +2288,16 @@ qemuDomainPrivateBlockJobFormatCommit(qemuBlockJobData
*job,
g_auto(virBuffer) disabledBitmapsBuf = VIR_BUFFER_INIT_CHILD(buf);
if (job->data.commit.base)
-virBufferAsprintf(buf, "\n",
job->data.commit.base->nodestorage);
+virBufferAsprintf(buf, "\n",
+
qemuBlockStorageSourceGetStorageNodename(job->data.commit.base));
if (job->data.commit.top)
-virBufferAsprintf(buf, "\n",
job->data.commit.top->nodestorage);
+virBufferAsprintf(buf, "\n",
+
qemuBlockStorageSourceGetStorageNodename(job->data.commit.top));
if (job->data.commit.topparent)
-virBufferAsprintf(buf, "\n",
job->data.commit.topparent->nodestorage);
+virBufferAsprintf(buf, "\n",
+
qemuBlockStorageSourceGetStorageNodename(job->data.commit.topparent));
if (job->data.commit.deleteCommittedImages)
virBufferAddLit(buf, "\n");
@@ -2357,7 +2360,8 @@ qemuDomainObjPrivateXMLFormatBlockjobIterator(void
*payload,
switch ((qemuBlockJobType) job->type) {
case QEMU_BLOCKJOB_TYPE_PULL:
if (job->data.pull.base)
-virBufferAsprintf(, "\n",
job->data.pull.base->nodestorage);
+virBufferAsprintf(, "\n",
+
qemuBlockStorageSourceGetStorageNodename(job->data.pull.base));
break;
case QEMU_BLOCKJOB_TYPE_COMMIT:
@@ -6060,8 +6064,8 @@
qemuDomainDeviceHostdevDefPostParseRestoreBackendAlias(virDomainHostdevDef *host
return -1;
}
-if (!src->nodestorage)
-src->nodestorage = g_strdup_printf("libvirt-%s-backend",
hostdev->info->alias);
+if (!qemuBlockStorageSourceGetStorageNodename(src))
+qemuBlockStorageSourceSetStorageNodename(src,
g_strdup_printf("libvirt-%s-backend", hostdev->info->alias));
return 0;
}
--
2.41.0
[PATCH 26/31] qemu: Use 'format' nodename accessors for block dirty bitmap operations
In most cases the bitmap operations are relevant only on qcow2 images
thus the 'format' layer will be present. Although in certain specific
cases temporary bitmaps can be created on top of other images as well,
thus we use the 'effective' bitmap name in all cases for bitmap
operations.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 27 ++-
src/qemu/qemu_blockjob.c | 4 ++--
src/qemu/qemu_checkpoint.c | 9 +
src/qemu/qemu_driver.c | 7 ---
src/qemu/qemu_snapshot.c | 6 --
5 files changed, 29 insertions(+), 24 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 4c1a711dd3..edc8edcb70 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -2807,7 +2807,8 @@ qemuBlockNamedNodeDataGetBitmapByName(GHashTable
*blockNamedNodeData,
qemuBlockNamedNodeData *nodedata;
size_t i;
-if (!(nodedata = virHashLookup(blockNamedNodeData, src->nodeformat)))
+if (!(nodedata = virHashLookup(blockNamedNodeData,
+
qemuBlockStorageSourceGetEffectiveNodename(src
return NULL;
for (i = 0; i < nodedata->nbitmaps; i++) {
@@ -2863,7 +2864,7 @@ qemuBlockGetBitmapMergeActionsGetBitmaps(virStorageSource
*topsrc,
/* for now it doesn't make sense to consider bitmaps which are not present
* in @topsrc as we can't recreate a bitmap for a layer if it's missing */
-if (!(entry = virHashLookup(blockNamedNodeData, topsrc->nodeformat)))
+if (!(entry = virHashLookup(blockNamedNodeData,
qemuBlockStorageSourceGetEffectiveNodename(topsrc
return NULL;
for (i = 0; i < entry->nbitmaps; i++) {
@@ -2972,7 +2973,7 @@ qemuBlockGetBitmapMergeActions(virStorageSource *topsrc,
granularity = bitmap->granularity;
if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(merge,
- n->nodeformat,
+
qemuBlockStorageSourceGetEffectiveNodename(n),
bitmap->name)
< 0)
return -1;
}
@@ -2982,7 +2983,7 @@ qemuBlockGetBitmapMergeActions(virStorageSource *topsrc,
target,
curbitmap))) {
if (qemuMonitorTransactionBitmapAdd(act,
-target->nodeformat,
+
qemuBlockStorageSourceGetEffectiveNodename(target),
mergebitmapname,
mergebitmappersistent,
mergebitmapdisabled,
@@ -2992,18 +2993,18 @@ qemuBlockGetBitmapMergeActions(virStorageSource *topsrc,
if (writebitmapsrc &&
qemuMonitorTransactionBitmapMergeSourceAddBitmap(merge,
-
writebitmapsrc->nodeformat,
+
qemuBlockStorageSourceGetEffectiveNodename(writebitmapsrc),
"libvirt-tmp-activewrite") < 0)
return -1;
-if (qemuMonitorTransactionBitmapMerge(act, target->nodeformat,
+if (qemuMonitorTransactionBitmapMerge(act,
qemuBlockStorageSourceGetEffectiveNodename(target),
mergebitmapname, ) < 0)
return -1;
}
done:
if (writebitmapsrc &&
-qemuMonitorTransactionBitmapRemove(act, writebitmapsrc->nodeformat,
+qemuMonitorTransactionBitmapRemove(act,
qemuBlockStorageSourceGetEffectiveNodename(writebitmapsrc),
"libvirt-tmp-activewrite") < 0)
return -1;
@@ -3578,8 +3579,8 @@ qemuBlockCommit(virDomainObj *vm,
rc = qemuMonitorBlockCommit(priv->mon,
qemuDomainDiskGetTopNodename(disk),
job->name,
-topSource->nodeformat,
-baseSource->nodeformat,
+
qemuBlockStorageSourceGetEffectiveNodename(topSource),
+
qemuBlockStorageSourceGetEffectiveNodename(baseSource),
backingPath, bandwidth,
autofinalize);
@@ -3663,7 +3664,7 @@ qemuBlockPivot(virDomainObj *vm,
bitmapactions = virJSONValueNewArray();
if (qemuMonitorTransactionBitmapAdd(bitmapactions,
-disk->mirror->nodeformat,
+
qemuBlockStorageSourceGetE
[PATCH 21/31] qemu: block: Use 'format' nodename accessors in '-blockdev' setup code
Convert the main -blockdev JSON object setup code to use the new
accessors. In these we use mainly the real 'format' layer node name.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 0fa5b6e55d..852028f014 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1282,7 +1282,7 @@
qemuBlockStorageSourceGetBlockdevFormatCommonProps(virStorageSource *src)
src->detect_zeroes);
g_autoptr(virJSONValue) props = NULL;
-if (qemuBlockNodeNameValidate(src->nodeformat) < 0)
+if
(qemuBlockNodeNameValidate(qemuBlockStorageSourceGetFormatNodename(src)) < 0)
return NULL;
if (src->discard)
@@ -1296,7 +1296,7 @@
qemuBlockStorageSourceGetBlockdevFormatCommonProps(virStorageSource *src)
*/
if (virJSONValueObjectAdd(,
- "s:node-name", src->nodeformat,
+ "s:node-name",
qemuBlockStorageSourceGetFormatNodename(src),
"b:read-only", src->readonly,
"S:discard", discard,
"S:detect-zeroes", detectZeroes,
@@ -1530,7 +1530,7 @@
qemuBlockStorageSourceAttachPrepareBlockdev(virStorageSource *src,
return NULL;
data->storageNodeName = qemuBlockStorageSourceGetStorageNodename(src);
-data->formatNodeName = src->nodeformat;
+data->formatNodeName = qemuBlockStorageSourceGetFormatNodename(src);
if (qemuBlockStorageSourceNeedsStorageSliceLayer(src)) {
if (!(data->storageSliceProps =
qemuBlockStorageSourceGetBlockdevStorageSliceProps(src)))
@@ -1748,7 +1748,7 @@ qemuBlockStorageSourceDetachPrepare(virStorageSource *src)
data = g_new0(qemuBlockStorageSourceAttachData, 1);
-data->formatNodeName = src->nodeformat;
+data->formatNodeName = qemuBlockStorageSourceGetFormatNodename(src);
data->formatAttached = true;
data->storageNodeName = qemuBlockStorageSourceGetStorageNodename(src);
data->storageAttached = true;
@@ -1941,7 +1941,8 @@ qemuBlockStorageSourceDetachOneBlockdev(virDomainObj *vm,
if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0)
return -1;
-ret = qemuMonitorBlockdevDel(qemuDomainGetMonitor(vm), src->nodeformat);
+ret = qemuMonitorBlockdevDel(qemuDomainGetMonitor(vm),
+ qemuBlockStorageSourceGetFormatNodename(src));
if (ret == 0)
ret = qemuMonitorBlockdevDel(qemuDomainGetMonitor(vm),
@@ -1959,8 +1960,8 @@ qemuBlockSnapshotAddBlockdev(virJSONValue *actions,
virStorageSource *newsrc)
{
return qemuMonitorTransactionSnapshotBlockdev(actions,
- disk->src->nodeformat,
- newsrc->nodeformat);
+
qemuBlockStorageSourceGetEffectiveNodename(disk->src),
+
qemuBlockStorageSourceGetFormatNodename(newsrc));
}
--
2.41.0
[PATCH 23/31] tests: Use 'format' layer nodename accessors in test code
The test code cares mostly about the actual layer nodenames thus,
appropriate accessors are used.
Signed-off-by: Peter Krempa
---
tests/qemublocktest.c | 10 +-
tests/qemumonitorjsontest.c | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
index 9a72a67ce2..addd646071 100644
--- a/tests/qemublocktest.c
+++ b/tests/qemublocktest.c
@@ -241,7 +241,7 @@ testQemuDiskXMLToJSONFakeSecrets(virStorageSource *src)
srcpriv->encinfo[0] = g_new0(qemuDomainSecretInfo, 1);
srcpriv->encinfo[0]->alias = g_strdup_printf("%s-encalias",
- NULLSTR(src->nodeformat));
+
qemuBlockStorageSourceGetFormatNodename(src));
srcpriv->enccount = 1;
}
@@ -653,7 +653,7 @@ testQemuBitmapListPrint(const char *title,
for (; next; next = next->next) {
virStorageSource *src = next->data;
-virBufferAsprintf(buf, "%s\n", src->nodeformat);
+virBufferAsprintf(buf, "%s\n",
qemuBlockStorageSourceGetFormatNodename(src));
}
}
@@ -668,7 +668,7 @@ testQemuBackupIncrementalBitmapCalculateGetFakeImage(size_t
idx)
ret->format = VIR_STORAGE_FILE_QCOW2;
ret->path = g_strdup_printf("/image%zu", idx);
qemuBlockStorageSourceSetStorageNodename(ret,
g_strdup_printf("libvirt-%zu-storage", idx));
- ret->nodeformat = g_strdup_printf("libvirt-%zu-format", idx);
+ qemuBlockStorageSourceSetFormatNodename(ret,
g_strdup_printf("libvirt-%zu-format", idx));
return ret;
}
@@ -741,7 +741,7 @@ testQemuBackupIncrementalBitmapCalculate(const void *opaque)
}
target = virStorageSourceNew();
-target->nodeformat = g_strdup_printf("target_node");
+qemuBlockStorageSourceSetFormatNodename(target,
g_strdup_printf("target_node"));
if (qemuBackupDiskPrepareOneBitmapsChain(data->chain,
target,
@@ -872,7 +872,7 @@ testQemuBlockBitmapBlockcopy(const void *opaque)
g_autoptr(virStorageSource) fakemirror = virStorageSourceNew();
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
-fakemirror->nodeformat = g_strdup("mirror-format-node");
+qemuBlockStorageSourceSetFormatNodename(fakemirror,
g_strdup("mirror-format-node"));
expectpath = g_strdup_printf("%s/%s%s-out.json", abs_srcdir,
blockcopyPrefix, data->name);
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 6b82e3841b..6293b416bd 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -2600,7 +2600,7 @@ testQemuMonitorJSONBlockdevReopen(const void *opaque)
src->format = VIR_STORAGE_FILE_QCOW2;
src->readonly = true;
-src->nodeformat = g_strdup("test node");
+qemuBlockStorageSourceSetFormatNodename(src, g_strdup("test node"));
qemuBlockStorageSourceSetStorageNodename(src, g_strdup("backing
nodename"));
src->backingStore = virStorageSourceNew();
--
2.41.0
[PATCH 14/31] qemuDomainSetBlockThreshold: Use 'storage' node name accessor
We need to keep setting the block threshold on the real storage layer per semantics of the API. Use the appropriate accessor. Signed-off-by: Peter Krempa --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e2fe2016af..6d95711cbc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18682,7 +18682,7 @@ qemuDomainSetBlockThreshold(virDomainPtr dom, goto endjob; } -nodename = g_strdup(src->nodestorage); +nodename = g_strdup(qemuBlockStorageSourceGetStorageNodename(src)); qemuDomainObjEnterMonitor(vm); rc = qemuMonitorSetBlockThreshold(priv->mon, nodename, threshold); -- 2.41.0
[PATCH 20/31] qemu: blockjob: Use 'format' nodename accessors for job naming
Use the effective nodename for naming the job as we use that one now.
It doesn't matter too much which one we pick, because it's used just for
the name of the job, which we preserve in the status XML.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_blockjob.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 12f3d59df5..cb9948ae2a 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -252,7 +252,8 @@ qemuBlockJobDiskNewPull(virDomainObj *vm,
unsigned int jobflags)
{
g_autoptr(qemuBlockJobData) job = NULL;
-g_autofree char *jobname = g_strdup_printf("pull-%s-%s", disk->dst,
disk->src->nodeformat);
+g_autofree char *jobname = g_strdup_printf("pull-%s-%s", disk->dst,
+
qemuBlockStorageSourceGetEffectiveNodename(disk->src));
if (!(job = qemuBlockJobDataNew(QEMU_BLOCKJOB_TYPE_PULL, jobname)))
return NULL;
@@ -278,7 +279,8 @@ qemuBlockJobDiskNewCommit(virDomainObj *vm,
unsigned int jobflags)
{
g_autoptr(qemuBlockJobData) job = NULL;
-g_autofree char *jobname = g_strdup_printf("commit-%s-%s", disk->dst,
top->nodeformat);
+g_autofree char *jobname = g_strdup_printf("commit-%s-%s", disk->dst,
+
qemuBlockStorageSourceGetEffectiveNodename(top));
qemuBlockJobType jobtype = QEMU_BLOCKJOB_TYPE_COMMIT;
if (topparent == NULL)
@@ -309,7 +311,7 @@ qemuBlockJobNewCreate(virDomainObj *vm,
{
g_autoptr(qemuBlockJobData) job = NULL;
g_autofree char *jobname = NULL;
-const char *nodename = src->nodeformat;
+const char *nodename = qemuBlockStorageSourceGetEffectiveNodename(src);
if (storage)
nodename = qemuBlockStorageSourceGetStorageNodename(src);
@@ -340,7 +342,8 @@ qemuBlockJobDiskNewCopy(virDomainObj *vm,
unsigned int jobflags)
{
g_autoptr(qemuBlockJobData) job = NULL;
-g_autofree char *jobname = g_strdup_printf("copy-%s-%s", disk->dst,
disk->src->nodeformat);
+g_autofree char *jobname = g_strdup_printf("copy-%s-%s", disk->dst,
+
qemuBlockStorageSourceGetEffectiveNodename(disk->src));
if (!(job = qemuBlockJobDataNew(QEMU_BLOCKJOB_TYPE_COPY, jobname)))
return NULL;
@@ -368,7 +371,8 @@ qemuBlockJobDiskNewBackup(virDomainObj *vm,
g_autoptr(qemuBlockJobData) job = NULL;
g_autofree char *jobname = NULL;
-jobname = g_strdup_printf("backup-%s-%s", disk->dst,
disk->src->nodeformat);
+jobname = g_strdup_printf("backup-%s-%s", disk->dst,
+
qemuBlockStorageSourceGetEffectiveNodename(disk->src));
if (!(job = qemuBlockJobDataNew(QEMU_BLOCKJOB_TYPE_BACKUP, jobname)))
return NULL;
--
2.41.0
[PATCH 19/31] qemu: backup: Use format nodename accessors
Both modified cases in this patch require the effective nodename as they deal with the data being backed up. Signed-off-by: Peter Krempa --- src/qemu/qemu_backup.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index e4db967e2c..857709b17e 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -335,9 +335,9 @@ qemuBackupDiskPrepareDataOnePush(virJSONValue *actions, syncmode = QEMU_MONITOR_TRANSACTION_BACKUP_SYNC_MODE_INCREMENTAL; if (qemuMonitorTransactionBackup(actions, - dd->domdisk->src->nodeformat, + qemuBlockStorageSourceGetEffectiveNodename(dd->domdisk->src), dd->blockjob->name, - dd->store->nodeformat, + qemuBlockStorageSourceGetEffectiveNodename(dd->store), dd->incrementalBitmap, syncmode) < 0) return -1; @@ -355,9 +355,9 @@ qemuBackupDiskPrepareDataOnePull(virJSONValue *actions, dd->backupdisk->exportbitmap = g_strdup(dd->incrementalBitmap); if (qemuMonitorTransactionBackup(actions, - dd->domdisk->src->nodeformat, + qemuBlockStorageSourceGetEffectiveNodename(dd->domdisk->src), dd->blockjob->name, - dd->store->nodeformat, + qemuBlockStorageSourceGetEffectiveNodename(dd->store), NULL, QEMU_MONITOR_TRANSACTION_BACKUP_SYNC_MODE_NONE) < 0) return -1; -- 2.41.0
[PATCH 25/31] qemu: driver: Convert disk stats code to use 'format' nodename accessors
I case of statistics we're interested in the statistics of the effective
bitmap whatever it happens to be.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6d95711cbc..2d1d0bb3e2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9368,7 +9368,7 @@ qemuDomainBlocksStatsGather(virDomainObj *vm,
/* capacity are reported only per node-name so we need to transfer
them */
if (disk && disk->src &&
-(capstats = virHashLookup(blockstats, disk->src->nodeformat))) {
+(capstats = virHashLookup(blockstats,
qemuBlockStorageSourceGetEffectiveNodename(disk->src {
(*retstats)->capacity = capstats->capacity;
(*retstats)->physical = capstats->physical;
(*retstats)->wr_highest_offset = capstats->wr_highest_offset;
@@ -17348,7 +17348,7 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef
*disk,
if (QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName) {
frontendalias = QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName;
-backendalias = n->nodeformat;
+backendalias = qemuBlockStorageSourceGetEffectiveNodename(n);
backendstoragealias = qemuBlockStorageSourceGetStorageNodename(n);
} else {
/* alias may be NULL if the VM is not running */
@@ -17402,7 +17402,7 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef
*disk,
return -1;
if (qemuDomainGetStatsOneBlock(driver, cfg, dom, params,
- disk->mirror->nodeformat,
+
qemuBlockStorageSourceGetEffectiveNodename(disk->mirror),
disk->mirror,
*recordnr,
stats) < 0)
@@ -17431,7 +17431,7 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef
*disk,
return -1;
if (qemuDomainGetStatsOneBlock(driver, cfg, dom, params,
-
backupdisk->store->nodeformat,
+
qemuBlockStorageSourceGetEffectiveNodename(backupdisk->store),
backupdisk->store,
*recordnr,
stats) < 0)
--
2.41.0
[PATCH 24/31] qemu: Convert disk backend setup code to use 'format' nodename accessors
The disk backend setup code is concerned only about the effective
nodename. Doing this conversion will also simplify further changes
needed to drop the 'raw' layer in cases when it's not really needed.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 6 +++---
src/qemu/qemu_domain.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 852028f014..4c1a711dd3 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1980,7 +1980,7 @@ qemuBlockStorageGetCopyOnReadProps(virDomainDiskDef *disk)
ignore_value(virJSONValueObjectAdd(,
"s:driver", "copy-on-read",
"s:node-name", priv->nodeCopyOnRead,
- "s:file", disk->src->nodeformat,
+ "s:file",
qemuBlockStorageSourceGetEffectiveNodename(disk->src),
"s:discard", "unmap",
NULL));
@@ -2735,10 +2735,10 @@ qemuBlockStorageSourceCreateDetectSize(GHashTable
*blockNamedNodeData,
{
qemuBlockNamedNodeData *entry;
-if (!(entry = virHashLookup(blockNamedNodeData, templ->nodeformat))) {
+if (!(entry = virHashLookup(blockNamedNodeData,
qemuBlockStorageSourceGetEffectiveNodename(templ {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to update capacity data for block node
'%1$s'"),
- templ->nodeformat);
+ qemuBlockStorageSourceGetEffectiveNodename(templ));
return -1;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index d8d3a17e55..995aa3f79c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7891,7 +7891,7 @@ qemuDomainDiskGetTopNodename(virDomainDiskDef *disk)
if (disk->copy_on_read == VIR_TRISTATE_SWITCH_ON)
return priv->nodeCopyOnRead;
-return disk->src->nodeformat;
+return qemuBlockStorageSourceGetEffectiveNodename(disk->src);
}
--
2.41.0
[PATCH 31/31] conf: Rename 'nodeformat' field of virStorageSource to 'nodenameformat'
While the name itself doesn't matter, this rename is done to prove that
all places using 'nodeformat' were converted to the appropriate
accessors.
Signed-off-by: Peter Krempa
---
src/conf/storage_source_conf.c | 4 ++--
src/conf/storage_source_conf.h | 2 +-
src/qemu/qemu_block.c | 8
src/qemu/qemu_blockjob.c | 2 +-
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c
index ea171f9945..f974a521b1 100644
--- a/src/conf/storage_source_conf.c
+++ b/src/conf/storage_source_conf.c
@@ -828,7 +828,7 @@ virStorageSourceCopy(const virStorageSource *src,
def->backingStoreRawFormat = src->backingStoreRawFormat;
def->snapshot = g_strdup(src->snapshot);
def->configFile = g_strdup(src->configFile);
-def->nodeformat = g_strdup(src->nodeformat);
+def->nodenameformat = g_strdup(src->nodenameformat);
def->nodenamestorage = g_strdup(src->nodenamestorage);
def->compat = g_strdup(src->compat);
def->tlsAlias = g_strdup(src->tlsAlias);
@@ -1169,7 +1169,7 @@ virStorageSourceClear(virStorageSource *def)
virObjectUnref(def->privateData);
VIR_FREE(def->nodenamestorage);
-VIR_FREE(def->nodeformat);
+VIR_FREE(def->nodenameformat);
virStorageSourceBackingStoreClear(def);
diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h
index e9525c8f65..5a9b03b610 100644
--- a/src/conf/storage_source_conf.h
+++ b/src/conf/storage_source_conf.h
@@ -370,7 +370,7 @@ struct _virStorageSource {
virStorageFileFormat backingStoreRawFormat;
/* metadata that allows identifying given storage source */
-char *nodeformat; /* name of the format handler object */
+char *nodenameformat; /* name of the format handler object */
char *nodenamestorage; /* name of the storage object */
/* An optional setting to enable usage of TLS for the storage source */
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index fa31028db3..0d252552de 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -81,8 +81,8 @@ void
qemuBlockStorageSourceSetFormatNodename(virStorageSource *src,
char *nodename)
{
-g_free(src->nodeformat);
-src->nodeformat = nodename;
+g_free(src->nodenameformat);
+src->nodenameformat = nodename;
}
@@ -96,7 +96,7 @@ qemuBlockStorageSourceSetFormatNodename(virStorageSource *src,
const char *
qemuBlockStorageSourceGetEffectiveNodename(virStorageSource *src)
{
-return src->nodeformat;
+return src->nodenameformat;
}
@@ -141,7 +141,7 @@ qemuBlockStorageSourceGetStorageNodename(virStorageSource
*src)
const char *
qemuBlockStorageSourceGetFormatNodename(virStorageSource *src)
{
-return src->nodeformat;
+return src->nodenameformat;
}
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 0913a224e3..4b5b63d287 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -617,7 +617,7 @@ qemuBlockJobCleanStorageSourceRuntime(virStorageSource *src)
VIR_FREE(src->relPath);
VIR_FREE(src->backingStoreRaw);
VIR_FREE(src->nodenamestorage);
-VIR_FREE(src->nodeformat);
+VIR_FREE(src->nodenameformat);
VIR_FREE(src->tlsAlias);
VIR_FREE(src->tlsCertdir);
}
--
2.41.0
[PATCH 07/31] qemu: block: Use proper accessors for image formatting/creation code
Use 'qemuBlockStorageSourceGetEffectiveStorageNodename' in all the JSON
props formatters for setting up a 'blockdev-create' job of a format
layer.
In case of the blockjob name designator we're okay to use just the
storage layer nodename as that serves only to find the appropriate
entry.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c| 10 +-
src/qemu/qemu_blockjob.c | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 0c9460f678..a98caa330e 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -2078,7 +2078,7 @@
qemuBlockStorageSourceCreateGetFormatPropsGeneric(virStorageSource *src,
if (virJSONValueObjectAdd(,
"s:driver", driver,
- "s:file", src->nodestorage,
+ "s:file",
qemuBlockStorageSourceGetEffectiveStorageNodename(src),
"U:size", src->capacity,
NULL) < 0)
return -1;
@@ -2143,7 +2143,7 @@
qemuBlockStorageSourceCreateGetFormatPropsLUKS(virStorageSource *src,
if (virJSONValueObjectAdd(,
"s:driver", "luks",
- "s:file", src->nodestorage,
+ "s:file",
qemuBlockStorageSourceGetEffectiveStorageNodename(src),
"U:size", src->capacity,
NULL) < 0)
return -1;
@@ -2200,7 +2200,7 @@
qemuBlockStorageSourceCreateGetFormatPropsQcow2(virStorageSource *src,
if (virJSONValueObjectAdd(,
"s:driver", "qcow2",
- "s:file", src->nodestorage,
+ "s:file",
qemuBlockStorageSourceGetEffectiveStorageNodename(src),
"U:size", src->capacity,
"S:version", qcow2version,
"P:cluster-size", src->clusterSize,
@@ -2226,7 +2226,7 @@
qemuBlockStorageSourceCreateGetFormatPropsQcow(virStorageSource *src,
if (virJSONValueObjectAdd(,
"s:driver", "qcow",
- "s:file", src->nodestorage,
+ "s:file",
qemuBlockStorageSourceGetEffectiveStorageNodename(src),
"U:size", src->capacity,
NULL) < 0)
return -1;
@@ -2249,7 +2249,7 @@
qemuBlockStorageSourceCreateGetFormatPropsQed(virStorageSource *src,
if (virJSONValueObjectAdd(,
"s:driver", "qed",
- "s:file", src->nodestorage,
+ "s:file",
qemuBlockStorageSourceGetEffectiveStorageNodename(src),
"U:size", src->capacity,
NULL) < 0)
return -1;
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index f1d22df59f..25ac74d6c4 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -312,7 +312,7 @@ qemuBlockJobNewCreate(virDomainObj *vm,
const char *nodename = src->nodeformat;
if (storage)
-nodename = src->nodestorage;
+nodename = qemuBlockStorageSourceGetStorageNodename(src);
jobname = g_strdup_printf("create-%s", nodename);
--
2.41.0
[PATCH 29/31] qemu: migration: Use 'format' nodename accessors in dirty bitmap migration
The persistent bitmaps are stored in the format layer, using 'effective' bitmap name is the most reasonable approach in this case. Signed-off-by: Peter Krempa --- src/qemu/qemu_migration.c| 11 ++- src/qemu/qemu_migration_cookie.c | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 949ef6d6d5..76da981d08 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2393,7 +2393,8 @@ qemuMigrationSrcBeginPhaseBlockDirtyBitmaps(qemuMigrationCookie *mig, qemuMigrationBlockDirtyBitmapsDisk *disk; GSList *bitmaps = NULL; virDomainDiskDef *diskdef = vm->def->disks[i]; -qemuBlockNamedNodeData *nodedata = virHashLookup(blockNamedNodeData, diskdef->src->nodeformat); +qemuBlockNamedNodeData *nodedata = virHashLookup(blockNamedNodeData, + qemuBlockStorageSourceGetEffectiveNodename(diskdef->src)); size_t j; if (!nodedata) @@ -4456,7 +4457,7 @@ qemuMigrationSrcRunPrepareBlockDirtyBitmapsMerge(virDomainObj *vm, granularity = b->granularity; if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(merge, - n->nodeformat, + qemuBlockStorageSourceGetEffectiveNodename(n), b->name) < 0) return -1; } @@ -4465,19 +4466,19 @@ qemuMigrationSrcRunPrepareBlockDirtyBitmapsMerge(virDomainObj *vm, bitmap->persistent = VIR_TRISTATE_BOOL_YES; if (qemuMonitorTransactionBitmapAdd(actions, -disk->disk->src->nodeformat, + qemuBlockStorageSourceGetEffectiveNodename(disk->disk->src), bitmap->sourcebitmap, false, false, granularity) < 0) return -1; if (qemuMonitorTransactionBitmapMerge(actions, - disk->disk->src->nodeformat, + qemuBlockStorageSourceGetEffectiveNodename(disk->disk->src), bitmap->sourcebitmap, ) < 0) return -1; tmpbmp = g_new0(qemuDomainJobPrivateMigrateTempBitmap, 1); -tmpbmp->nodename = g_strdup(disk->disk->src->nodeformat); +tmpbmp->nodename = g_strdup(qemuBlockStorageSourceGetEffectiveNodename(disk->disk->src)); tmpbmp->bitmapname = g_strdup(bitmap->sourcebitmap); tmpbitmaps = g_slist_prepend(tmpbitmaps, tmpbmp); } diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index ba146960d5..5505fdaf22 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -1573,7 +1573,7 @@ qemuMigrationCookieBlockDirtyBitmapsMatchDisks(virDomainDef *def, return -1; } -disk->nodename = disk->disk->src->nodeformat; +disk->nodename = qemuBlockStorageSourceGetEffectiveNodename(disk->disk->src); } return 0; -- 2.41.0
[PATCH 18/31] qemuBlockStorageSourceGetFormatProps: Use new frontend name accessor
---
src/qemu/qemu_block.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 10c2c0104b..0fa5b6e55d 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1418,7 +1418,7 @@ qemuBlockStorageSourceGetFormatProps(virStorageSource
*src,
src->format >= VIR_STORAGE_FILE_BACKING) {
if (virStorageSourceIsBacking(backingStore)) {
backingFormatterStr = "s:backing";
-backingNodename = backingStore->nodeformat;
+backingNodename =
qemuBlockStorageSourceGetEffectiveNodename(backingStore);
} else {
/* chain is terminated, indicate that no detection should happen
in qemu */
backingFormatterStr = "n:backing";
--
2.41.0
[PATCH 28/31] qemu: Convert migration setup code to use 'format' layer node name accessors
The blockjob, NBD export and setup of the cookie data all care about the
effective nodename.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c| 6 +++---
src/qemu/qemu_migration.c| 4 ++--
src/qemu/qemu_migration_cookie.c | 3 ++-
3 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index edc8edcb70..fa31028db3 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -3406,11 +3406,11 @@ qemuBlockExportAddNBD(virDomainObj *vm,
const char *bitmaps[2] = { bitmap, NULL };
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCK_EXPORT_ADD))
-return qemuMonitorNBDServerAdd(priv->mon, src->nodeformat,
+return qemuMonitorNBDServerAdd(priv->mon,
qemuBlockStorageSourceGetEffectiveNodename(src),
exportname, writable, bitmap);
-if (!(nbdprops = qemuBlockExportGetNBDProps(src->nodeformat, exportname,
-writable, bitmaps)))
+if (!(nbdprops =
qemuBlockExportGetNBDProps(qemuBlockStorageSourceGetEffectiveNodename(src),
+exportname, writable,
bitmaps)))
return -1;
return qemuMonitorBlockExportAdd(priv->mon, );
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 7d08df1fc5..949ef6d6d5 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1012,7 +1012,7 @@
qemuMigrationSrcNBDStorageCopyBlockdevPrepareSource(virDomainDiskDef *disk,
copysrc->tlsHostname = g_strdup(tlsHostname);
qemuBlockStorageSourceSetStorageNodename(copysrc,
g_strdup_printf("migration-%s-storage", disk->dst));
-copysrc->nodeformat = g_strdup_printf("migration-%s-format", disk->dst);
+qemuBlockStorageSourceSetFormatNodename(copysrc,
g_strdup_printf("migration-%s-format", disk->dst));
return g_steal_pointer();
}
@@ -1060,7 +1060,7 @@ qemuMigrationSrcNBDStorageCopyBlockdev(virDomainObj *vm,
if (mon_ret == 0)
mon_ret = qemuMonitorBlockdevMirror(qemuDomainGetMonitor(vm),
diskAlias, true,
qemuDomainDiskGetTopNodename(disk),
-copysrc->nodeformat,
+
qemuBlockStorageSourceGetEffectiveNodename(copysrc),
mirror_speed, 0, 0, mirror_shallow,
syncWrites);
diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c
index 7f0b7a3412..ba146960d5 100644
--- a/src/qemu/qemu_migration_cookie.c
+++ b/src/qemu/qemu_migration_cookie.c
@@ -31,6 +31,7 @@
#include "qemu_domain.h"
#include "qemu_migration_cookie.h"
#include "qemu_migration_params.h"
+#include "qemu_block.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -507,7 +508,7 @@ qemuMigrationCookieAddNBD(qemuMigrationCookie *mig,
virDomainDiskDef *disk = vm->def->disks[i];
qemuBlockStats *entry;
-if (!(entry = virHashLookup(stats, disk->src->nodeformat)))
+if (!(entry = virHashLookup(stats,
qemuBlockStorageSourceGetEffectiveNodename(disk->src
continue;
mig->nbd->disks[mig->nbd->ndisks].target = g_strdup(disk->dst);
--
2.41.0
[PATCH 27/31] qemu: command: Use 'format' nodename accessors for 'pflash' backend setup
The frontend device needs to access the blocks directly so it cares
about the effective nodename.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_command.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 40de712c61..d40d3a4e13 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7032,9 +7032,11 @@ qemuBuildMachineCommandLine(virCommand *cmd,
if (virDomainDefHasOldStyleUEFI(def)) {
if (priv->pflash0)
-virBufferAsprintf(, ",pflash0=%s", priv->pflash0->nodeformat);
+virBufferAsprintf(, ",pflash0=%s",
+
qemuBlockStorageSourceGetEffectiveNodename(priv->pflash0));
if (def->os.loader->nvram)
-virBufferAsprintf(, ",pflash1=%s",
def->os.loader->nvram->nodeformat);
+virBufferAsprintf(, ",pflash1=%s",
+
qemuBlockStorageSourceGetEffectiveNodename(def->os.loader->nvram));
}
if (virDomainNumaHasHMAT(def->numa))
--
2.41.0
[PATCH 30/31] qemu: driver: Use 'format' nodename accessors for disk resize
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b286d94ca1..43d96739d5 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9252,7 +9252,7 @@ qemuDomainBlockResize(virDomainPtr dom,
}
if (!qemuDiskBusIsSD(disk->bus)) {
-nodename = disk->src->nodeformat;
+nodename = qemuBlockStorageSourceGetEffectiveNodename(disk->src);
} else {
if (!(device = qemuAliasDiskDriveFromDisk(disk)))
goto endjob;
--
2.41.0
[PATCH 22/31] qemu: domain: Use 'format' layer node name accessors for nodename setup code
The code setting the nodenames needs to use the 'true' nodename of the
format layer.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_domain.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 1b2f09a316..d8d3a17e55 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2001,7 +2001,7 @@ qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt,
xmlNodePtr nbdkitnode = NULL;
qemuBlockStorageSourceSetStorageNodename(src,
virXPathString("string(./nodenames/nodename[@type='storage']/@name)", ctxt));
-src->nodeformat =
virXPathString("string(./nodenames/nodename[@type='format']/@name)", ctxt);
+qemuBlockStorageSourceSetFormatNodename(src,
virXPathString("string(./nodenames/nodename[@type='format']/@name)", ctxt));
src->tlsAlias = virXPathString("string(./objects/TLSx509/@alias)", ctxt);
if (src->sliceStorage)
@@ -2112,7 +2112,7 @@ qemuStorageSourcePrivateDataFormat(virStorageSource *src,
g_auto(virBuffer) fdsetsChildBuf = VIR_BUFFER_INIT_CHILD(buf);
virBufferEscapeString(, "\n", qemuBlockStorageSourceGetStorageNodename(src));
-virBufferEscapeString(, "\n", src->nodeformat);
+virBufferEscapeString(, "\n", qemuBlockStorageSourceGetFormatNodename(src));
if (src->sliceStorage)
virBufferEscapeString(, "\n",
@@ -2817,8 +2817,9 @@ qemuDomainVirStorageSourceFindByNodeName(virStorageSource
*top,
for (tmp = top; virStorageSourceIsBacking(tmp); tmp = tmp->backingStore) {
const char *nodestorage =
qemuBlockStorageSourceGetStorageNodename(tmp);
+const char *nodeformat = qemuBlockStorageSourceGetFormatNodename(tmp);
-if ((tmp->nodeformat && STREQ(tmp->nodeformat, nodeName)) ||
+if ((nodeformat && STREQ(nodeformat, nodeName)) ||
(nodestorage && STREQ(nodestorage, nodeName)))
return tmp;
}
@@ -11144,10 +11145,11 @@
qemuDomainPrepareStorageSourceBlockdevNodename(virDomainDiskDef *disk,
virQEMUDriverConfig *cfg)
{
char *nodestorage = g_strdup_printf("%s-storage", nodenameprefix);
+char *nodeformat = g_strdup_printf("%s-format", nodenameprefix);
/* qemuBlockStorageSourceSetStorageNodename steals 'nodestorage' */
qemuBlockStorageSourceSetStorageNodename(src, nodestorage);
-src->nodeformat = g_strdup_printf("%s-format", nodenameprefix);
+qemuBlockStorageSourceSetFormatNodename(src, nodeformat);
if (qemuBlockStorageSourceNeedsStorageSliceLayer(src))
src->sliceStorage->nodename = g_strdup_printf("libvirt-%u-slice-sto",
src->id);
@@ -11161,8 +11163,7 @@
qemuDomainPrepareStorageSourceBlockdevNodename(virDomainDiskDef *disk,
qemuDomainPrepareStorageSourceConfig(src, cfg);
qemuDomainPrepareDiskSourceData(disk, src);
-if (qemuDomainSecretStorageSourcePrepareEncryption(priv, src,
- src->nodeformat) < 0)
+if (qemuDomainSecretStorageSourcePrepareEncryption(priv, src, nodeformat)
< 0)
return -1;
if (!qemuDomainPrepareStorageSourceNbdkit(src, cfg, nodestorage, priv)) {
@@ -11698,7 +11699,7 @@ qemuDomainInitializePflashStorageSource(virDomainObj
*vm,
pflash0->path = g_strdup(def->os.loader->path);
pflash0->readonly = false;
virTristateBoolToBool(def->os.loader->readonly, >readonly);
-pflash0->nodeformat = g_strdup("libvirt-pflash0-format");
+qemuBlockStorageSourceSetFormatNodename(pflash0,
g_strdup("libvirt-pflash0-format"));
qemuBlockStorageSourceSetStorageNodename(pflash0,
g_strdup("libvirt-pflash0-storage"));
if (def->os.loader->nvram) {
--
2.41.0
[PATCH 10/31] qemu: domain: Rework assignment of 'storage' nodenames to use new accessors
Refactor the code which assigns the 'storage' layer nodenames for disks.
scsi hostdevs and pflash backend.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_domain.c| 25 ++---
src/qemu/qemu_migration.c | 2 +-
2 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 78dc99d243..885c5ee96b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11143,7 +11143,10 @@
qemuDomainPrepareStorageSourceBlockdevNodename(virDomainDiskDef *disk,
qemuDomainObjPrivate *priv,
virQEMUDriverConfig *cfg)
{
-src->nodestorage = g_strdup_printf("%s-storage", nodenameprefix);
+char *nodestorage = g_strdup_printf("%s-storage", nodenameprefix);
+
+/* qemuBlockStorageSourceSetStorageNodename steals 'nodestorage' */
+qemuBlockStorageSourceSetStorageNodename(src, nodestorage);
src->nodeformat = g_strdup_printf("%s-format", nodenameprefix);
if (qemuBlockStorageSourceNeedsStorageSliceLayer(src))
@@ -11162,18 +11165,17 @@
qemuDomainPrepareStorageSourceBlockdevNodename(virDomainDiskDef *disk,
src->nodeformat) < 0)
return -1;
-if (!qemuDomainPrepareStorageSourceNbdkit(src, cfg, src->nodestorage,
priv)) {
+if (!qemuDomainPrepareStorageSourceNbdkit(src, cfg, nodestorage, priv)) {
/* If we're using nbdkit to serve the storage source, we don't pass
* authentication secrets to qemu, but will pass them to nbdkit
instead */
-if (qemuDomainSecretStorageSourcePrepareAuth(priv, src,
- src->nodestorage) < 0)
+if (qemuDomainSecretStorageSourcePrepareAuth(priv, src, nodestorage) <
0)
return -1;
}
-if (qemuDomainPrepareStorageSourcePR(src, priv, src->nodestorage) < 0)
+if (qemuDomainPrepareStorageSourcePR(src, priv, nodestorage) < 0)
return -1;
-if (qemuDomainPrepareStorageSourceTLS(src, cfg, src->nodestorage,
+if (qemuDomainPrepareStorageSourceTLS(src, cfg, nodestorage,
priv) < 0)
return -1;
@@ -11297,12 +11299,14 @@ qemuDomainPrepareHostdevSCSI(virDomainHostdevDef
*hostdev,
}
if (src) {
-const char *backendalias = hostdev->info->alias;
+char *backendalias;
src->readonly = hostdev->readonly;
src->id = qemuDomainStorageIDNew(priv);
-src->nodestorage = g_strdup_printf("libvirt-%d-backend", src->id);
-backendalias = src->nodestorage;
+backendalias = g_strdup_printf("libvirt-%d-backend", src->id);
+
+/* 'src' takes ownership of 'backendalias' */
+qemuBlockStorageSourceSetStorageNodename(src, backendalias);
if (src->auth) {
virSecretUsageType usageType = VIR_SECRET_USAGE_TYPE_ISCSI;
@@ -11695,8 +11699,7 @@ qemuDomainInitializePflashStorageSource(virDomainObj
*vm,
pflash0->readonly = false;
virTristateBoolToBool(def->os.loader->readonly, >readonly);
pflash0->nodeformat = g_strdup("libvirt-pflash0-format");
-pflash0->nodestorage = g_strdup("libvirt-pflash0-storage");
-
+qemuBlockStorageSourceSetStorageNodename(pflash0,
g_strdup("libvirt-pflash0-storage"));
if (def->os.loader->nvram) {
if (qemuDomainPrepareStorageSourceBlockdevNodename(NULL,
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 011482c2b5..7d08df1fc5 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1011,7 +1011,7 @@
qemuMigrationSrcNBDStorageCopyBlockdevPrepareSource(virDomainDiskDef *disk,
copysrc->tlsAlias = g_strdup(tlsAlias);
copysrc->tlsHostname = g_strdup(tlsHostname);
-copysrc->nodestorage = g_strdup_printf("migration-%s-storage", disk->dst);
+qemuBlockStorageSourceSetStorageNodename(copysrc,
g_strdup_printf("migration-%s-storage", disk->dst));
copysrc->nodeformat = g_strdup_printf("migration-%s-format", disk->dst);
return g_steal_pointer();
--
2.41.0
[PATCH 05/31] tests: Use 'storage' layer nodename accessors in tests
Convert all places in tests to use the 'storage' layer nodename
accessors instead of (virStorageSource)->nodestorage.
Signed-off-by: Peter Krempa
---
tests/qemublocktest.c | 4 ++--
tests/qemumonitorjsontest.c | 2 +-
tests/qemuxml2argvtest.c| 3 ++-
3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
index edfe7719c8..9a72a67ce2 100644
--- a/tests/qemublocktest.c
+++ b/tests/qemublocktest.c
@@ -233,7 +233,7 @@ testQemuDiskXMLToJSONFakeSecrets(virStorageSource *src)
srcpriv->secinfo->username = g_strdup(src->auth->username);
srcpriv->secinfo->alias = g_strdup_printf("%s-secalias",
- NULLSTR(src->nodestorage));
+
NULLSTR(qemuBlockStorageSourceGetStorageNodename(src)));
}
if (src->encryption) {
@@ -667,7 +667,7 @@ testQemuBackupIncrementalBitmapCalculateGetFakeImage(size_t
idx)
ret->type = VIR_STORAGE_TYPE_FILE;
ret->format = VIR_STORAGE_FILE_QCOW2;
ret->path = g_strdup_printf("/image%zu", idx);
- ret->nodestorage = g_strdup_printf("libvirt-%zu-storage", idx);
+ qemuBlockStorageSourceSetStorageNodename(ret,
g_strdup_printf("libvirt-%zu-storage", idx));
ret->nodeformat = g_strdup_printf("libvirt-%zu-format", idx);
return ret;
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 2e7b661db4..6b82e3841b 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -2601,7 +2601,7 @@ testQemuMonitorJSONBlockdevReopen(const void *opaque)
src->format = VIR_STORAGE_FILE_QCOW2;
src->readonly = true;
src->nodeformat = g_strdup("test node");
-src->nodestorage = g_strdup("backing nodename");
+qemuBlockStorageSourceSetStorageNodename(src, g_strdup("backing
nodename"));
src->backingStore = virStorageSourceNew();
if (qemuMonitorTestAddItem(test, "blockdev-reopen", "{\"return\":{}}") < 0)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index a454dcb205..48058cb924 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -12,6 +12,7 @@
# include "internal.h"
# include "viralloc.h"
# include "viridentity.h"
+# include "qemu/qemu_block.h"
# include "qemu/qemu_capabilities.h"
# include "qemu/qemu_domain.h"
# include "qemu/qemu_migration.h"
@@ -338,7 +339,7 @@ testCompareXMLToArgvCreateArgs(virQEMUDriver *drv,
srcpriv = qemuDomainStorageSourcePrivateFetch(src);
-srcpriv->fdpass = qemuFDPassNew(src->nodestorage, priv);
+srcpriv->fdpass =
qemuFDPassNew(qemuBlockStorageSourceGetStorageNodename(src), priv);
qemuFDPassAddFD(srcpriv->fdpass, , "-vdpa");
}
}
--
2.41.0
[PATCH 15/31] conf: Rename 'nodestorage' field of virStorageSource to 'nodenamestorage'
While the name itself doesn't matter, this rename is done to prove that
all places using 'nodestorage' were converted to the appropriate
accessors.
Signed-off-by: Peter Krempa
---
src/conf/storage_source_conf.c | 4 ++--
src/conf/storage_source_conf.h | 2 +-
src/qemu/qemu_block.c | 8
src/qemu/qemu_blockjob.c | 2 +-
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c
index 61a433ab1f..ea171f9945 100644
--- a/src/conf/storage_source_conf.c
+++ b/src/conf/storage_source_conf.c
@@ -829,7 +829,7 @@ virStorageSourceCopy(const virStorageSource *src,
def->snapshot = g_strdup(src->snapshot);
def->configFile = g_strdup(src->configFile);
def->nodeformat = g_strdup(src->nodeformat);
-def->nodestorage = g_strdup(src->nodestorage);
+def->nodenamestorage = g_strdup(src->nodenamestorage);
def->compat = g_strdup(src->compat);
def->tlsAlias = g_strdup(src->tlsAlias);
def->tlsCertdir = g_strdup(src->tlsCertdir);
@@ -1168,7 +1168,7 @@ virStorageSourceClear(virStorageSource *def)
virStorageAuthDefFree(def->auth);
virObjectUnref(def->privateData);
-VIR_FREE(def->nodestorage);
+VIR_FREE(def->nodenamestorage);
VIR_FREE(def->nodeformat);
virStorageSourceBackingStoreClear(def);
diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h
index fc6c67f426..e9525c8f65 100644
--- a/src/conf/storage_source_conf.h
+++ b/src/conf/storage_source_conf.h
@@ -371,7 +371,7 @@ struct _virStorageSource {
/* metadata that allows identifying given storage source */
char *nodeformat; /* name of the format handler object */
-char *nodestorage; /* name of the storage object */
+char *nodenamestorage; /* name of the storage object */
/* An optional setting to enable usage of TLS for the storage source */
virTristateBool haveTLS;
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 7355cb0b5e..a2414dc2e3 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -64,8 +64,8 @@ void
qemuBlockStorageSourceSetStorageNodename(virStorageSource *src,
char *nodename)
{
-g_free(src->nodestorage);
-src->nodestorage = nodename;
+g_free(src->nodenamestorage);
+src->nodenamestorage = nodename;
}
@@ -83,7 +83,7 @@
qemuBlockStorageSourceGetEffectiveStorageNodename(virStorageSource *src)
src->sliceStorage->nodename)
return src->sliceStorage->nodename;
-return src->nodestorage;
+return src->nodenamestorage;
}
@@ -96,7 +96,7 @@
qemuBlockStorageSourceGetEffectiveStorageNodename(virStorageSource *src)
const char *
qemuBlockStorageSourceGetStorageNodename(virStorageSource *src)
{
-return src->nodestorage;
+return src->nodenamestorage;
}
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 25ac74d6c4..12f3d59df5 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -612,7 +612,7 @@ qemuBlockJobCleanStorageSourceRuntime(virStorageSource *src)
src->detected = false;
VIR_FREE(src->relPath);
VIR_FREE(src->backingStoreRaw);
-VIR_FREE(src->nodestorage);
+VIR_FREE(src->nodenamestorage);
VIR_FREE(src->nodeformat);
VIR_FREE(src->tlsAlias);
VIR_FREE(src->tlsCertdir);
--
2.41.0
[PATCH 09/31] qemu: block: Convert disk 'storage' backend JSON props generator to new accessors
We need to use the 'effective' storage nodename (one which includes the
optional storage slice 'raw' intermediate layer) in the code which
formats the 'format' layer props.
All other cases need the real storage driver nodename as they either
generate the 'storage' layer props, or the storage slice, which refers
to the proper storage backend.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index a98caa330e..1fc36569a9 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1060,8 +1060,10 @@ qemuBlockStorageSourceGetBackendProps(virStorageSource
*src,
return NULL;
if (!onlytarget) {
-if (qemuBlockNodeNameValidate(src->nodestorage) < 0 ||
-virJSONValueObjectAdd(, "S:node-name", src->nodestorage,
NULL) < 0)
+if
(qemuBlockNodeNameValidate(qemuBlockStorageSourceGetStorageNodename(src)) < 0 ||
+virJSONValueObjectAdd(,
+ "S:node-name",
qemuBlockStorageSourceGetStorageNodename(src),
+ NULL) < 0)
return NULL;
if (!legacy) {
@@ -1358,10 +1360,6 @@ qemuBlockStorageSourceGetFormatProps(virStorageSource
*src,
g_autoptr(virJSONValue) props = NULL;
const char *backingFormatterStr = NULL;
const char *backingNodename = NULL;
-const char *storagenode = src->nodestorage;
-
-if (qemuBlockStorageSourceNeedsStorageSliceLayer(src))
-storagenode = src->sliceStorage->nodename;
if (virStorageSourceIsBacking(backingStore) &&
src->format < VIR_STORAGE_FILE_BACKING) {
@@ -1386,7 +1384,7 @@ qemuBlockStorageSourceGetFormatProps(virStorageSource
*src,
return NULL;
if (virJSONValueObjectAdd(,
- "s:file", storagenode,
+ "s:file",
qemuBlockStorageSourceGetEffectiveStorageNodename(src),
backingFormatterStr, backingNodename,
NULL) < 0)
return 0;
@@ -1408,7 +1406,7 @@
qemuBlockStorageSourceGetBlockdevStorageSliceProps(virStorageSource *src)
"s:node-name", src->sliceStorage->nodename,
"U:offset", src->sliceStorage->offset,
"U:size", src->sliceStorage->size,
- "s:file", src->nodestorage,
+ "s:file",
qemuBlockStorageSourceGetStorageNodename(src),
"b:auto-read-only", true,
"s:discard", "unmap",
NULL) < 0)
--
2.41.0
[PATCH 16/31] qemu: block: Add accessors for format layer node names
Introduce a set of accessors, which return node names based on
semantics. This will allow to us to modify how we setup the backing
chain in cases when e.g. the format driver can be omitted, without
breaking all the code.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 31 +++
src/qemu/qemu_block.h | 7 +++
2 files changed, 38 insertions(+)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index a2414dc2e3..cba1fb1c1e 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -69,6 +69,23 @@ qemuBlockStorageSourceSetStorageNodename(virStorageSource
*src,
}
+/**
+ * qemuBlockStorageSourceSetFormatNodename:
+ * @src: virStorageSource to set the format nodename
+ * @nodename: The node name to set (stolen)
+ *
+ * Sets @nodename as the format node name of @src. Using NULL @nodename clears
+ * the nodename. @src takes ownership of @nodename.
+ */
+void
+qemuBlockStorageSourceSetFormatNodename(virStorageSource *src,
+char *nodename)
+{
+g_free(src->nodeformat);
+src->nodeformat = nodename;
+}
+
+
/**
* qemuBlockStorageSourceGetEffectiveStorageNodename:
* @src: virStorageSource to get the effective nodename of
@@ -100,6 +117,20 @@ qemuBlockStorageSourceGetStorageNodename(virStorageSource
*src)
}
+/**
+ * qemuBlockStorageSourceGetFormatNodename:
+ * @src: virStorageSource to get the effective nodename of
+ *
+ * Gets the nodename corresponding to the format layer. Useful when accessing
+ * format specific features. Returns NULL if there is no format layer.
+ */
+const char *
+qemuBlockStorageSourceGetFormatNodename(virStorageSource *src)
+{
+return src->nodeformat;
+}
+
+
/**
* qemuBlockStorageSourceSupportsConcurrentAccess:
* @src: disk storage source
diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h
index ecc5711dcd..6ed0aa85b2 100644
--- a/src/qemu/qemu_block.h
+++ b/src/qemu/qemu_block.h
@@ -30,12 +30,19 @@ void
qemuBlockStorageSourceSetStorageNodename(virStorageSource *src,
char *nodename);
+void
+qemuBlockStorageSourceSetFormatNodename(virStorageSource *src,
+char *nodename);
+
const char *
qemuBlockStorageSourceGetEffectiveStorageNodename(virStorageSource *src);
const char *
qemuBlockStorageSourceGetStorageNodename(virStorageSource *src);
+const char *
+qemuBlockStorageSourceGetFormatNodename(virStorageSource *src);
+
typedef struct qemuBlockNodeNameBackingChainData
qemuBlockNodeNameBackingChainData;
struct qemuBlockNodeNameBackingChainData {
--
2.41.0
[PATCH 12/31] qemu: Refactor storage backend 'storage' layer helepr object setup
Use the new nodename accessors for any storage layer helper object.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_domain.c | 2 +-
src/qemu/qemu_nbdkit.c | 4 ++--
src/qemu/qemu_process.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 885c5ee96b..1b2f09a316 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11106,7 +11106,7 @@ qemuDomainPrepareStorageSourceFDs(virStorageSource *src,
srcpriv = qemuDomainStorageSourcePrivateFetch(src);
-srcpriv->fdpass = qemuFDPassNew(src->nodestorage, priv);
+srcpriv->fdpass =
qemuFDPassNew(qemuBlockStorageSourceGetStorageNodename(src), priv);
for (i = 0; i < fdt->nfds; i++) {
g_autofree char *idx = g_strdup_printf("%zu", i);
diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
index 3ad63cfaa0..85e61be44c 100644
--- a/src/qemu/qemu_nbdkit.c
+++ b/src/qemu/qemu_nbdkit.c
@@ -923,7 +923,7 @@ qemuNbdkitStopStorageSource(virStorageSource *src,
if (priv && priv->nbdkitProcess &&
qemuNbdkitProcessStop(priv->nbdkitProcess, vm) < 0)
-VIR_WARN("Unable to stop nbdkit for storage source '%s'",
src->nodestorage);
+VIR_WARN("Unable to stop nbdkit for storage source '%s'",
qemuBlockStorageSourceGetStorageNodename(src));
}
}
@@ -1173,7 +1173,7 @@ qemuNbdkitProcessStart(qemuNbdkitProcess *proc,
logfd = qemuLogContextGetWriteFD(logContext);
-VIR_DEBUG("starting nbdkit process for %s", proc->source->nodestorage);
+VIR_DEBUG("starting nbdkit process for %s",
qemuBlockStorageSourceGetStorageNodename(proc->source));
virCommandSetErrorFD(cmd, );
virCommandSetOutputFD(cmd, );
virCommandSetPidFile(cmd, proc->pidfile);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index ae0bb7bf80..63c0c62a46 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6771,7 +6771,7 @@ qemuProcessPrepareHostStorageSourceVDPA(virStorageSource
*src,
srcpriv = qemuDomainStorageSourcePrivateFetch(src);
-srcpriv->fdpass = qemuFDPassNew(src->nodestorage, priv);
+srcpriv->fdpass =
qemuFDPassNew(qemuBlockStorageSourceGetStorageNodename(src), priv);
qemuFDPassAddFD(srcpriv->fdpass, , "-vdpa");
return 0;
}
--
2.41.0
[PATCH 01/31] qemu: domain: Identify blockjobs by storage nodename in VM status XML
Use the node name of the storage access driver to identify the block job
volumes. This will prepare the blockjob code to the possibility that the
format layer may be missing. Our lookup code can find either of them,
thus we can safely switch.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_domain.c | 8
tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml | 12 ++--
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index eec7bed28b..918b5a14e1 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2288,13 +2288,13 @@ qemuDomainPrivateBlockJobFormatCommit(qemuBlockJobData
*job,
g_auto(virBuffer) disabledBitmapsBuf = VIR_BUFFER_INIT_CHILD(buf);
if (job->data.commit.base)
-virBufferAsprintf(buf, "\n",
job->data.commit.base->nodeformat);
+virBufferAsprintf(buf, "\n",
job->data.commit.base->nodestorage);
if (job->data.commit.top)
-virBufferAsprintf(buf, "\n",
job->data.commit.top->nodeformat);
+virBufferAsprintf(buf, "\n",
job->data.commit.top->nodestorage);
if (job->data.commit.topparent)
-virBufferAsprintf(buf, "\n",
job->data.commit.topparent->nodeformat);
+virBufferAsprintf(buf, "\n",
job->data.commit.topparent->nodestorage);
if (job->data.commit.deleteCommittedImages)
virBufferAddLit(buf, "\n");
@@ -2357,7 +2357,7 @@ qemuDomainObjPrivateXMLFormatBlockjobIterator(void
*payload,
switch ((qemuBlockJobType) job->type) {
case QEMU_BLOCKJOB_TYPE_PULL:
if (job->data.pull.base)
-virBufferAsprintf(, "\n",
job->data.pull.base->nodeformat);
+virBufferAsprintf(, "\n",
job->data.pull.base->nodestorage);
break;
case QEMU_BLOCKJOB_TYPE_COMMIT:
diff --git a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
index b62b3149c2..380ef053d2 100644
--- a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
+++ b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
@@ -238,14 +238,14 @@
-
-
-
+
+
+
-
-
+
+
@@ -301,7 +301,7 @@
-
+
--
2.41.0
[PATCH 04/31] qemu: block: Add accessors for protocol/storage node names
Introduce a set of accessors, which return node names based on
semantics. This will allow to us to modify how we setup the backing
chain in cases when e.g. the format driver can be omitted, without
breaking all the code.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 48 +++
src/qemu/qemu_block.h | 11 ++
2 files changed, 59 insertions(+)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 07bc8ede76..0c9460f678 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -52,6 +52,54 @@ qemuBlockNodeNameValidate(const char *nn)
}
+/**
+ * qemuBlockStorageSourceSetStorageNodename:
+ * @src: virStorageSource to set the storage nodename
+ * @nodename: The node name to set (stolen)
+ *
+ * Sets @nodename as the storage node name of @src. Using NULL @nodename clears
+ * the nodename. @src takes ownership of @nodename.
+ */
+void
+qemuBlockStorageSourceSetStorageNodename(virStorageSource *src,
+ char *nodename)
+{
+g_free(src->nodestorage);
+src->nodestorage = nodename;
+}
+
+
+/**
+ * qemuBlockStorageSourceGetEffectiveStorageNodename:
+ * @src: virStorageSource to get the effective nodename of
+ *
+ * Gets the nodename that exposes the storage corresponding to @src, without
+ * the format driver applied. This function always returns a name.
+ */
+const char *
+qemuBlockStorageSourceGetEffectiveStorageNodename(virStorageSource *src)
+{
+if (src->sliceStorage &&
+src->sliceStorage->nodename)
+return src->sliceStorage->nodename;
+
+return src->nodestorage;
+}
+
+
+/**
+ * qemuBlockStorageSourceGetStorageNodename:
+ * @src: virStorageSource to get the effective nodename of
+ *
+ * Gets the nodename corresponding to the real backing storage format layer.
+ */
+const char *
+qemuBlockStorageSourceGetStorageNodename(virStorageSource *src)
+{
+return src->nodestorage;
+}
+
+
/**
* qemuBlockStorageSourceSupportsConcurrentAccess:
* @src: disk storage source
diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h
index cf5eaf87f3..ecc5711dcd 100644
--- a/src/qemu/qemu_block.h
+++ b/src/qemu/qemu_block.h
@@ -26,6 +26,17 @@
#include "virjson.h"
#include "viruri.h"
+void
+qemuBlockStorageSourceSetStorageNodename(virStorageSource *src,
+ char *nodename);
+
+const char *
+qemuBlockStorageSourceGetEffectiveStorageNodename(virStorageSource *src);
+
+const char *
+qemuBlockStorageSourceGetStorageNodename(virStorageSource *src);
+
+
typedef struct qemuBlockNodeNameBackingChainData
qemuBlockNodeNameBackingChainData;
struct qemuBlockNodeNameBackingChainData {
char *qemufilename; /* name of the image from qemu */
--
2.41.0
[PATCH 03/31] qemu: block: Rename qemuBlockStorageSourceGetBlockdevProps
Use qemuBlockStorageSourceGetFormatProps as it formats the properties of
the 'format' driver in qemu. Adjust the comment which was hinting
otherwise.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 16 +++-
src/qemu/qemu_block.h | 4 ++--
tests/qemublocktest.c | 2 +-
3 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 9b6d901e8c..07bc8ede76 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1295,18 +1295,17 @@
qemuBlockStorageSourceGetBlockdevFormatProps(virStorageSource *src)
/**
- * qemuBlockStorageSourceGetBlockdevProps:
+ * qemuBlockStorageSourceGetFormatProps:
*
* @src: storage source to format
* @backingStore: a storage source to use as backing of @src
*
- * Formats @src into a JSON object which can be used with blockdev-add or
- * -blockdev. The formatted object contains both the storage and format layer
- * in nested form including link to the backing chain layer if necessary.
+ * Formats properties of @src related to the format blockdev driver in qemu
+ * into a JSON object which can be used with blockdev-add or -blockdev.
*/
virJSONValue *
-qemuBlockStorageSourceGetBlockdevProps(virStorageSource *src,
- virStorageSource *backingStore)
+qemuBlockStorageSourceGetFormatProps(virStorageSource *src,
+ virStorageSource *backingStore)
{
g_autoptr(virJSONValue) props = NULL;
const char *backingFormatterStr = NULL;
@@ -1434,8 +1433,7 @@
qemuBlockStorageSourceAttachPrepareBlockdev(virStorageSource *src,
data = g_new0(qemuBlockStorageSourceAttachData, 1);
-if (!(data->formatProps = qemuBlockStorageSourceGetBlockdevProps(src,
-
backingStore)) ||
+if (!(data->formatProps = qemuBlockStorageSourceGetFormatProps(src,
backingStore)) ||
!(data->storageProps = qemuBlockStorageSourceGetBackendProps(src,
backendpropsflags)))
return NULL;
@@ -3049,7 +3047,7 @@ qemuBlockReopenFormatMon(qemuMonitor *mon,
g_autoptr(virJSONValue) srcprops = NULL;
g_autoptr(virJSONValue) reopenoptions = virJSONValueNewArray();
-if (!(srcprops = qemuBlockStorageSourceGetBlockdevProps(src,
src->backingStore)))
+if (!(srcprops = qemuBlockStorageSourceGetFormatProps(src,
src->backingStore)))
return -1;
if (virJSONValueArrayAppend(reopenoptions, ) < 0)
diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h
index 530d88d28e..cf5eaf87f3 100644
--- a/src/qemu/qemu_block.h
+++ b/src/qemu/qemu_block.h
@@ -57,8 +57,8 @@ virURI *
qemuBlockStorageSourceGetURI(virStorageSource *src);
virJSONValue *
-qemuBlockStorageSourceGetBlockdevProps(virStorageSource *src,
- virStorageSource *backingStore);
+qemuBlockStorageSourceGetFormatProps(virStorageSource *src,
+ virStorageSource *backingStore);
virJSONValue *
qemuBlockStorageGetCopyOnReadProps(virDomainDiskDef *disk);
diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
index 8bad69e7ac..edfe7719c8 100644
--- a/tests/qemublocktest.c
+++ b/tests/qemublocktest.c
@@ -301,7 +301,7 @@ testQemuDiskXMLToProps(const void *opaque)
qemuDomainPrepareDiskSourceData(disk, n);
-if (!(formatProps = qemuBlockStorageSourceGetBlockdevProps(n,
n->backingStore)) ||
+if (!(formatProps = qemuBlockStorageSourceGetFormatProps(n,
n->backingStore)) ||
!(storageSrcOnlyProps = qemuBlockStorageSourceGetBackendProps(n,
backendpropsflagstarget)) ||
!(storageProps = qemuBlockStorageSourceGetBackendProps(n,
backendpropsflagsnormal)) ||
!(backingstore = qemuBlockGetBackingStoreString(n, true))) {
--
2.41.0
[PATCH 06/31] qemuDomainVirStorageSourceFindByNodeName: Use proper accessor
The lookup by nodename requires the proper storage nodename which we use
also in status XML.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_domain.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 918b5a14e1..f891defa91 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2812,8 +2812,10 @@
qemuDomainVirStorageSourceFindByNodeName(virStorageSource *top,
virStorageSource *tmp;
for (tmp = top; virStorageSourceIsBacking(tmp); tmp = tmp->backingStore) {
+const char *nodestorage =
qemuBlockStorageSourceGetStorageNodename(tmp);
+
if ((tmp->nodeformat && STREQ(tmp->nodeformat, nodeName)) ||
-(tmp->nodestorage && STREQ(tmp->nodestorage, nodeName)))
+(nodestorage && STREQ(nodestorage, nodeName)))
return tmp;
}
--
2.41.0
[PATCH 02/31] qemu: block: Refactor logic in qemuBlockStorageSourceGetBlockdevProps
Restructure the conditions so that we can use virJSONValueObjectAdd with
a clearer logic for backing store control.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 46 ++-
1 file changed, 24 insertions(+), 22 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 7e870baa2f..9b6d901e8c 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1309,39 +1309,41 @@ qemuBlockStorageSourceGetBlockdevProps(virStorageSource
*src,
virStorageSource *backingStore)
{
g_autoptr(virJSONValue) props = NULL;
+const char *backingFormatterStr = NULL;
+const char *backingNodename = NULL;
const char *storagenode = src->nodestorage;
if (qemuBlockStorageSourceNeedsStorageSliceLayer(src))
storagenode = src->sliceStorage->nodename;
-if (!(props = qemuBlockStorageSourceGetBlockdevFormatProps(src)))
-return NULL;
-
-if (virJSONValueObjectAppendString(props, "file", storagenode) < 0)
+if (virStorageSourceIsBacking(backingStore) &&
+src->format < VIR_STORAGE_FILE_BACKING) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("storage format '%1$s' does not support backing
store"),
+ virStorageFileFormatTypeToString(src->format));
return NULL;
+}
-if (backingStore) {
-if (src->format >= VIR_STORAGE_FILE_BACKING) {
-if (virStorageSourceIsBacking(backingStore)) {
-if (virJSONValueObjectAppendString(props, "backing",
- backingStore->nodeformat) <
0)
-return NULL;
-} else {
-/* chain is terminated, indicate that no detection should
happen
- * in qemu */
-if (virJSONValueObjectAppendNull(props, "backing") < 0)
-return NULL;
-}
+if (backingStore &&
+src->format >= VIR_STORAGE_FILE_BACKING) {
+if (virStorageSourceIsBacking(backingStore)) {
+backingFormatterStr = "s:backing";
+backingNodename = backingStore->nodeformat;
} else {
-if (virStorageSourceIsBacking(backingStore)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("storage format '%1$s' does not support
backing store"),
- virStorageFileFormatTypeToString(src->format));
-return NULL;
-}
+/* chain is terminated, indicate that no detection should happen
in qemu */
+backingFormatterStr = "n:backing";
}
}
+if (!(props = qemuBlockStorageSourceGetBlockdevFormatProps(src)))
+return NULL;
+
+if (virJSONValueObjectAdd(,
+ "s:file", storagenode,
+ backingFormatterStr, backingNodename,
+ NULL) < 0)
+return 0;
+
return g_steal_pointer();
}
--
2.41.0
[PATCH 00/31] qemu: Add accessors for 'storage' and 'format' nodenames and refactor callers ('raw' driver removal part 1)
This is a prequel series necessary for removing the dummy 'raw' driver node if it isn't needed for performance reasons. Peter Krempa (31): qemu: domain: Identify blockjobs by storage nodename in VM status XML qemu: block: Refactor logic in qemuBlockStorageSourceGetBlockdevProps qemu: block: Rename qemuBlockStorageSourceGetBlockdevProps qemu: block: Add accessors for protocol/storage node names tests: Use 'storage' layer nodename accessors in tests qemuDomainVirStorageSourceFindByNodeName: Use proper accessor qemu: block: Use proper accessors for image formatting/creation code qemu: domain: Convert the status XML code for 'storage' nodenames to new accessors qemu: block: Convert disk 'storage' backend JSON props generator to new accessors qemu: domain: Rework assignment of 'storage' nodenames to use new accessors qemu: Refactor storage backend attach/detach setup code to use 'storage' nodename accessors qemu: Refactor storage backend 'storage' layer helepr object setup qemuDomainGetStatsBlockExportDisk: Use 'storage' node name accessors qemuDomainSetBlockThreshold: Use 'storage' node name accessor conf: Rename 'nodestorage' field of virStorageSource to 'nodenamestorage' qemu: block: Add accessors for format layer node names qemu: block: Add accessors for storage source effective nodename qemuBlockStorageSourceGetFormatProps: Use new frontend name accessor qemu: backup: Use format nodename accessors qemu: blockjob: Use 'format' nodename accessors for job naming qemu: block: Use 'format' nodename accessors in '-blockdev' setup code qemu: domain: Use 'format' layer node name accessors for nodename setup code tests: Use 'format' layer nodename accessors in test code qemu: Convert disk backend setup code to use 'format' nodename accessors qemu: driver: Convert disk stats code to use 'format' nodename accessors qemu: Use 'format' nodename accessors for block dirty bitmap operations qemu: command: Use 'format' nodename accessors for 'pflash' backend setup qemu: Convert migration setup code to use 'format' layer node name accessors qemu: migration: Use 'format' nodename accessors in dirty bitmap migration qemu: driver: Use 'format' nodename accessors for disk resize conf: Rename 'nodeformat' field of virStorageSource to 'nodenameformat' src/conf/storage_source_conf.c| 8 +- src/conf/storage_source_conf.h| 4 +- src/qemu/qemu_backup.c| 8 +- src/qemu/qemu_block.c | 238 -- src/qemu/qemu_block.h | 25 +- src/qemu/qemu_blockjob.c | 24 +- src/qemu/qemu_checkpoint.c| 9 +- src/qemu/qemu_command.c | 12 +- src/qemu/qemu_domain.c| 68 ++--- src/qemu/qemu_driver.c| 25 +- src/qemu/qemu_migration.c | 17 +- src/qemu/qemu_migration_cookie.c | 5 +- src/qemu/qemu_nbdkit.c| 4 +- src/qemu/qemu_process.c | 2 +- src/qemu/qemu_snapshot.c | 6 +- tests/qemublocktest.c | 16 +- tests/qemumonitorjsontest.c | 4 +- .../blockjob-blockdev-in.xml | 12 +- tests/qemuxml2argvtest.c | 3 +- 19 files changed, 314 insertions(+), 176 deletions(-) -- 2.41.0
Re: [libvirt] [PATCHv2 37/62] qemu: process: Setup disk io throttling?for -blockdev
On Mon, Oct 16, 2023 at 14:19:08 +, Chun Feng Wu wrote: > Thanks for your info! Currently, the iotune in libvirt’s xml indeed works > well, BTW, is there any plan for such “re-engineering”? No, there are no plans, as the current approach covers the vast majority of use cases. Spending significant engineering resources on something which will not get much use is not in the interest of the comunity.
Re: [libvirt] [PATCHv2 37/62] qemu: process: Setup disk io throttling for -blockdev
On Mon, Oct 16, 2023 at 13:36:42 +, Chun Feng Wu wrote: > Does this libvirt throttling support “chained throttle filters” for single > disk described in QEMU doc: > https://github.com/qemu/qemu/blob/master/docs/throttle.txt (“The 'throttle' > block filter”) No, libvirt currently still uses the older approach by using 'block_set_io_throttle' QMP command. This was used at the beginning because the 'throttle' layer was not yet ready at the point when libvirt adopted -blockdev. It is sufficient for the throttling options that are currently configurable via libvirt's XML. Note that to use the full potential of the throttle groups and hierarchical throttling will require significant re-engineering of the XML configuration of throttling.
Re: [PATCH 4/4] virSecretLoad: Simplify cleanup path
On Mon, Oct 16, 2023 at 10:07:51 +0200, Michal Privoznik wrote: > When loading a secret value fails, the control jumps over to the > 'cleanup' label where explicit call to virSecretDefFree() > happens. This is unnecessary as the corresponding variable can be > declared with g_autoptr() after which all error paths can just > return NULL instantly. > > Signed-off-by: Michal Privoznik > --- > src/conf/virsecretobj.c | 11 +-- > 1 file changed, 5 insertions(+), 6 deletions(-) Reviewed-by: Peter Krempa
Re: [PATCH 3/4] virSecretLoadAllConfigs: Use g_autofree for @path
On Mon, Oct 16, 2023 at 10:07:50 +0200, Michal Privoznik wrote: > When loading virSecret configs, the @path variable holds path to > individual config files. In each iteration it is freed explicitly > using VIR_FREE(). Switch it to g_autofree and remove those > explicit calls. > > Signed-off-by: Michal Privoznik > --- > src/conf/virsecretobj.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) Reviewed-by: Peter Krempa
Re: [PATCH 2/4] virfile: Drop virBuildPathInternal()
On Mon, Oct 16, 2023 at 10:07:49 +0200, Michal Privoznik wrote: > After previous cleanup the virBuildPathInternal() function is no > longer used. Drop it. > > Signed-off-by: Michal Privoznik > --- > src/libvirt_private.syms | 1 - > src/util/virfile.c | 22 -- > src/util/virfile.h | 5 - > 3 files changed, 28 deletions(-) Reviewed-by: Peter Krempa
Re: [PATCH 1/4] lib: Replace virBuildPath() with g_build_filename()
On Mon, Oct 16, 2023 at 10:07:48 +0200, Michal Privoznik wrote: > Our virBuildPath() constructs a path from given arguments. > Exactly like g_build_filename(), except the latter is more > generic as it uses backslashes on Windows. Therefore, replace the > former with the latter. > > Signed-off-by: Michal Privoznik > --- > docs/kbase/internals/command.rst | 2 +- > src/util/virfcp.c| 2 +- > src/util/virhook.c | 4 ++-- > src/util/virpci.c| 4 ++-- > 4 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/docs/kbase/internals/command.rst > b/docs/kbase/internals/command.rst > index 738fb5930a..d958c9d16a 100644 > --- a/docs/kbase/internals/command.rst > +++ b/docs/kbase/internals/command.rst > @@ -444,7 +444,7 @@ src/util/hooks.c > g_autofree char *path = NULL; > g_autoptr(virCommand) cmd = NULL; > > - virBuildPath(, LIBVIRT_HOOK_DIR, drvstr); > + path = g_build_filename(LIBVIRT_HOOK_DIR, drvstr); It's docs, but it's missing the NULL sentinel. Reviewed-by: Peter Krempa
[PATCH 6/7] conf: Save translated disk definition for disk type='volume' to status XML
Re-translating the disk source pools when reconnecting to a VM makes no sense as the volume might have changed or pool became inactive. The VM still uses the original volume though. Failing to re-translate the pool also causes the VM to be killed. Fix this by storing the original translation in the status XML. Resolves: https://issues.redhat.com/browse/RHEL-7345 Signed-off-by: Peter Krempa --- src/conf/domain_conf.c | 3 ++- src/conf/virdomainobjlist.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9b636215e9..80f467ae7a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -28247,7 +28247,8 @@ virDomainObjSave(virDomainObj *obj, VIR_DOMAIN_DEF_FORMAT_STATUS | VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET | VIR_DOMAIN_DEF_FORMAT_PCI_ORIG_STATES | - VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST); + VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST | + VIR_DOMAIN_DEF_FORMAT_VOLUME_TRANSLATED); g_autofree char *xml = NULL; diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 6be5de5e2b..0bd833257d 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -532,7 +532,8 @@ virDomainObjListLoadStatus(virDomainObjList *doms, VIR_DOMAIN_DEF_PARSE_ACTUAL_NET | VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE | - VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL))) + VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL | + VIR_DOMAIN_DEF_PARSE_VOLUME_TRANSLATED))) goto error; virUUIDFormat(obj->def->uuid, uuidstr); -- 2.41.0
[PATCH 1/7] virStorageSourcePoolDef: Turn 'mode' member into proper enum type
Use proper enum type and refactor the formatter accordingly.
Signed-off-by: Peter Krempa
---
src/conf/domain_conf.c | 12
src/conf/storage_source_conf.h | 2 +-
2 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 4435ee2ad4..d7f167a469 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7011,7 +7011,6 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node,
{
virStorageSourcePoolDef *source;
int ret = -1;
-g_autofree char *mode = NULL;
*srcpool = NULL;
@@ -7019,7 +7018,6 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node,
source->pool = virXMLPropString(node, "pool");
source->volume = virXMLPropString(node, "volume");
-mode = virXMLPropString(node, "mode");
/* CD-ROM and Floppy allows no source */
if (!source->pool && !source->volume) {
@@ -7033,13 +7031,11 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node,
goto cleanup;
}
-if (mode &&
-(source->mode = virStorageSourcePoolModeTypeFromString(mode)) <= 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown source mode '%1$s' for volume type disk"),
- mode);
+if (virXMLPropEnum(node, "mode",
+ virStorageSourcePoolModeTypeFromString,
+ VIR_XML_PROP_NONZERO,
+ >mode) < 0)
goto cleanup;
-}
*srcpool = g_steal_pointer();
ret = 0;
diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h
index fc6c67f426..bfa8d625e5 100644
--- a/src/conf/storage_source_conf.h
+++ b/src/conf/storage_source_conf.h
@@ -201,7 +201,7 @@ struct _virStorageSourcePoolDef {
int voltype; /* virStorageVolType, internal only */
int pooltype; /* virStoragePoolType from storage_conf.h, internal only */
virStorageType actualtype; /* internal only */
-int mode; /* virStorageSourcePoolMode, currently makes sense only for
iscsi pool */
+virStorageSourcePoolMode mode; /* currently makes sense only for iscsi
pool */
};
--
2.41.0
[PATCH 5/7] qemustatusxml2xmltest: Demonstrate use of VIR_DOMAIN_DEF_(PARSE|FORMAT)_VOLUME_TRANSLATED
Enable the flags in the status xml2xmtest and add an exaple to the test
data.
Signed-off-by: Peter Krempa
---
tests/qemustatusxml2xmldata/modern-in.xml | 4 ++--
tests/qemustatusxml2xmltest.c | 6 --
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/tests/qemustatusxml2xmldata/modern-in.xml
b/tests/qemustatusxml2xmldata/modern-in.xml
index e139c8d38c..67e0aa4952 100644
--- a/tests/qemustatusxml2xmldata/modern-in.xml
+++ b/tests/qemustatusxml2xmldata/modern-in.xml
@@ -378,9 +378,9 @@
-
+
-
+
diff --git a/tests/qemustatusxml2xmltest.c b/tests/qemustatusxml2xmltest.c
index 418a724b94..f1589345c3 100644
--- a/tests/qemustatusxml2xmltest.c
+++ b/tests/qemustatusxml2xmltest.c
@@ -30,7 +30,8 @@ testCompareStatusXMLToXMLFiles(const void *opaque)
VIR_DOMAIN_DEF_PARSE_ACTUAL_NET |
VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES |
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE |
-
VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL))) {
+
VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL |
+
VIR_DOMAIN_DEF_PARSE_VOLUME_TRANSLATED))) {
VIR_TEST_DEBUG("\nfailed to parse '%s'", data->infile);
goto cleanup;
}
@@ -40,7 +41,8 @@ testCompareStatusXMLToXMLFiles(const void *opaque)
VIR_DOMAIN_DEF_FORMAT_STATUS |
VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET |
VIR_DOMAIN_DEF_FORMAT_PCI_ORIG_STATES |
- VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST))) {
+ VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST |
+
VIR_DOMAIN_DEF_FORMAT_VOLUME_TRANSLATED))) {
VIR_TEST_DEBUG("\nfailed to format back '%s'", data->infile);
goto cleanup;
}
--
2.41.0
[PATCH 2/7] virDomainDiskSourcePoolDefParse: Refactor cleanup
Register autoptr cleanup function for virStorageSourcePoolDef and
refactor the parser to simplify the logic.
Signed-off-by: Peter Krempa
---
src/conf/domain_conf.c | 35 +++---
src/conf/storage_source_conf.h | 1 +
2 files changed, 12 insertions(+), 24 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d7f167a469..3e0989e2e8 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7005,44 +7005,31 @@ virDomainLeaseDefParseXML(xmlNodePtr node,
return NULL;
}
-static int
-virDomainDiskSourcePoolDefParse(xmlNodePtr node,
-virStorageSourcePoolDef **srcpool)
+static virStorageSourcePoolDef *
+virDomainDiskSourcePoolDefParse(xmlNodePtr node)
{
-virStorageSourcePoolDef *source;
-int ret = -1;
-
-*srcpool = NULL;
-
-source = g_new0(virStorageSourcePoolDef, 1);
+g_autoptr(virStorageSourcePoolDef) source =
g_new0(virStorageSourcePoolDef, 1);
source->pool = virXMLPropString(node, "pool");
source->volume = virXMLPropString(node, "volume");
-/* CD-ROM and Floppy allows no source */
-if (!source->pool && !source->volume) {
-ret = 0;
-goto cleanup;
-}
+/* CD-ROM and Floppy allows no source -> empty pool */
+if (!source->pool && !source->volume)
+return g_steal_pointer();
if (!source->pool || !source->volume) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("'pool' and 'volume' must be specified together for
'pool' type source"));
-goto cleanup;
+return NULL;
}
if (virXMLPropEnum(node, "mode",
virStorageSourcePoolModeTypeFromString,
VIR_XML_PROP_NONZERO,
>mode) < 0)
-goto cleanup;
-
-*srcpool = g_steal_pointer();
-ret = 0;
+return NULL;
- cleanup:
-virStorageSourcePoolDefFree(source);
-return ret;
+return g_steal_pointer();
}
@@ -7482,7 +7469,7 @@ virDomainStorageSourceParse(xmlNodePtr node,
return -1;
break;
case VIR_STORAGE_TYPE_VOLUME:
-if (virDomainDiskSourcePoolDefParse(node, >srcpool) < 0)
+if (!(src->srcpool = virDomainDiskSourcePoolDefParse(node)))
return -1;
break;
case VIR_STORAGE_TYPE_NVME:
@@ -8660,7 +8647,7 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt,
units = virXMLPropString(source_node, "units");
} else if (def->type == VIR_DOMAIN_FS_TYPE_VOLUME) {
def->src->type = VIR_STORAGE_TYPE_VOLUME;
-if (virDomainDiskSourcePoolDefParse(source_node,
>src->srcpool) < 0)
+if (!(def->src->srcpool =
virDomainDiskSourcePoolDefParse(source_node)))
goto error;
}
}
diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h
index bfa8d625e5..0cd5cd0192 100644
--- a/src/conf/storage_source_conf.h
+++ b/src/conf/storage_source_conf.h
@@ -498,6 +498,7 @@ virStorageSourceInitChainElement(virStorageSource *newelem,
void
virStorageSourcePoolDefFree(virStorageSourcePoolDef *def);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageSourcePoolDef,
virStorageSourcePoolDefFree);
void
virStorageSourceClear(virStorageSource *def);
--
2.41.0
[PATCH 4/7] qemu: domain: Allow preserving translated disk type='volume' data into XML if needed
Re-translating a disk type='volume' definition from a storage pool is
not a good idea in cases when the volume might have changed or we might
not have access to the storage driver.
Specific cases are if a storage pool is not activated on daemon restart,
then re-connecting to a VM fails, or if the virt-aa-helper program tries
to setup labelling for apparmor.
Add a new flag which will preserve the translated data in the
definition.
Signed-off-by: Peter Krempa
---
src/conf/domain_conf.c | 69 ++
src/conf/domain_conf.h | 4 +++
2 files changed, 53 insertions(+), 20 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e128457b00..9b636215e9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7006,7 +7006,8 @@ virDomainLeaseDefParseXML(xmlNodePtr node,
}
static virStorageSourcePoolDef *
-virDomainDiskSourcePoolDefParse(xmlNodePtr node)
+virDomainDiskSourcePoolDefParse(xmlNodePtr node,
+virDomainDefParseFlags flags)
{
g_autoptr(virStorageSourcePoolDef) source =
g_new0(virStorageSourcePoolDef, 1);
@@ -7029,6 +7030,14 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node)
>mode) < 0)
return NULL;
+if (flags & VIR_DOMAIN_DEF_PARSE_VOLUME_TRANSLATED) {
+if (virXMLPropEnum(node, "actualType",
+ virStorageTypeFromString,
+ VIR_XML_PROP_NONZERO,
+ >actualtype) < 0)
+return NULL;
+}
+
return g_steal_pointer();
}
@@ -7448,12 +7457,22 @@ virDomainStorageSourceParse(xmlNodePtr node,
unsigned int flags,
virDomainXMLOption *xmlopt)
{
+virStorageType actualType = src->type;
VIR_XPATH_NODE_AUTORESTORE(ctxt)
xmlNodePtr tmp;
ctxt->node = node;
-switch (src->type) {
+if (src->type == VIR_STORAGE_TYPE_VOLUME) {
+if (!(src->srcpool = virDomainDiskSourcePoolDefParse(node, flags)))
+return -1;
+
+/* If requested we need to also parse the translated volume runtime
data */
+if (flags & VIR_DOMAIN_DEF_PARSE_VOLUME_TRANSLATED)
+actualType = virStorageSourceGetActualType(src);
+}
+
+switch (actualType) {
case VIR_STORAGE_TYPE_FILE:
src->path = virXMLPropString(node, "file");
src->fdgroup = virXMLPropString(node, "fdgroup");
@@ -7469,8 +7488,7 @@ virDomainStorageSourceParse(xmlNodePtr node,
return -1;
break;
case VIR_STORAGE_TYPE_VOLUME:
-if (!(src->srcpool = virDomainDiskSourcePoolDefParse(node)))
-return -1;
+/* parsed above */
break;
case VIR_STORAGE_TYPE_NVME:
if (virDomainDiskSourceNVMeParse(node, ctxt, src) < 0)
@@ -7488,7 +7506,7 @@ virDomainStorageSourceParse(xmlNodePtr node,
case VIR_STORAGE_TYPE_LAST:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected disk type %1$s"),
- virStorageTypeToString(src->type));
+ virStorageTypeToString(actualType));
return -1;
}
@@ -8647,7 +8665,7 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt,
units = virXMLPropString(source_node, "units");
} else if (def->type == VIR_DOMAIN_FS_TYPE_VOLUME) {
def->src->type = VIR_STORAGE_TYPE_VOLUME;
-if (!(def->src->srcpool =
virDomainDiskSourcePoolDefParse(source_node)))
+if (!(def->src->srcpool =
virDomainDiskSourcePoolDefParse(source_node, flags)))
goto error;
}
}
@@ -22335,10 +22353,28 @@ virDomainDiskSourceFormat(virBuffer *buf,
bool skipEnc,
virDomainXMLOption *xmlopt)
{
+virStorageType actualType = src->type;
g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
-switch (src->type) {
+if (src->type == VIR_STORAGE_TYPE_VOLUME) {
+if (src->srcpool) {
+virBufferEscapeString(, " pool='%s'", src->srcpool->pool);
+virBufferEscapeString(, " volume='%s'",
src->srcpool->volume);
+if (src->srcpool->mode)
+virBufferAsprintf(, " mode='%s'",
+
virStorageSourcePoolModeTypeToString(src->srcpool->mode));
+}
+
+if (flags & VIR_DOMAIN_DEF_FORMAT_VOLUME_TRANSLATED &&
+src->srcpool->actualtype != VIR_STORAGE_TYPE_NONE) {
+virBufferAsprintf(, " actualType='%s'",
+
virStorageTypeToString(src->srcpool->actualtype));
+actualType = virStorageSource
[PATCH 0/7] qemu: Fix two pool-translation related bugs
Fix following problems: - fix disk type='volume' disks resolving to local images on apparmor-protected hosts - VMs are killed if pool is not available after libvirtd/virtqemud restart The first one will be annoying for users of Cockpit on apparmor based boxes. NOTE: I didn't test it though as I don't have such distro handy. Peter Krempa (7): virStorageSourcePoolDef: Turn 'mode' member into proper enum type virDomainDiskSourcePoolDefParse: Refactor cleanup virDomainDiskTranslateSourcePool: Don't re-translate already translated defs qemu: domain: Allow preserving translated disk type='volume' data into XML if needed qemustatusxml2xmltest: Demonstrate use of VIR_DOMAIN_DEF_(PARSE|FORMAT)_VOLUME_TRANSLATED conf: Save translated disk definition for disk type='volume' to status XML security: apparmor: Use translated disk definitions for disk type=volume src/conf/domain_conf.c| 111 -- src/conf/domain_conf.h| 4 + src/conf/storage_source_conf.h| 3 +- src/conf/virdomainobjlist.c | 3 +- src/security/security_apparmor.c | 8 +- src/security/virt-aa-helper.c | 3 +- tests/qemustatusxml2xmldata/modern-in.xml | 4 +- tests/qemustatusxml2xmltest.c | 6 +- 8 files changed, 84 insertions(+), 58 deletions(-) -- 2.41.0
[PATCH 3/7] virDomainDiskTranslateSourcePool: Don't re-translate already translated defs
If a disk definition was already translated re-doing it makes no sense.
Skip the translation if the 'actualtype' is already populated.
Signed-off-by: Peter Krempa
---
src/conf/domain_conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3e0989e2e8..e128457b00 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -30529,7 +30529,7 @@ virDomainDiskTranslateSourcePool(virDomainDiskDef *def)
virStorageSource *n;
for (n = def->src; virStorageSourceIsBacking(n); n = n->backingStore) {
-if (n->type != VIR_STORAGE_TYPE_VOLUME || !n->srcpool)
+if (n->type != VIR_STORAGE_TYPE_VOLUME || !n->srcpool ||
n->srcpool->actualtype != VIR_STORAGE_TYPE_NONE)
continue;
if (!conn) {
--
2.41.0
[PATCH 7/7] security: apparmor: Use translated disk definitions for disk type=volume
The 'virt-aa-helper' process gets a XML of the VM it needs to create a
profile for. For a disk type='volume' this XML contained only the
pool and volume name.
The 'virt-aa-helper' needs a local path though for anything it needs to
label. This means that we'd either need to invoke connection to the
storage driver and re-resolve the volume. Alternative which makes more
sense is to pass the proper data in the XML already passed to it via the
new XML formatter and parser flags.
This was indirectly reported upstream in
https://gitlab.com/libvirt/libvirt/-/issues/546
The configuration in the issue above was created by Cockpit on Debian.
Since Cockpit is getting more popular it's more likely that users will
be impacted by this problem.
Signed-off-by: Peter Krempa
---
src/security/security_apparmor.c | 8 ++--
src/security/virt-aa-helper.c| 3 ++-
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index bce797de7c..6fd0aedacf 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -159,13 +159,17 @@ load_profile(virSecurityManager *mgr G_GNUC_UNUSED,
bool append)
{
bool create = true;
+g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
g_autofree char *xml = NULL;
g_autoptr(virCommand) cmd = NULL;
-xml = virDomainDefFormat(def, NULL, VIR_DOMAIN_DEF_FORMAT_SECURE);
-if (!xml)
+if (virDomainDefFormatInternal(def, NULL, ,
+ VIR_DOMAIN_DEF_FORMAT_SECURE |
+ VIR_DOMAIN_DEF_FORMAT_VOLUME_TRANSLATED) <
0)
return -1;
+xml = virBufferContentAndReset();
+
if (profile_status_file(profile) >= 0)
create = false;
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 0855eb68ca..be13979cef 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -654,7 +654,8 @@ get_definition(vahControl * ctl, const char *xmlStr)
ctl->def = virDomainDefParseString(xmlStr,
ctl->xmlopt, NULL,
VIR_DOMAIN_DEF_PARSE_SKIP_SECLABEL |
- VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE);
+ VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE |
+ VIR_DOMAIN_DEF_PARSE_VOLUME_TRANSLATED);
if (ctl->def == NULL) {
vah_error(ctl, 0, _("could not parse XML"));
--
2.41.0
Re: [PATCH 1/1] Checking the value returned by the function
On Wed, Oct 11, 2023 at 15:24:17 +0300, Sergey Mironov wrote:
> The virSecuritySELinuxSetFilecon function (by definition) always returns
> values 0 or -1.
> The result of this function is written to 'ret'.
> The code compares the value of the variable 'ret' with 1.
>
> Signed-off-by: Sergey Mironov
> ---
> src/security/security_selinux.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 7914aba84d..7bff780ddf 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -1988,7 +1988,7 @@
> virSecuritySELinuxSetImageLabelInternal(virSecurityManager *mgr,
> ret = virSecuritySELinuxSetFilecon(mgr, path, use_label, remember);
> }
>
> -if (ret == 1 && !disk_seclabel) {
> +if (ret == -1 && !disk_seclabel) {
Based on the git history it appears that this condition is impossible to
satisfy for more than 10 years. If that is so we need to first assess if
we even want to make this code active, given that it wans't a problem
for such a long time.
It's also possible that it became impossible to reach after a change
outside of the context I've checked, but either way it should not be
blindly fixed even if it appears to be what the author intended
originally
Re: [PATCHv2 1/1] Changes from [v1]:
On Wed, Oct 11, 2023 at 11:19:08 +0300, Sergey Mironov wrote: > - now checked devAlias and making the code more robust in case when qemu stop > sending a devAlias > > [v1] https://listman.redhat.com/archives/libvir-list/2023-October/242544.html When this is commited to the repository the history of reviews is no longer important. The commit message should not include it at all and neither link to previous versions, but rather just state what the patch is doing. I'll address that by rewriting the commit message before pushing, so you don't need to send another version, just keep that in mind for next time. > > Signed-off-by: Sergey Mironov > > --- > v2: devAlias pointer was checked > --- > src/qemu/qemu_monitor_json.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c > index 5b9edadcf7..e37faff683 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -934,7 +934,7 @@ qemuMonitorJSONHandleTrayChange(qemuMonitor *mon, > int reason; > > /* drive alias is always reported but empty for -blockdev */ > -if (*devAlias == '\0') > +if (devAlias && *devAlias == '\0') Reviewed-by: Peter Krempa
Re: [PATCH 1/1] The virJSONValueObjectGetString function can return NULL, there are no checks
On Tue, Oct 10, 2023 at 17:55:26 +0300, Sergey Mironov wrote:
> The devAlias check is performed in the "if (!devAlias && !devid) {", so the
> situation is not handled when devAlias is NULL, and devid is not NULL
>
> Signed-off-by: Sergey Mironov
> ---
> src/qemu/qemu_monitor_json.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 5b9edadcf7..80b41d77d3 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -933,6 +933,11 @@ qemuMonitorJSONHandleTrayChange(qemuMonitor *mon,
> bool trayOpened;
> int reason;
>
> +if (!devAlias) {
> +VIR_WARN("missing device in tray change event");
> +return;
> +}
This is not correct and based on a wrong assumption. Currently the code
always gets a 'devAlias' set but it *may* be an empty string.
If it is an empty string it's cleared first before continuing.
Then if both the alias and 'id' is NULL the function terminates.
Making the code more robust in case when qemu stops sending a devAlias
is a good thing though, but the fix propsed here would break the
function as it would end early even if 'devid' is non-NULL. The
code afterwards is okay with devAlias being NULL.
Thus the correct fix is for the condition to read:
if (devAlias && *devAlias == '\0')
devAlias = NULL;
and leave the other logic as-is.
Re: [RFC PATCH 4/4] qemu_command: Added "ebpf_rss_fds" support for virtio-net.
On Mon, Oct 09, 2023 at 09:16:14 +0300, Andrew Melnychenko wrote:
> Added logic for loading the "RSS" eBPF program.
> eBPF file descriptors passed to the QEMU.
>
> Signed-off-by: Andrew Melnychenko
> ---
> src/qemu/qemu_command.c | 53 +
> src/qemu/qemu_domain.c | 4
> src/qemu/qemu_domain.h | 3 +++
> 3 files changed, 60 insertions(+)
This will require addition to qemuxml2argvtest once the qemu bit is
ready.
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 8a7b80719f..ca6cb1bc7a 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3786,6 +3786,23 @@ qemuBuildLegacyNicStr(virDomainNetDef *net)
> NULLSTR_EMPTY(net->info.alias));
> }
>
> +static char *qemuBuildEbpfRssArg(virDomainNetDef *net) {
> +qemuDomainNetworkPrivate *netpriv = QEMU_DOMAIN_NETWORK_PRIVATE(net);
> +g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
> +size_t nfds;
> +GSList *n;
> +
> +if (netpriv->ebpfrssfds) {
This check is not needed.
> +nfds = 0;
> +for (n = netpriv->ebpfrssfds; n; n = n->next) {
> +virBufferAsprintf(, "%s:", qemuFDPassDirectGetPath(n->data));
> +nfds++;
'nfds' is not used.
> +}
> +}
> +virBufferTrim(, ":");
> +
> +return virBufferContentAndReset();
> +}
>
> virJSONValue *
> qemuBuildNicDevProps(virDomainDef *def,
> @@ -3871,6 +3888,11 @@ qemuBuildNicDevProps(virDomainDef *def,
>"T:failover", failover,
>NULL) < 0)
> return NULL;
> +if (net->driver.virtio.rss == VIR_TRISTATE_SWITCH_ON &&
> virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_EBPF_RSS_FDS)){
This check should not be needed here ...
> +g_autofree char *ebpf = qemuBuildEbpfRssArg(net);
Because this will return a NULL string if it was not set up.
> +if (virJSONValueObjectAdd(, "S:ebpf_rss_fds", ebpf, NULL)
> < 0)
This is definitely not needed in a separate step and can be done in the
block above as "S:" skipps the attribute if the argument is NULL.
> +return NULL;
> +}
> } else {
> if (virJSONValueObjectAdd(,
>"s:driver",
> virDomainNetGetModelString(net),
> @@ -4152,6 +4174,33 @@ qemuBuildWatchdogDevProps(const virDomainDef *def,
> }
>
>
> +static void qemuOpenEbpfRssFds(virDomainNetDef *net,
> + virQEMUCaps *qemuCaps) {
> +const void *ebpfRSSObject = NULL;
> +size_t ebpfRSSObjectSize = 0;
> +int fds[16];
16 feels arbitrary
> +int nfds = 0;
> +qemuDomainNetworkPrivate *netpriv = QEMU_DOMAIN_NETWORK_PRIVATE(net);
> +
> +netpriv->libbpfRSSObject = NULL;
> +netpriv->ebpfrssfds = NULL;
> +
> +/* Add ebpf values */
> +if (net->driver.virtio.rss == VIR_TRISTATE_SWITCH_ON &&
> virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_EBPF_RSS_FDS)) {
This line is too long and there's a good spot to break it up;
> +ebpfRSSObject = virQEMUCapsGetEbpf(qemuCaps, "rss",
> );
> +nfds = qemuInterfaceLoadEbpf(ebpfRSSObject, ebpfRSSObjectSize,
> >libbpfRSSObject, fds, 16);
Note that the '16' argument is unused by the callee.
Also note that when running an unprivileged instance of libvirt, this
function might not have permissions to do anything. It seems that the
rest is okay, but we need to avoid any form of spurious errors and such
if that is really okay and there are fallback paths.
> +
> +if (nfds > 0) {
> +for (int i = 0; i < nfds; ++i) {
> +g_autofree char *name = g_strdup_printf("ebpfrssfd-%s-%u",
> net->info.alias, i);
> +netpriv->ebpfrssfds = g_slist_prepend(netpriv->ebpfrssfds,
> qemuFDPassDirectNew(name, fds + i));
> +}
> +netpriv->ebpfrssfds = g_slist_reverse(netpriv->ebpfrssfds);
> +}
> +}
> +}
> +
> +
> static int
> qemuBuildWatchdogCommandLine(virCommand *cmd,
> const virDomainDef *def,
> @@ -8735,6 +8784,10 @@ qemuBuildInterfaceCommandLine(virQEMUDriver *driver,
> qemuFDPassDirectTransferCommand(netpriv->slirpfd, cmd);
> qemuFDPassTransferCommand(netpriv->vdpafd, cmd);
>
> +qemuOpenEbpfRssFds(net, qemuCaps);
Note that this will cause problems when you'll be about to test this
functionality in qemuxml2argvtest. The test code must not modify the
host state, so this will either need to be mocked in the testsuite or
you'll need to move it to the host-prepare function
'qemuProcessPrepareHost' which is skipped from the test suite. The test
suite will then need to fake the data.
> +for (n = netpriv->ebpfrssfds; n; n = n->next)
> +qemuFDPassDirectTransferCommand(n->data, cmd);
> +
> if (!(hostnetprops = qemuBuildHostNetProps(vm, net)))
> goto cleanup;
>
> diff --git a/src/qemu/qemu_domain.c
Re: [RFC PATCH 3/4] qemu_interface: Added routine for loading the eBPF objects.
On Mon, Oct 09, 2023 at 09:16:13 +0300, Andrew Melnychenko wrote:
> Also, added dependencies for libbpf with bpf option.
>
> Signed-off-by: Andrew Melnychenko
> ---
> meson.build | 6 ++
> meson_options.txt | 1 +
> src/qemu/meson.build | 1 +
> src/qemu/qemu_interface.c | 42 +++
> src/qemu/qemu_interface.h | 4
> 5 files changed, 54 insertions(+)
Yet again fails to compile:
a.p/qemu_interface.c.o -c ../../../libvirt/src/qemu/qemu_interface.c
../../../libvirt/src/qemu/qemu_interface.c: In function ‘qemuInterfaceLoadEbpf’:
../../../libvirt/src/qemu/qemu_interface.c:779:5: error: ISO C90 forbids mixed
declarations and code [-Werror=declaration-after-statement]
779 | struct bpf_program *prog;
| ^~
../../../libvirt/src/qemu/qemu_interface.c:769:103: error: unused parameter
‘nfds’ [-Werror=unused-parameter]
769 | int qemuInterfaceLoadEbpf(const void *ebpfObject, size_t ebpfSize, void
**retLibbpfObj, int *fds, int nfds) {
|
^~~~
Fixing that also shows that the syntax-check rules were also broken.
>
> diff --git a/meson.build b/meson.build
> index de23fbda1e..b68e916246 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1381,6 +1381,11 @@ if yajl_dep.found()
>conf.set('WITH_YAJL', 1)
> endif
>
> +bpf_version = '1.1.0'
> +bpf_dep = dependency('libbpf', version: '>=' + bpf_version, required:
> get_option('bpf'))
> +if bpf_dep.found()
> +conf.set('WITH_BPF', 1)
> +endif
>
> # generic build dependencies checks
>
> @@ -2269,6 +2274,7 @@ libs_summary = {
>'udev': udev_dep.found(),
>'xdr': xdr_dep.found(),
>'yajl': yajl_dep.found(),
> + 'bpf': bpf_dep.found(),
> }
> summary(libs_summary, section: 'Libraries', bool_yn: true)
>
> diff --git a/meson_options.txt b/meson_options.txt
> index 7c428a9eb0..092b2efe06 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -44,6 +44,7 @@ option('udev', type: 'feature', value: 'auto', description:
> 'udev support')
> option('wireshark_dissector', type: 'feature', value: 'auto', description:
> 'wireshark support')
> option('wireshark_plugindir', type: 'string', value: '', description:
> 'wireshark plugins directory for use when installing wireshark plugin')
> option('yajl', type: 'feature', value: 'auto', description: 'yajl support')
> +option('bpf', type: 'feature', value: 'auto', description: 'qemu libbpf
> support')
>
>
> # build driver options
> diff --git a/src/qemu/meson.build b/src/qemu/meson.build
> index 64c62e584f..afd9133ae0 100644
> --- a/src/qemu/meson.build
> +++ b/src/qemu/meson.build
> @@ -105,6 +105,7 @@ if conf.has('WITH_QEMU')
>selinux_dep,
>src_dep,
>xdr_dep,
> + bpf_dep,
> ],
> include_directories: [
>conf_inc_dir,
> diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
> index 8856bb95a8..a3a43a43c5 100644
> --- a/src/qemu/qemu_interface.c
> +++ b/src/qemu/qemu_interface.c
> @@ -36,6 +36,7 @@
>
> #include
> #include
> +#include
>
> #define VIR_FROM_THIS VIR_FROM_QEMU
>
> @@ -763,3 +764,44 @@ qemuInterfaceOpenVhostNet(virDomainObj *vm,
> virDomainAuditNetDevice(vm->def, net, vhostnet_path, vhostfdSize);
> return 0;
> }
> +
> +
Please document this function. It's not very obvious what it does on the
first glance.
> +int qemuInterfaceLoadEbpf(const void *ebpfObject, size_t ebpfSize, void
> **retLibbpfObj, int *fds, int nfds) {
> +int err = 0;
> +int i = 0;
> +
> +struct bpf_object *obj = bpf_object__open_mem(ebpfObject, ebpfSize,
> NULL);
This is compiled regardless of 'WITH_BPF' flag ...
> +err = libbpf_get_error(obj);
> +if(err) {
> +return -1;
> +}
Re: [RFC PATCH 2/4] qemu_capabilities: Added new capability ebpf_rss_fds for QEMU.
On Mon, Oct 09, 2023 at 09:16:12 +0300, Andrew Melnychenko wrote:
> Also, added logic for retrieving eBPF objects from QEMU.
> eBPF objects stored in the hash table during runtime.
> eBPF objects cached encoded in base64 in the .xml cache file.
>
> Signed-off-by: Andrew Melnychenko
> ---
> src/qemu/qemu_capabilities.c | 181 +++
> src/qemu/qemu_capabilities.h | 4 +
> 2 files changed, 185 insertions(+)
Once again, make sure to check that each patch builds and passes tests.
This one fails to compile:
u/libvirt_driver_qemu_impl.a.p/qemu_capabilities.c.o -c
../../../libvirt/src/qemu/qemu_capabilities.c
../../../libvirt/src/qemu/qemu_capabilities.c:808:6: error: no previous
prototype for ‘virQEMUEbpfOBjectDataDestroy’ [-Werror=missing-prototypes]
808 | void virQEMUEbpfOBjectDataDestroy(gpointer data) {
| ^~~~
../../../libvirt/src/qemu/qemu_capabilities.c: In function
‘virQEMUEbpfOBjectDataDestroy’:
../../../libvirt/src/qemu/qemu_capabilities.c:811:5: error: ISO C90 forbids
mixed declarations and code [-Werror=declaration-after-statement]
811 | struct virQEMUEbpfOBjectData *object = data;
| ^~
../../../libvirt/src/qemu/qemu_capabilities.c: In function
‘virQEMUCapsProbeQMPEbpfObject’:
../../../libvirt/src/qemu/qemu_capabilities.c:5560:5: error: ISO C90 forbids
mixed declarations and code [-Werror=declaration-after-statement]
5560 | struct virQEMUEbpfOBjectData *object = g_malloc(sizeof(*object));
| ^~
../../../libvirt/src/qemu/qemu_capabilities.c: In function
‘virQEMUCapsProbeQMPSchemaEbpf’:
../../../libvirt/src/qemu/qemu_capabilities.c:5573:5: error: ISO C90 forbids
mixed declarations and code [-Werror=declaration-after-statement]
5573 | virJSONValue *requestSchema = virHashLookup(schema, "request-ebpf");
| ^~~~
../../../libvirt/src/qemu/qemu_capabilities.c:5577:5: error: ISO C90 forbids
mixed declarations and code [-Werror=declaration-after-statement]
5577 | const char *argType = virJSONValueObjectGetString(requestSchema,
"arg-type");
| ^
../../../libvirt/src/qemu/qemu_capabilities.c:5581:5: error: ISO C90 forbids
mixed declarations and code [-Werror=declaration-after-statement]
5581 | virJSONValue *argSchema = virHashLookup(schema, argType);
| ^~~~
../../../libvirt/src/qemu/qemu_capabilities.c:5586:5: error: ISO C90 forbids
mixed declarations and code [-Werror=declaration-after-statement]
5586 | virJSONValue *members = virJSONValueObjectGetArray(argSchema,
"members");
| ^~~~
../../../libvirt/src/qemu/qemu_capabilities.c:5591:5: error: ISO C90 forbids
mixed declarations and code [-Werror=declaration-after-statement]
5591 | virJSONValue *idSchema = NULL;
| ^~~~
../../../libvirt/src/qemu/qemu_capabilities.c:5601:5: error: ISO C90 forbids
mixed declarations and code [-Werror=declaration-after-statement]
5601 | const char *ebpfIds = virJSONValueObjectGetString(idSchema, "type");
| ^
../../../libvirt/src/qemu/qemu_capabilities.c:5606:5: error: ISO C90 forbids
mixed declarations and code [-Werror=declaration-after-statement]
5606 | virJSONValue *ebpfIdsArray =
virJSONValueObjectGetArray(ebpfIdsSchema, "values");
| ^~~~
../../../libvirt/src/qemu/qemu_capabilities.c:5611:5: error: ISO C90 forbids
mixed declarations and code [-Werror=declaration-after-statement]
5611 | virJSONValue *id = NULL;
| ^~~~
cc1: all warnings being treated as errors
After the compile error is fixed there are further errors caught by
syntax-check.
> @@ -789,6 +790,9 @@ struct _virQEMUCaps {
> virQEMUCapsAccel kvm;
> virQEMUCapsAccel hvf;
> virQEMUCapsAccel tcg;
> +
> +/* Hash of ebpf objects virQEMUEbpfOBjectData */
> +GHashTable *ebpfObjects;
> };
>
> struct virQEMUCapsSearchData {
> @@ -796,6 +800,18 @@ struct virQEMUCapsSearchData {
> const char *binaryFilter;
> };
>
> +struct virQEMUEbpfOBjectData {
> +void *ebpfData;
> +size_t ebpfSize;
> +};
> +
> +void virQEMUEbpfOBjectDataDestroy(gpointer data) {
> +if (!data)
> +return;
> +struct virQEMUEbpfOBjectData *object = data;
> +g_free(object->ebpfData);
> +g_free(data);
> +}
As noted, store the program as the base64 string throughout libvirt,
none of the above will be needed.
>
> static virClass *virQEMUCapsClass;
> static void virQEMUCapsDispose(void *obj);
> @@ -836,6 +852,19 @@ const char *virQEMUCapsArchToString(virArch arch)
> }
>
>
> +const void *
> +virQEMUCapsGetEbpf(virQEMUCaps *qemuCaps, const char *id, size_t *size)
> +{
> +struct virQEMUEbpfOBjectData *object =
> virHashLookup(qemuCaps->ebpfObjects, id);
> +
> +if (!object)
> +return NULL;
> +
> +*size = object->ebpfSize;
> +return object->ebpfData;
> +}
> +
> +
> /* Checks whether a domain with @guest arch can run
Re: [RFC PATCH 1/4] qemu_monitor: Added QEMU's "request-ebpf" support.
On Mon, Oct 09, 2023 at 09:16:11 +0300, Andrew Melnychenko wrote:
> Added code for monitor and monitor_json.
> The "request-ebpf" return's eBPF binary object encoded in base64.
> The function qemuMonitorGetEbpf() returns a decoded blob.
>
> QEMU provides eBPF that can be loaded and passed to it from Libvirt.
> QEMU requires exact eBPF program/maps, so it can be retrieved using QAPI.
> To load eBPF program - administrative capabilities are required, so Libvirt
> may load it and pass it to the QEMU instance.
> For now, there is only "RSS"(Receive Side Scaling) for virtio-net eBPF
> program and maps.
Please wrap the text in the commit message to the usual line length.
Since it's just explanation, the sentences can be neatly wrapped. Long
lines in commit message are acceptable only if it's impossible to split
them or splitting them would destroy the content (e.g. do not split
pasted output of commands).
> Signed-off-by: Andrew Melnychenko
> ---
> src/qemu/qemu_monitor.c | 23 +++
> src/qemu/qemu_monitor.h | 3 +++
> src/qemu/qemu_monitor_json.c | 21 +
> src/qemu/qemu_monitor_json.h | 3 +++
> 4 files changed, 50 insertions(+)
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 320729f067..07596c78ee 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -4512,3 +4512,26 @@ qemuMonitorGetStatsByQOMPath(virJSONValue *arr,
>
> return NULL;
> }
See coding style problems mentioned in reply to cover letter.
> +
> +void *
> +qemuMonitorGetEbpf(qemuMonitor *mon, const char *ebpfName, size_t *size)
> +{
> +QEMU_CHECK_MONITOR_NULL(mon);
Fails to compile:
In file included from /usr/lib64/glib-2.0/include/glibconfig.h:9,
from /usr/include/glib-2.0/glib/gtypes.h:34,
from /usr/include/glib-2.0/glib/galloca.h:34,
from /usr/include/glib-2.0/glib.h:32,
from /usr/include/glib-2.0/gobject/gbinding.h:30,
from /usr/include/glib-2.0/glib-object.h:24,
from /usr/include/glib-2.0/gio/gioenums.h:30,
from /usr/include/glib-2.0/gio/giotypes.h:30,
from /usr/include/glib-2.0/gio/gio.h:28,
from ../../../libvirt/src/qemu/qemu_monitor.c:27:
../../../libvirt/src/qemu/qemu_monitor.c: In function ‘qemuMonitorGetEbpf’:
/usr/include/glib-2.0/glib/gmacros.h:1347:43: error: ISO C90 forbids mixed
declarations and code [-Werror=declaration-after-statement]
1347 | #define _GLIB_CLEANUP(func) __attribute__((cleanup(func)))
| ^
/usr/include/glib-2.0/glib/gmacros.h:1380:29: note: in expansion of macro
‘_GLIB_CLEANUP’
1380 | #define g_autoptr(TypeName)
_GLIB_CLEANUP(_GLIB_AUTOPTR_FUNC_NAME(TypeName))
_GLIB_AUTOPTR_TYPENAME(TypeName)
| ^
../../../libvirt/src/qemu/qemu_monitor.c:4520:5: note: in expansion of macro
‘g_autoptr’
4520 | g_autoptr(virJSONValue) reply = NULL;
| ^
cc1: all warnings being treated as errors
Make sure your code compiles and passes test suite after every single
patch per our coding guidelines.
> +g_autoptr(virJSONValue) reply = NULL;
> +const char *ebpfBase64 = NULL;
> +void *ebpfObject = NULL;
> +
> +if (ebpfName == NULL || size == NULL)
You are mixing returns which do not set a libvirt error ...
> +return NULL;
> +
> +reply = qemuMonitorJSONGetEbpf(mon, ebpfName);
> +
> +if (reply == NULL)
... with those that do. This is not a good idea as the caller can't
distinguish easily whether it's an error or desired NULL value. We don't
allow that.
> +return NULL;
> +
> +ebpfBase64 = virJSONValueObjectGetString(reply, "object");
> +
> +ebpfObject = g_base64_decode(ebpfBase64, size);
I'd very strongly prefer that this is stored in the base64 form inside
libvirt and decoded just before use. Storing strings is simpler, doesn't
require you to pass around the 'size' needlessly and simplifies the XML
formatter/parser in capabilities.
Also please move all extraction of data from the monitor reply to the
JSON command. The qemu_monitor.c wrapper at this point should simply
pass through. It might eventually be completely removed as it serves no
purpose once the HMP monitor support was deleted. (And before that it'd
be semantically wrong because you pass the JSON out of the QMP
function).
> +
> +return ebpfObject;
> +}
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 6c590933aa..15f32f105c 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -1579,3 +1579,6 @@ qemuMonitorExtractQueryStats(virJSONValue *info);
> virJSONValue *
> qemuMonitorGetStatsByQOMPath(virJSONValue *arr,
> char *qom_path);
> +
> +void *
> +qemuMonitorGetEbpf(qemuMonitor *mon, const char *ebpfName, size_t *size);
> diff --git
Re: [RFC PATCH 0/4] Added virtio-net RSS with eBPF support.
On Mon, Oct 09, 2023 at 10:15:34 +0200, Peter Krempa wrote:
> On Mon, Oct 09, 2023 at 09:16:10 +0300, Andrew Melnychenko wrote:
> > This series of rfc patches adds support for loading the RSS eBPF program
> > and passing it to the QEMU.
> > Comments and suggestions would be useful.
> >
> > QEMU with vhost may work with RSS through eBPF. To load eBPF,
> > the capabilities required that Libvirt may provide.
> > eBPF program and maps may be unique for particular QEMU and
> > Libvirt retrieves eBPF through qapi.
> > For now, there is only "RSS" eBPF object in QEMU, in the future,
> > there may be another one(g.e. network filters).
> > That's why in Libvirt added logic to load and store any
> > eBPF object that QEMU provides using qapi schema.
> >
> > For virtio-net RSS, the document has not changed.
> > ```
> >
> >
> >
> >
> > ```
> >
> > Simplified routine for RSS:
> > * Libvirt retrieves eBPF "RSS" and load it.
> > * Libvirt passes file descriptors to virtio-net with property
> > "ebpf_rss_fds" ("rss" property should be "on" too).
> > * if fds was provided - QEMU using eBPF RSS implementation.
> > * if fds was not provided - QEMU tries to load eBPF RSS in own context and
> > use it.
> > * if eBPF RSS was not loaded - QEMU uses "in-qemu" RSS(vhost not
> > supported).
>
> Hi,
>
Also what is the status of the qemu patches, specifically the QMP
interface? I see that the patch was in v1 of Jasons 'Net patches' pull
request in September, but was dropped in v2 for some reason.
Specifically our policy is that a feature depending on new qemu
interfaces can be merged to libvirt only after the code was comitted to
qemu.
We do not require that it is released, in fact we follow the development
cycle with the the "qemu capabilities" updates done throughout the the
dev cycle.
Once the qemu code is merged I can do the required update of the
capability test output (tests/qemucapabilitiesdata/...replies) output
file based on your patch so that churn and your manual input is
minimized.
Re: [RFC PATCH 0/4] Added virtio-net RSS with eBPF support.
On Mon, Oct 09, 2023 at 09:16:10 +0300, Andrew Melnychenko wrote:
> This series of rfc patches adds support for loading the RSS eBPF program and
> passing it to the QEMU.
> Comments and suggestions would be useful.
>
> QEMU with vhost may work with RSS through eBPF. To load eBPF,
> the capabilities required that Libvirt may provide.
> eBPF program and maps may be unique for particular QEMU and
> Libvirt retrieves eBPF through qapi.
> For now, there is only "RSS" eBPF object in QEMU, in the future,
> there may be another one(g.e. network filters).
> That's why in Libvirt added logic to load and store any
> eBPF object that QEMU provides using qapi schema.
>
> For virtio-net RSS, the document has not changed.
> ```
>
>
>
>
> ```
>
> Simplified routine for RSS:
> * Libvirt retrieves eBPF "RSS" and load it.
> * Libvirt passes file descriptors to virtio-net with property "ebpf_rss_fds"
> ("rss" property should be "on" too).
> * if fds was provided - QEMU using eBPF RSS implementation.
> * if fds was not provided - QEMU tries to load eBPF RSS in own context and
> use it.
> * if eBPF RSS was not loaded - QEMU uses "in-qemu" RSS(vhost not supported).
Hi,
please note that in my review following this mail I'll be mostly
commenting about general problems, coding style problems and the
capabilies probing an caching.
I'm not familiar with what this feature is supposed to be doing as I'm
involved more with storage and I maintain the qemu capability code.
Thus input of others may be needed in terms of the actual feature.
> meson.build | 6 ++
> meson_options.txt| 1 +
> src/qemu/meson.build | 1 +
> src/qemu/qemu_capabilities.c | 181 +++
> src/qemu/qemu_capabilities.h | 4 +
> src/qemu/qemu_command.c | 53 ++
> src/qemu/qemu_domain.c | 4 +
> src/qemu/qemu_domain.h | 3 +
> src/qemu/qemu_interface.c| 42
> src/qemu/qemu_interface.h| 4 +
> src/qemu/qemu_monitor.c | 23 +
> src/qemu/qemu_monitor.h | 3 +
> src/qemu/qemu_monitor_json.c | 21
> src/qemu/qemu_monitor_json.h | 3 +
During a quick look at the patches I've noticed few repeated coding
style problems.
Note that our prefered function declaration/definition header is
virReturnType *
virFunctionName(virFirstArg argname,
virSecondArg *argval,
virEtc etc)
{
}
We also keep two line spacing between functions.
Additionally we also require C90-style variable declarations (variable
declaration must not mix with code inside a block).
Other comments inline.
Re: [PATCH] qemu: add 'media=cdrom' attribute for usb CDROM
On Sat, Oct 07, 2023 at 16:12:09 +0800, Minglei Liu wrote:
> From: "minglei.liu"
>
> In commit 1328a83, the 'media=cdrom' attribute was removed from -drive.
> However, this attribute is still essential for usb cdrom and is still
> supported in qemu 8.1.1. Therefore, we need to reintroduce this attribute
> for usb cdrom.
This just states that it _is_ needed but not why. Please add
justification next time.
Your patch is also missing ceritifcation that it conforms with the
Developer Certificate of origin:
https://www.libvirt.org/hacking.html#developer-certificate-of-origin
> ---
> src/qemu/qemu_command.c| 7 +++
> .../disk-cdrom-bus-other.x86_64-latest.args| 2 +-
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 8a7b80719f..42f3f8f740 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1705,6 +1705,13 @@ qemuBuildDriveStr(virDomainDiskDef *disk)
>
> virBufferAsprintf(, "if=sd,index=%d", virDiskNameToIndex(disk->dst));
>
> +/* While this is a frontend attribute, it only makes sense to be used
> when
> + * legacy -drive is used. In modern qemu the 'ide-cd' or 'scsi-cd' are
> used.
> + * currently only usb cdrom need this attribute */
> +if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM &&
> +disk->bus == VIR_DOMAIN_DISK_BUS_USB)
> +virBufferAddLit(, ",media=cdrom");
This code path is exclusively used for '-drive if=sd'. All other code
paths use -blockdev. Thus this hunk and it's explanation makes no sense.
This will never be executed for USB cdroms.
If you want to add USB cdrom device frontend properties you need to
modify qemuBuildDiskDeviceProps.
Please note though that the 'usb-storage' device does not have a 'media'
parameter or anything similar.
> +
> if (disk->src->readonly)
> virBufferAddLit(, ",readonly=on");
>
> diff --git a/tests/qemuxml2argvdata/disk-cdrom-bus-other.x86_64-latest.args
> b/tests/qemuxml2argvdata/disk-cdrom-bus-other.x86_64-latest.args
> index de5fa083d8..38093423cf 100644
> --- a/tests/qemuxml2argvdata/disk-cdrom-bus-other.x86_64-latest.args
> +++ b/tests/qemuxml2argvdata/disk-cdrom-bus-other.x86_64-latest.args
> @@ -27,7 +27,7 @@
> XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
> -no-shutdown \
> -boot strict=on \
> -device
> '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \
> --blockdev
> '{"driver":"file","filename":"/root/boot.iso","node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}'
> \
> +-blockdev
> '{"driver":"file","filename":"/root/boot.iso","node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap","media":"cdrom"}'
> \
> -blockdev
> '{"node-name":"libvirt-2-format","read-only":true,"driver":"raw","file":"libvirt-2-storage"}'
> \
And this makes no sense either. You are modifying a test output file
which will not be formatted like this by the code. In fact with this
patch the test suite will fail.
Also -blockdev is the device backend so the placement doesn't make any
sense either.
For the next version please add a justification (description what the
actual problem is) in the first place. Do not attempt to modify anything
'-drive' related. Those code paths are deprecated and we don't add new
features for them. As noted, nowadays it's used only for disk bus='sd'.a
Also make sure you run the test-suite before posting patches
Re: [RFC: vf-token 5/5] qemu: validate and generate vf-token on command line
On Thu, Oct 05, 2023 at 16:05:04 -0700, Vivek Kashyap wrote:
> Introduce a validation function for vf-token support in qemu
> and generate vf-token device attribute in qmeu command line.
>
> Signed-off-by: Vivek Kashyap
> ---
> src/qemu/qemu_command.c | 27 ---
> src/qemu/qemu_validate.c | 19 +++
> 2 files changed, 43 insertions(+), 3 deletions(-)
Missing qemuxml2argvtest and qemuxml2xmtest additions, which are
absolutely required for anything that adds/changes the commandline syntax.
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 8a7b80719f..91d7836f5c 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1979,7 +1979,6 @@ qemuBuildZPCIDevProps(virDomainDeviceInfo *dev)
> return props;
> }
>
> -
Spurious whitespace change. As noted we do 2 lines between functions.
> static int
> qemuCommandAddExtDevice(virCommand *cmd,
> virDomainDeviceInfo *dev,
> @@ -4729,6 +4728,8 @@ qemuBuildPCIHostdevDevProps(const virDomainDef *def,
> pcisrc->addr.slot,
> pcisrc->addr.function);
> const char *failover_pair_id = NULL;
> +g_autofree char *token = NULL;
> +int ret = 0;
>
> /* caller has to assign proper passthrough backend type */
> switch (pcisrc->backend) {
> @@ -4755,13 +4756,33 @@ qemuBuildPCIHostdevDevProps(const virDomainDef *def,
> teaming->persistent)
> failover_pair_id = teaming->persistent;
>
> -if (virJSONValueObjectAdd(,
> +if ((dev->info->pciAddrExtFlags & VIR_PCI_ADDRESS_EXTENSION_VFTOKEN) &&
> + pcisrc->addr.token.isSet) {
Broken alignment/code style.
> +const unsigned char *uuid = pcisrc->addr.token.uuid;
> +
> +token = g_strdup_printf(VIR_PCI_DEVICE_TOKEN_FMT,
> + uuid[0], uuid[1], uuid[2], uuid[3],
> + uuid[4], uuid[5], uuid[6], uuid[7],
> + uuid[8], uuid[9], uuid[10], uuid[11],
> + uuid[12], uuid[13], uuid[14], uuid[15]);
Broken alignment/code style.
> +
> +ret = virJSONValueObjectAdd(,
>"s:driver", "vfio-pci",
>"s:host", host,
> + "s:vf-token", token,
>"s:id", dev->info->alias,
>"p:bootindex", dev->info->effectiveBootIndex,
>"S:failover_pair_id", failover_pair_id,
> - NULL) < 0)
> + NULL);
So firstly you break the code style here by your addition.
Secondly there's no need to duplicate the whole virJSONValueObjectAdd
block in the first place as we allow conditional formatting of fields.
You need to fill the 'token' field only optionally what you do, but then
you can unconditionally format it using:
"S:vf-token", token,
argument tuple in virJSONValueObjectAdd. The 'S' modifier formats the
attribute only if the string argument is non-NULL.
> +} else
Trailing whitespace, but this block is not needed ... and in fact not
desired as described above.
> +ret = virJSONValueObjectAdd(,
> + "s:driver", "vfio-pci",
> + "s:host", host,
> + "s:id", dev->info->alias,
> + "p:bootindex", dev->info->effectiveBootIndex,
> + "S:failover_pair_id", failover_pair_id,
> + NULL);
> +if (ret < 0)
> return NULL;
>
> if (qemuBuildDeviceAddressProps(props, def, dev->info) < 0)
Re: [RFC: vf-token 4/5] conf: vf-token parsing and formatting
On Thu, Oct 05, 2023 at 16:05:03 -0700, Vivek Kashyap wrote:
> Introduce XML parsing and formatting of vf-token attribute
>
> Signed-off-by: Vivek Kashyap
> ---
> src/conf/device_conf.c| 31 +--
> src/conf/domain_conf.c| 5 +
> src/conf/schemas/basictypes.rng | 11 +++
> src/conf/schemas/domaincommon.rng | 1 +
> src/libvirt_private.syms | 1 +
> src/util/virpci.c | 5 +
> 6 files changed, 52 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
> index f3d977f2b7..d5ed4ef452 100644
> --- a/src/conf/device_conf.c
> +++ b/src/conf/device_conf.c
> @@ -188,11 +188,20 @@ virDeviceInfoPCIAddressExtensionIsWanted(const
> virDomainDeviceInfo *info)
> virZPCIDeviceAddressIsIncomplete(>addr.pci.zpci);
> }
>
> +bool
> +virDeviceExtensionIsPresent(const virPCIDeviceAddress *pci)
> +{
> +return ((pci->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
> +virZPCIDeviceAddressIsPresent(>zpci)) ||
> +((pci->extFlags & VIR_PCI_ADDRESS_EXTENSION_VFTOKEN) &&
> +pci->token.isSet);
The code alignment is broken here. Also add a function documentation
describing what this function does as it's really not obvious.
> +}
> +
Code style dictates two empty lines between functions.
> bool
> virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info)
> {
> -return (info->addr.pci.extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
> - virZPCIDeviceAddressIsPresent(>addr.pci.zpci);
> +return (info->addr.pci.extFlags != VIR_PCI_ADDRESS_EXTENSION_NONE) &&
> +virDeviceExtensionIsPresent(>addr.pci);
This change is suspicious and should be justified. I'm not going to
comment on whether it's correct or not.
> }
>
> int
> @@ -200,6 +209,7 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
> virPCIDeviceAddress *addr)
> {
> xmlNodePtr zpci;
> +xmlNodePtr token;
>
> memset(addr, 0, sizeof(*addr));
>
> @@ -231,6 +241,10 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
> return -1;
> }
>
> +if ((token = virXMLNodeGetSubelement(node, "vf-token")))
Multi-line condition bodies dictate use of a { } block around it.
> +if (virPCIDeviceTokenParseXML(token, addr) < 0)
> +return -1;
This one is okay though.
> +
> return 0;
> }
>
> @@ -248,6 +262,19 @@ virPCIDeviceAddressFormat(virBuffer *buf,
>addr.function);
> }
>
> +int
> +virPCIDeviceTokenParseXML(xmlNodePtr node,
> + virPCIDeviceAddress *addr)
> +{
> +if (virXMLPropUUID(node, "uuid", VIR_XML_PROP_NONE,
> + addr->token.uuid) < 0)
virXMLPropUUID parses into a buffer which is not a string (NUL-byte
terminated) but a byte array ...
> +return -1;
> +
> +addr->token.isSet = 1;
> +
> +return 0;
> +}
> +
Two empty lines between functions.
> int
> virCCWDeviceAddressParseXML(xmlNodePtr node,
> virCCWDeviceAddress *addr)
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 4435ee2ad4..cceb1d3b83 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5389,6 +5389,11 @@ virDomainDeviceInfoFormat(virBuffer *buf,
>info->addr.pci.zpci.uid.value,
>info->addr.pci.zpci.fid.value);
> }
> +if (virPCIVFIOTokenIDIsPresent(>addr.pci.token)) {
> +virBufferAsprintf(,
> + "\n",
> + info->addr.pci.token.uuid);
... but here you use a *string* formatting function. So this will either
not print any reasonable output or potentially crash if the UUID
contains no NUL-bytes.
You would figure this out if you'd add any tests for this feature which
is in fact required. It is okay to add the tests at the end though.
> +}
> break;
>
> case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE:
> diff --git a/src/conf/schemas/basictypes.rng b/src/conf/schemas/basictypes.rng
> index 26eb538077..bf2c30dd18 100644
> --- a/src/conf/schemas/basictypes.rng
> +++ b/src/conf/schemas/basictypes.rng
> @@ -138,6 +138,17 @@
>
>
>
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
>
>
>
> diff --git a/src/conf/schemas/domaincommon.rng
> b/src/conf/schemas/domaincommon.rng
> index a26986b5ce..b228a3ca73 100644
> --- a/src/conf/schemas/domaincommon.rng
> +++ b/src/conf/schemas/domaincommon.rng
> @@ -7034,6 +7034,7 @@
>
>
>
> +
>
>
>
Any XML addition _must_ come with correspondign documentation update.
Please document the new flag and how it's supposed to be used in
docs/formatdomain.rst.
> diff --git
Re: [RFC: vf-token 3/5] conf: vf-token flag
On Thu, Oct 05, 2023 at 16:05:02 -0700, Vivek Kashyap wrote: > Define PCI address extension flag for vf-token > > Signed-off-by: Vivek Kashyap > --- > src/conf/device_conf.h | 3 +++ > src/conf/domain_addr.h | 1 + > src/qemu/qemu_domain_address.c | 3 +++ > 3 files changed, 7 insertions(+) > > diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h > index a83377417a..29e03baa99 100644 > --- a/src/conf/device_conf.h > +++ b/src/conf/device_conf.h > @@ -188,6 +188,9 @@ bool virDeviceInfoPCIAddressExtensionIsPresent(const > virDomainDeviceInfo *info); > int virPCIDeviceAddressParseXML(xmlNodePtr node, > virPCIDeviceAddress *addr); > > +int virPCIDeviceTokenParseXML(xmlNodePtr node, > +virPCIDeviceAddress *addr); Broken alignment of the second line. And same as in 1/5 the function is not used here so don't define it in this patch.
Re: [RFC: vf-token 2/5] qemu: vf-token capability
On Thu, Oct 05, 2023 at 16:05:01 -0700, Vivek Kashyap wrote:
> Introduce qemu capability for vf-token
>
> Signed-off-by: Vivek Kashyap
> ---
> src/qemu/qemu_capabilities.c | 3 +++
> src/qemu/qemu_capabilities.h | 1 +
> 2 files changed, 4 insertions(+)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 3a1bfbf74d..f84c4df8db 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -698,6 +698,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
>/* 450 */
>"run-with.async-teardown", /*
> QEMU_CAPS_RUN_WITH_ASYNC_TEARDOWN */
>"virtio-blk-vhost-vdpa", /*
> QEMU_CAPS_DEVICE_VIRTIO_BLK_VHOST_VDPA */
> + "vf-token", /* QEMU_CAPS_VFIO_VFTOKEN */
> );
>
>
> @@ -1384,6 +1385,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[]
> = {
> { "virtio-crypto-device", QEMU_CAPS_DEVICE_VIRTIO_CRYPTO },
> { "cryptodev-backend-lkcf", QEMU_CAPS_OBJECT_CRYPTO_LKCF },
> { "pvpanic-pci", QEMU_CAPS_DEVICE_PANIC_PCI },
> +{ "vf-token", QEMU_CAPS_VFIO_VFTOKEN },
> };
>
>
> @@ -1446,6 +1448,7 @@ static struct virQEMUCapsDevicePropsFlags
> virQEMUCapsDevicePropsVirtioSCSI[] = {
> };
>
> static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVfioPCI[] = {
> +{ "vf-token", QEMU_CAPS_VFIO_VFTOKEN, NULL },
Extra trailing whitespace.
> };
Also the 'vf-token' field was added in qemu-8.1 thus you are missing the
appropriate test file updates.
Also the whitespace error would be caught by our test suite.
Please always make sure to post patches where our test suite passes
after every single commit:
https://www.libvirt.org/hacking.html#preparing-patches
Notably you'll need to re-generate the 'qemucapabilitiestest' output
files to include the new flag. To do so automatically run:
VIR_TEST_REGENERATE_OUTPUT=1 ./tests/qemucapabilitiestest
from your build directory after you build everything.
Re: [RFC: vf-token 1/5] virpci: Define vf-token
On Thu, Oct 05, 2023 at 16:05:00 -0700, Vivek Kashyap wrote:
> Define the vf-token extension for PCI device
>
> Signed-off-by: Vivek Kashyap
> ---
> src/util/virpci.h | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/src/util/virpci.h b/src/util/virpci.h
> index faca6cf6f9..b4e9e95d88 100644
> --- a/src/util/virpci.h
> +++ b/src/util/virpci.h
> @@ -50,7 +50,15 @@ struct _virZPCIDeviceAddress {
> /* Don't forget to update virPCIDeviceAddressCopy if needed. */
> };
>
> +typedef struct _virPCIDeviceToken virPCIDeviceToken;
> +
> +struct _virPCIDeviceToken {
> +unsigned char uuid[VIR_UUID_BUFLEN];
> +bool isSet;
> +};
> +
> #define VIR_PCI_DEVICE_ADDRESS_FMT "%04x:%02x:%02x.%d"
> +#define VIR_PCI_DEVICE_TOKEN_FMT
> "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x"
>
> /* Represents format of PF's phys_port_name in switchdev mode:
> * 'p%u' or 'p%us%u'. New line checked since value is read from sysfs file.
> @@ -65,6 +73,7 @@ struct _virPCIDeviceAddress {
> virTristateSwitch multi;
> int extFlags; /* enum virPCIDeviceAddressExtensionFlags */
> virZPCIDeviceAddress zpci;
> +virPCIDeviceToken token;
> /* Don't forget to update virPCIDeviceAddressCopy if needed. */
> };
>
> @@ -269,6 +278,8 @@ int virPCIDeviceAddressParse(char *address,
> virPCIDeviceAddress *bdf);
>
> bool virZPCIDeviceAddressIsIncomplete(const virZPCIDeviceAddress *addr);
> bool virZPCIDeviceAddressIsPresent(const virZPCIDeviceAddress *addr);
> +bool virPCIVFIOTokenIDIsPresent(const virPCIDeviceToken *token);
> +bool virDeviceExtensionIsPresent(const virPCIDeviceAddress *pci);
These function definitions should go into the patch that actually
implements the functions themselves.
Re: [RFC: vf-token 0/5] Introduce vf-token when using userspace PF
On Thu, Oct 05, 2023 at 16:04:59 -0700, Vivek Kashyap wrote:
> vf token is set by a vfio-pci based PF driver and it must be known to the
> vfio-pci based VF driver. This vf-token is set by the PF driver before VF
> drivers can access the device. vfio-pci driver and qemu support vf-token.
> This RFC patch series adds support to provide the vf-token (uuid format)
> in the domain XML and to generate the qemu commandline including the
> vf-token.
>
> To support vf-token the new element will be used as follows:
>
>
>
>
>
>
>
>
>
>
>
> The generated commandline will include the following:
>
> -device {"driver":"vfio-pci","host":":00:0.1",
> "vf-token":"00112233-4455-6677-8899-aabbccddeeff",
> "id":"hostdev0","bus":"pci.0","addr":"0x1"}
>
> This patch is get feedback on the approach. Will post with add
> documentation and testcases in follow-up.
>
> Vivek Kashyap (5):
> virpci: Define vf-token
> qemu: vf-token capability
> conf: vf-token flag
> conf: vf-token parsing and formatting
> qemu: validate and generate vf-token on command line
Please note that my forthcoming review will not include any comments on
whether this feature itself makes sense or whether the XML desing you
are proposing is reasonable ...
> src/conf/device_conf.c| 31 +--
> src/conf/device_conf.h| 3 +++
> src/conf/domain_addr.h| 1 +
> src/conf/domain_conf.c| 5 +
> src/conf/schemas/basictypes.rng | 11 +++
> src/conf/schemas/domaincommon.rng | 1 +
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_capabilities.c | 3 +++
> src/qemu/qemu_capabilities.h | 1 +
> src/qemu/qemu_command.c | 27 ---
> src/qemu/qemu_domain_address.c| 3 +++
> src/qemu/qemu_validate.c | 19 +++
> src/util/virpci.c | 5 +
> src/util/virpci.h | 11 +++
> 14 files changed, 117 insertions(+), 5 deletions(-)
This is mostly because the patches do not contain any changes to
documentation that would explain it and I'm not familiar with what the
feature is supposed to do. Thus my comments will be purely for the code
itself and a further review will be required.
Re: [libvirt PATCH v3 00/10] external snapshot revert fixes
On Mon, Sep 18, 2023 at 15:29:17 +0200, Pavel Hrdina wrote: > This fixes reverting external snapshots to not error out in cases where > it should work and makes it correctly load the memory state when > reverting to snapshot of running VM. > > This discards v2 completely and makes changes to v1: > > - moves qemuSaveImageStartProcess to qemu_process as > qemuProcessStartWithMemoryState > > - change it to use cookie from memory state file instead of > using cookie from snapshot xml > > - comments improvements > > - introduces new helpers to start and stop decompression process > > - reintroduces capability > > Pavel Hrdina (10): > qemu_saveimage: extract starting process to qemuSaveImageStartProcess > qemu_saveimage: introduce helpers to decompress memory state file > qemu_saveimage: move qemuSaveImageStartProcess to qemu_process > qemuProcessStartWithMemoryState: allow setting reason for audit log > qemuProcessStartWithMemoryState: add snapshot argument > qemuProcessStartWithMemoryState: make it possible to use without data > qemu_snapshot: fix reverting external snapshot when not all disks are > included > qemu_snapshot: correctly load the saved memory state file > capabilities: report full external snapshot support > NEWS: document support for reverting external snapshots Reviewed-by: Peter Krempa
Re: [libvirt PATCH v3 06/10] qemuProcessStartWithMemoryState: make it possible to use without data
On Mon, Sep 18, 2023 at 15:29:23 +0200, Pavel Hrdina wrote: > When used with internal snapshots there is no memory state file so we > have no data to load and decompression is not needed. > > Signed-off-by: Pavel Hrdina > --- > src/qemu/qemu_process.c | 23 +-- > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index c8430bf7b7..f96918073f 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -8165,7 +8165,7 @@ qemuProcessStart(virConnectPtr conn, > * @fd: FD pointer of memory state file > * @path: path to memory state file > * @snapshot: internal snapshot to load when starting QEMU process or NULL > - * @data: data from memory state file > + * @data: data from memory state file or NULL > * @asyncJob: type of asynchronous job > * @start_flags: flags to start QEMU process with > * @reason: audit log reason > @@ -8175,7 +8175,8 @@ qemuProcessStart(virConnectPtr conn, > * is correctly decompressed so it can be loaded by QEMU process. > * > * When reverting to internal snapshot callers needs to pass @snapshot as > well > - * correctly start QEMU process. > + * correctly start QEMU process. In addition there is no memory state file so > + * it's safe to pass NULL as @data. This does not fully address my comment from previous patch. It says that @data 'can' be NULL, not that it MUST be inull if @snapshot is non-null.
Re: [libvirt PATCH v3 05/10] qemuProcessStartWithMemoryState: add snapshot argument
On Mon, Sep 18, 2023 at 15:29:22 +0200, Pavel Hrdina wrote: > When called from snapshot code we will need to pass snapshot object in > order to make internal snapshots work correctly. > > Signed-off-by: Pavel Hrdina > --- > src/qemu/qemu_process.c | 9 - > src/qemu/qemu_process.h | 1 + > src/qemu/qemu_saveimage.c | 2 +- > 3 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index d414a40fd5..c8430bf7b7 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -8164,6 +8164,7 @@ qemuProcessStart(virConnectPtr conn, > * @vm: domain object > * @fd: FD pointer of memory state file > * @path: path to memory state file > + * @snapshot: internal snapshot to load when starting QEMU process or NULL > * @data: data from memory state file > * @asyncJob: type of asynchronous job > * @start_flags: flags to start QEMU process with > @@ -8173,6 +8174,11 @@ qemuProcessStart(virConnectPtr conn, > * Start VM with existing memory state. Make sure that the stored memory > state > * is correctly decompressed so it can be loaded by QEMU process. > * > + * When reverting to internal snapshot callers needs to pass @snapshot as > well > + * correctly start QEMU process. It doesn't mention that callers must not pass the other state-related parameters as both methods can't be used at once. > + * > + * When restoring VM from saved image @snapshot needs to be NULL. This is only an implication (one way) that says that with a saveimage @snapshot must be null, but not that it's also the other way around.
Re: [PATCH 08/31] configure: ensure dependency for cross-compile setup
On Mon, 25 Sept 2023 at 17:45, Alex Bennée wrote: > > > Paolo Bonzini writes: > > > On 9/25/23 16:48, Alex Bennée wrote: > >> If we update configure we should make sure we regenerate all the > >> compiler details. We should also ensure those details are upto date > >> before building the TCG tests. > >> Signed-off-by: Alex Bennée > >> --- > >> configure | 2 ++ > >> 1 file changed, 2 insertions(+) > >> diff --git a/configure b/configure > >> index e83872571d..a95e0f5767 100755 > >> --- a/configure > >> +++ b/configure > >> @@ -1788,6 +1788,8 @@ for target in $target_list; do > >> echo "HOST_GDB_SUPPORTS_ARCH=y" >> "$config_target_mak" > >> fi > >> + echo "$config_target_mak: configure" >> Makefile.prereqs > > > > This in practice is not adding anything; if "configure" changes then > > Makefile's dependency on config-host.mak will trigger a configure > > rerun anyway. > > > > If you want to add it, you should also add it for other config-*.mak > > files. However, I'd remove this line and just change > > > > -# 1. ensure config-host.mak is up-to-date > > +# 1. ensure config-host.mak is up-to-date. All other config-*.mak > > +# files for subdirectories will be updated as well. > > Peter ran into a mismatch between config-host.mak and > tests/tcg/foo/config-target.mak in his build system so it didn't get > picked up at one point. I did, but looking at the timestamps on the two files, the problem wasn't that one file got updated and not the other: $ grep CONFIG_PLUGIN build/x86/config-host.h #undef CONFIG_PLUGIN $ grep CONFIG_PLUGIN build/x86/tests/tcg/config-host.mak CONFIG_PLUGIN=y e104462:jammy:qemu$ ls -l build/x86/config-host.mak build/x86/tests/tcg/config-host.mak -rw-r--r-- 1 petmay01 petmay01 549 Sep 22 16:38 build/x86/config-host.mak -rw-r--r-- 1 petmay01 petmay01 159 Sep 22 16:38 build/x86/tests/tcg/config-host.mak (both newer than 'configure' itself by about 10 days.) -- PMM
Re: [libvirt PATCH v8 10/37] qemu: add functions to start and stop nbdkit
On Thu, Sep 21, 2023 at 12:50:52 -0500, Jonathon Jongsma wrote:
> On 9/20/23 7:24 AM, Pavel Hrdina wrote:
> > On Thu, Aug 31, 2023 at 04:39:50PM -0500, Jonathon Jongsma wrote:
> > > Add some helper functions to build a virCommand object and run the
> > > nbdkit process for a given virStorageSource.
> > >
> > > Signed-off-by: Jonathon Jongsma
> > > Reviewed-by: Peter Krempa
> > > ---
> > > src/qemu/qemu_nbdkit.c | 250 +
> > > src/qemu/qemu_nbdkit.h | 10 ++
> > > 2 files changed, 260 insertions(+)
[...]
> > > +VIR_DEBUG("Stopping nbdkit process %i", proc->pid);
> > > +virProcessKill(proc->pid, SIGTERM);
> >
> > Coverity complains here that the return value of virProcessKill() is not
> > checked which leads me to a question if we should use
> > virProcessKillPainfully() instead.
> >
> > With the code that is pushed the function qemuNbdkitProcessStop() is
> > called only within the qemu_nbdkit.c for these cases:
> >
> > - in qemuNbdkitProcessRestart() before starting the process again
> >where we do not check if the original process was killed correctly
> >or not,
> >
> > - in qemuNbdkitStopStorageSource() where we check return value of
> >qemuNbdkitProcessStop() but it will always be 0,
> >
> > - in qemuNbdkitProcessStart() as error path where we don't check any
> >return value.
> >
> > To me it seems that the return value qemuNbdkitProcessStop can be changed
> > to void as we always return 0 and use virProcessKillPainfully() or
> > properly pass return value of virProcessKill() and check it for every
> > use of qemuNbdkitProcessStop().
> >
> > Pavel
>
> Good question. In one of my earlier series I had actually used
> virProcessKillPainfully(), but changed it based on a suggestion from Peter
> that it would be bad to kill it painfully if nbdkit was ever used in
> read-write mode. But apparently I forgot to handle a shutdown failure. An
> alternative would be to simply refuse to use nbdkit if the user requests
> read-write mode.
We can use the same algorithm as with the qemu process where we first
issue SIGTERM, thus if the process is responsive it can execute the
shutdown actions. Otherwise it'll get SIGKILL right after.
Re: [libvirt PATCH] qemu_nbdkit: fix possible null dereference
On Wed, Sep 20, 2023 at 14:01:29 +0200, Pavel Hrdina wrote: > Function virGetConnectSecret() can return NULL so we need to check it > since in virSecretGetSecretString() it gets dereferenced. > > Reported-by: coverity > Signed-off-by: Pavel Hrdina > --- > src/qemu/qemu_nbdkit.c | 3 +++ > 1 file changed, 3 insertions(+) Reviewed-by: Peter Krempa
