At 2023-02-20 23:23:09, "Peter Krempa" <pkrempa@redhat.com> wrote:
>On Mon, Feb 20, 2023 at 10:57:55 +0800, jshen28 wrote:
>> From: ushen <yshxxsjt715@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;
>> +        }