Re: [PATCH 1/1] WIP: allow update more disk properties
On Mon, Feb 20, 2023 at 10:57:55 +0800, jshen28 wrote:
From: ushen <yshxxsjt715(a)gmail.com>
You should put your explanation and justification of the patch here
rather than into the cover-letter.
Also you need to comply with the following contributor guideline:
https://www.libvirt.org/hacking.html#developer-certificate-of-origin
---
src/qemu/qemu_block.c | 2 +-
src/qemu/qemu_block.h | 5 +++++
src/qemu/qemu_domain.c | 2 --
src/qemu/qemu_driver.c | 17 +++++++++++++++++
4 files changed, 23 insertions(+), 3 deletions(-)
[...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e9bc0f375d..e5b5fef87e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8239,7 +8239,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDef *disk,
CHECK_STREQ_NULLABLE(product,
"product");
- CHECK_EQ(cachemode, "cache", true);
Note that 'cachemode' actually influences 3 separate caching
properties, one of which is a frontend-device property. Specifically
that is the 'write-cache' property of the frontend device.
This patch as-is can't change the fronted property so it would be
incorrect to allow arbitrary change of the caching mode.
The two others are indeed properties of the image itself thus are
addressed by blockdev-reopen.
See qemuDomainDiskCachemodeFlags
CHECK_EQ(error_policy, "error_policy", true);
CHECK_EQ(rerror_policy, "rerror_policy", true);
CHECK_EQ(iomode, "io", true);
@@ -8267,7 +8266,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDef *disk,
CHECK_EQ(info.bootIndex, "boot order", true);
CHECK_EQ(rawio, "rawio", true);
CHECK_EQ(sgio, "sgio", true);
- CHECK_EQ(discard, "discard", true);
CHECK_EQ(iothread, "iothread", true);
Also note that discard options similarly to the cache mode are copied
over from the disk definition into all backing images' definitions.
Thus changing those actually requires doing a blockdev-reopen on all of
the backing chain.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6154fe9bfe..bcb68d6c66 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6802,6 +6802,7 @@ qemuDomainChangeDiskLive(virDomainObj *vm,
virDomainDiskDef *orig_disk = NULL;
virDomainStartupPolicy origStartupPolicy;
virDomainDeviceDef oldDev = { .type = dev->type };
+ int ret;
if (!(orig_disk = virDomainDiskByTarget(vm->def, disk->dst))) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -6840,6 +6841,22 @@ qemuDomainChangeDiskLive(virDomainObj *vm,
}
dev->data.disk->src = NULL;
+ } else {
+ // reopen disk device with more parameters
+ disk->src->backingStore = orig_disk->src->backingStore;
+ ret = qemuBlockReopenFormat(vm, disk->src, false);
3rd property of qemuBlockReopenFormat is 'virDomainAsyncJob asyncJob'
passing 'false' is wrong.
+ disk->src->backingStore = NULL;
+ if (!ret) {
The return value is not a boolean or pointer thus '!ret' is not how you
are supposed to check it.
Also as noted you'll need to do this for the whole backing chain.
Also customary in our code, on failure we jump out, and just then
process the rest of the success code path ...
+ orig_disk->cachemode = disk->cachemode;
+ orig_disk->src->cachemode = disk->src->cachemode;
+ orig_disk->detect_zeroes = disk->detect_zeroes;
+ orig_disk->src->detect_zeroes = disk->src->detect_zeroes;
+ orig_disk->discard = disk->discard;
+ orig_disk->src->discard = disk->src->discard;
+ disk->src->backingStore = NULL
So this should not be in a block.
+ } else {
This case also leaves 'backingStore' of the original disk assigned in
the definition which will be thrown away. Since you didn't increase
the reference count on failure the backin store would be freed but used
later.
+ return ret;
+ }