[libvirt] [PATCH 0/2] Fix migrations with automatic vnc sockets
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1326270 Martin Kletzander (2): qemu: Unref cfg in qemuDomainDefPostParse qemu: Regenerate VNC socket paths src/qemu/qemu_domain.c | 22 +++++++++++- .../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, 123 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 -- 2.8.1
Introduced by commit 15ad2ecf114d. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_domain.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3b152893a23a..a2f981043915 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1714,6 +1714,7 @@ qemuDomainDefPostParse(virDomainDefPtr def, ret = 0; cleanup: virObjectUnref(qemuCaps); + virObjectUnref(cfg); return ret; } -- 2.8.1
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@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); + } +} + + static int qemuDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps, - unsigned int parseFlags ATTRIBUTE_UNUSED, + unsigned int parseFlags, void *opaque) { virQEMUDriverPtr driver = opaque; @@ -1708,6 +1724,9 @@ qemuDomainDefPostParse(virDomainDefPtr def, qemuDomainDefEnableDefaultFeatures(def); + if (parseFlags & VIR_DOMAIN_DEF_PARSE_INACTIVE) + qemuDomainCleanupInternalPaths(def, cfg); + if (virSecurityManagerVerify(driver->securityManager, def) < 0) goto cleanup; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-autosocket.args b/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-autosocket.args new file mode 100644 index 000000000000..7e1fb6b3717f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-autosocket.args @@ -0,0 +1,22 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-vnc unix:/tmp/lib/domain--1-QEMUGuest1/vnc.socket \ +-vga cirrus diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-autosocket.xml b/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-autosocket.xml new file mode 100644 index 000000000000..fa59c39873a5 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-autosocket.xml @@ -0,0 +1,34 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='vnc' socket='/tmp/lib/domain-99-QEMUGuest1/delete.this.socket'/> + <video> + <model type='cirrus' vram='16384' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-vnc-autosocket.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-vnc-autosocket.xml new file mode 100644 index 000000000000..744053368745 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-vnc-autosocket.xml @@ -0,0 +1,39 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes'/> + <video> + <model type='cirrus' vram='16384' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index f766f4dc521e..62f8f96c2274 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -263,6 +263,7 @@ mymain(void) { int ret = 0; struct testInfo info; + virQEMUDriverConfigPtr cfg = NULL; if (qemuTestDriverInit(&driver) < 0) return EXIT_FAILURE; @@ -774,6 +775,12 @@ mymain(void) DO_TEST("virtio-input"); DO_TEST("virtio-input-passthrough"); + cfg = virQEMUDriverGetConfig(&driver); + cfg->vncAutoUnixSocket = true; + DO_TEST_FULL("graphics-vnc-autosocket", WHEN_INACTIVE, NONE); + cfg->vncAutoUnixSocket = false; + + virObjectUnref(cfg); qemuTestDriverFree(&driver); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; -- 2.8.1
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@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.
+} + + 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: 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); } ret = virDomainDefFormatInternal(def, driver->caps, @@ -2662,6 +2663,7 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver, def->ncontrollers = ncontrollers; } virObjectUnref(caps); + virObjectUnref(cfg); return ret; }
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@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.
+} + + 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
}
ret = virDomainDefFormatInternal(def, driver->caps, @@ -2662,6 +2663,7 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver, def->ncontrollers = ncontrollers; } virObjectUnref(caps); + virObjectUnref(cfg); return ret; }
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@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.
+} + + 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; }
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@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.
+} + + 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.
Pavel
}
ret = virDomainDefFormatInternal(def, driver->caps, @@ -2662,6 +2663,7 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver, def->ncontrollers = ncontrollers; } virObjectUnref(caps); + virObjectUnref(cfg); return ret; }
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@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.
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@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.
participants (2)
-
Martin Kletzander -
Pavel Hrdina