[libvirt] [PATCH 2/2] qemu: Regenerate VNC socket paths
Martin Kletzander
mkletzan at redhat.com
Wed Apr 27 12:53:09 UTC 2016
On Wed, Apr 27, 2016 at 02:07:57PM +0200, Pavel Hrdina wrote:
>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.
That's probably the way I'll go because modifying xmlopt for that is
pretty ugly. I'll send a v2 with that.
-------------- 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/21cdcaef/attachment-0001.sig>
More information about the libvir-list
mailing list