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

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