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

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(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. > 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.