On 10.11.2020 10:58, Nikolay Shirokovskiy wrote:
>
>
> On 09.11.2020 16:51, Daniel Henrique Barboza wrote:
>>
>>
>> On 11/3/20 8:59 AM, Nikolay Shirokovskiy wrote:
>>> This is basically just rebase of [1] as it was not get any attention at that
>>> time.
>>>
>>> [1] [PATCH 0/8] qemu: support renaming domains with snapshots/checkpoints
>>> https://www.redhat.com/archives/libvir-list/2020-March/msg00018.html
>>
>> Code LGTM:
>>
>> Reviewed-by: Daniel Henrique Barboza <danielhb...@gmail.com>
>>
>>
>> Shouldn't you add some test cases for this new behavior though? I'm a bit
>> nervous with pushing this upstream without any coverage.
>>
>>
>
> So basically we need to test how qemuDomainRenameCallback works. Currently
> there is no test for this function or virDomainRename. I spent some time
> looking at existing tests that need to mock driver/vm objects and it looks
> like
> it requires a good deal of effort in order to prepare the test in this case.
> At the same time those tests has many inputs so it looks like worth heavy
> preparation. In case of rename the code we test does not depend greatly on
> inputs so may be it does not worth adding such test given heavy preparation we
> have to do.
>
> Of course I give the patch series some manual testing.
>
Thanx for the review!
Pushed with next hunk squashed into the last patch:
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index c831ae6..fef0be6 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -5137,7 +5137,7 @@ qemuMigrationDstPersist(virQEMUDriverPtr driver,
priv->qemuCaps)))
goto error;
- if (!oldDef && qemuDomainNamePathsCleanup(cfg, vmdef->name, false) < 0)
+ if (!oldPersist && qemuDomainNamePathsCleanup(cfg, vmdef->name, false) <
0)
goto error;
if (virDomainDefSave(vmdef, driver->xmlopt, cfg->configDir) < 0 &&
Nikolay
