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

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(a)redhat.com&gt; > >> --- > >> 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.

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

Pavel > > > } > > > > ret = virDomainDefFormatInternal(def, driver->caps, > >@@ -2662,6 +2663,7 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver, > > def->ncontrollers = ncontrollers; > > } > > virObjectUnref(caps); > >+ virObjectUnref(cfg); > > return ret; > > }