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

[Pavel Hrdina]

On Wed, Apr 27, 2016 at 01:30:38PM +0200, Martin Kletzander wrote:
> 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.

And this is my bad :) I totally forget that we call devicePostParse during
device hotplug.

> 
> >>
> >> >> +}
> >> >> +
> >> >> +
> >> >>  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.

Right, few lines above we also remove some stuff from definition but in those
cases it's safe to remove them.  This complicate things a little bit because
while formatting we don't have any driver specific code available.  I think that
you have to add boolean to indicate, that this is auto-generated and probably
save it also into status XML and based in this boolean it would be easy to not
format it.