[libvirt] [PATCH 2/2] qemu: Regenerate VNC socket paths

[Martin Kletzander]

On Wed, Apr 27, 2016 at 11:36:19AM +0200, Pavel Hrdina wrote:
>On Wed, Apr 27, 2016 at 09:15:16AM +0200, Martin Kletzander wrote:
>> On Wed, Apr 27, 2016 at 06:24:43AM +0200, Pavel Hrdina wrote:
>> >On Tue, Apr 26, 2016 at 04:34:12PM +0200, Martin Kletzander wrote:
>> >> Similarly to what commit 714080791778 did with some internal paths,
>> >> clear vnc socket paths that were generated by us.  Having such path in
>> >> the definition can cause trouble when restoring the domain.  The path is
>> >> generated to the per-domain directory that contains the domain ID.
>> >> However, that ID will be different upon restoration, so qemu won't be
>> >> able to create that socket because the directory will not be prepared.
>> >>
>> >> Best viewed with '-C'.
>> >>
>> >> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1326270
>> >>
>> >> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>> >> ---
>> >>  src/qemu/qemu_domain.c                             | 21 +++++++++++-
>> >>  .../qemuxml2argv-graphics-vnc-autosocket.args      | 22 ++++++++++++
>> >>  .../qemuxml2argv-graphics-vnc-autosocket.xml       | 34 +++++++++++++++++++
>> >>  .../qemuxml2xmlout-graphics-vnc-autosocket.xml     | 39 ++++++++++++++++++++++
>> >>  tests/qemuxml2xmltest.c                            |  7 ++++
>> >>  5 files changed, 122 insertions(+), 1 deletion(-)
>> >>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-autosocket.args
>> >>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-autosocket.xml
>> >>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-vnc-autosocket.xml
>> >>
>> >> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> >> index a2f981043915..d6f704d6f91b 100644
>> >> --- a/src/qemu/qemu_domain.c
>> >> +++ b/src/qemu/qemu_domain.c
>> >> @@ -1659,10 +1659,26 @@ qemuCanonicalizeMachine(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
>> >>  }
>> >>
>> >>
>> >> +static void
>> >> +qemuDomainCleanupInternalPaths(virDomainDefPtr def, virQEMUDriverConfigPtr cfg)
>> >> +{
>> >> +    size_t i = 0;
>> >> +
>> >> +    for (i = 0; i < def->ngraphics; ++i) {
>> >> +        virDomainGraphicsDefPtr graphics = def->graphics[i];
>> >> +
>> >> +        if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
>> >> +            graphics->data.vnc.socket &&
>> >> +            STRPREFIX(graphics->data.vnc.socket, cfg->libDir))
>> >> +            VIR_FREE(graphics->data.vnc.socket);
>> >> +    }
>> >
>> >I would also move the channel socket cleanup code here so we don't have two
>> >places doing the same thing.
>> >
>>
>> That would introduce another loop over devices and one of the main
>> points of DevicePostParse is not having to do that.
>
>Yes, I know and still I thing that we should have only one place to cleanup
>those socket paths so it could be reused for the migration cleanup.
>

But then we'll have it in two places.  We need to keep it in the
DevicePostParse due to hotplug reasons.

>>
>> >> +}
>> >> +
>> >> +
>> >>  static int
>> >>  qemuDomainDefPostParse(virDomainDefPtr def,
>> >>                         virCapsPtr caps,
>> >> -                       unsigned int parseFlags ATTRIBUTE_UNUSED,
>> >> +                       unsigned int parseFlags,
>> >>                         void *opaque)
>> >>  {
>> >>      virQEMUDriverPtr driver = opaque;
>> >
>> >This isn't enough to fix the managedsave/migration.  The root cause is that we
>> >generate wrong migratable XML.  This cleanups the definition before migratable
>> >XML is created:
>> >
>>
>> It is enough, but I agree that's not the root cause.  Even though this
>> will fix it when migrating to new libvirt from whatever version, we want
>> to be able to do migrate from version with this patch in, so I'll amend that.
>>
>> >diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> >index 2e409d4..dc0b035 100644
>> >--- a/src/qemu/qemu_domain.c
>> >+++ b/src/qemu/qemu_domain.c
>> >@@ -2560,6 +2560,7 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver,
>> >     virCPUDefPtr cpu = NULL;
>> >     virCPUDefPtr def_cpu = def->cpu;
>> >     virDomainControllerDefPtr *controllers = NULL;
>> >+    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>> >     int ncontrollers = 0;
>> >     virCapsPtr caps = NULL;
>> >
>> >@@ -2646,7 +2647,7 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver,
>> >             }
>> >         }
>> >
>> >-
>> >+        qemuDomainCleanupInternalPaths(def, cfg);
>>
>> However, we cannot do that because we would erase that from the live
>> XML.  What we need to do instead is make sure we don't format that path
>> if flags & VIR_DOMAIN_FORMAT_XML_MIGRATABLE is true (or whatever the
>> flag name is).
>>
>> I'll send a v2 with that
>
>If you look closely at the location where I've put that call, you'll see, that
>it's in qemuDomainDefFormatBuf() in "if ((flags & VIR_DOMAIN_XML_MIGRATABLE))"
>condition which is exactly what we want.
>

Oh yep, my bad, but instead of clearing it we should really just not
format it.  Otherwise simple 'virsh dumpxml --migratable ' will delete
it forever.

>Pavel
>
>>
>> >     }
>> >
>> >     ret = virDomainDefFormatInternal(def, driver->caps,
>> >@@ -2662,6 +2663,7 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver,
>> >         def->ncontrollers = ncontrollers;
>> >     }
>> >     virObjectUnref(caps);
>> >+    virObjectUnref(cfg);
>> >     return ret;
>> > }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160427/3e9d8e7c/attachment-0001.sig>