On Mon, Feb 20, 2023 at 10:57:55 +0800, jshen28 wrote: > From: ushen <yshxxsjt715(a)gmail.com&gt; 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; > + }