[libvirt] [PATCH 2/2] qemu: Allow seamless migration for domains with multiple graphics

[Martin Kletzander]

On 07/03/2013 02:11 PM, Jiri Denemark wrote:
> On Wed, Jul 03, 2013 at 12:10:23 +0100, Daniel Berrange wrote:
>> On Tue, Jul 02, 2013 at 03:46:01PM +0200, Martin Kletzander wrote:
>>> Since commit 23e8b5d8, the code is refactored in a way that supports
>>> domains with multiple graphics elements and commit 37b415200 allows
>>> starting such domains.  However none of those commits take migration
>>> into account.  Even though qemu doesn't support relocation for
>>> anything else than VNC and for no more than one graphics, there is no
>>> reason to hardcode one graphics into this part of the code as well.
>>
>> I think you mean  s/VNC/SPICE/ here.
>>
>>>
>>> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>>> ---
>>>  src/qemu/qemu_migration.c | 29 ++++++++++++++++++-----------
>>>  1 file changed, 18 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>>> index 69d5398..d1a86b7 100644
>>> --- a/src/qemu/qemu_migration.c
>>> +++ b/src/qemu/qemu_migration.c
>>> @@ -427,19 +427,21 @@ qemuMigrationCookieAddGraphics(qemuMigrationCookiePtr mig,
>>>                                 virQEMUDriverPtr driver,
>>>                                 virDomainObjPtr dom)
>>>  {
>>> +    size_t i = 0;
>>> +
>>>      if (mig->flags & QEMU_MIGRATION_COOKIE_GRAPHICS) {
>>>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>>                         _("Migration graphics data already present"));
>>>          return -1;
>>>      }
>>>
>>> -    if (dom->def->ngraphics == 1 &&
>>> -        (dom->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC ||
>>> -         dom->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE)) {
>>> -        if (!(mig->graphics =
>>> -              qemuMigrationCookieGraphicsAlloc(driver, dom->def->graphics[0])))
>>> -            return -1;
>>> -        mig->flags |= QEMU_MIGRATION_COOKIE_GRAPHICS;
>>> +    for (i = 0; i < dom->def->ngraphics; i++) {
>>> +       if (dom->def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
>>> +           if (!(mig->graphics =
>>> +                 qemuMigrationCookieGraphicsAlloc(driver, dom->def->graphics[i])))
>>> +               return -1;
>>> +           mig->flags |= QEMU_MIGRATION_COOKIE_GRAPHICS;
>>> +       }
>>>      }
> 
> I think you should break the loop inside the if statement to make it
> clear qemuMigrationCookieGraphicsAlloc won't be called more than once
> causing a memory leak.
> 
> Jirka
> 

You're right, thanks for finding that out.  I fixed this, the commit
message and pushed the series.

Thank you both for review,
Martin