Hello,
Sorry for not keeping communication on list, forgot to click reply to button. please take
Thank you and appologize for bringing inconvenience.
Best,
Jiatong Shen
At 2023-02-20 23:23:09, "Peter Krempa" <pkrempa@xxxxxxxxxx> wrote: >On Mon, Feb 20, 2023 at 10:57:55 +0800, jshen28 wrote: >> From: ushen <yshxxsjt715@xxxxxxxxx> > >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 = "" >> + } 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; >> + }
