Re: [PATCH v2 00/10] qemu: support renaming domains with snapshots/checkpoints
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 <danielhb413(a)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