[libvirt] [PATCH 0/3] qemu: Fix hotplug of guest agent
Martin Kletzander (3): qemuhotplugtest: Allow testing of live data qemu: Move channel path generation out of command creation qemu: Generate channel target paths on hotplug as well src/qemu/qemu_command.c | 25 ++-------- src/qemu/qemu_command.h | 5 +- src/qemu/qemu_domain.c | 19 +++++++ src/qemu/qemu_domain.h | 4 ++ src/qemu/qemu_hotplug.c | 3 ++ src/qemu/qemu_process.c | 12 +++-- tests/qemuhotplugtest.c | 42 ++++++++++++---- .../qemuhotplug-hotplug-base+qemu-agent-detach.xml | 58 ++++++++++++++++++++++ .../qemuhotplug-hotplug-base+qemu-agent.xml | 58 ++++++++++++++++++++++ .../qemuhotplug-qemu-agent-detach.xml | 5 ++ .../qemuhotplugtestdata/qemuhotplug-qemu-agent.xml | 5 ++ 11 files changed, 197 insertions(+), 39 deletions(-) create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+qemu-agent-detach.xml create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+qemu-agent.xml create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-qemu-agent-detach.xml create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-qemu-agent.xml -- 2.8.0
For now, the test was dumping an XML of inactive domain (well, setting the id to '-1' to be precise) when checking the results. This patch enables future additions to test the live XML output as well. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tests/qemuhotplugtest.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 1eb2b6a881f2..2b0de94fb4a6 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -52,6 +52,7 @@ struct qemuHotplugTestData { bool keep; virDomainObjPtr vm; bool deviceDeletedEvent; + bool live; }; static int @@ -177,12 +178,13 @@ static int testQemuHotplugCheckResult(virDomainObjPtr vm, const char *expected, const char *expectedFile, - bool fail) + bool fail, bool live) { char *actual; int ret; - vm->def->id = -1; + if (!live) + vm->def->id = -1; actual = virDomainDefFormat(vm->def, driver.caps, VIR_DOMAIN_DEF_FORMAT_SECURE); if (!actual) @@ -219,6 +221,7 @@ testQemuHotplug(const void *data) const char *const *tmp; bool fail = test->fail; bool keep = test->keep; + bool live = test->live; unsigned int device_parse_flags = 0; virDomainObjPtr vm = NULL; virDomainDeviceDefPtr dev = NULL; @@ -300,14 +303,14 @@ testQemuHotplug(const void *data) } if (ret == 0 || fail) ret = testQemuHotplugCheckResult(vm, result_xml, - result_filename, fail); + result_filename, fail, live); break; case DETACH: ret = testQemuHotplugDetach(vm, dev); if (ret == 0 || fail) ret = testQemuHotplugCheckResult(vm, domain_xml, - domain_filename, fail); + domain_filename, fail, live); break; case UPDATE: @@ -371,7 +374,7 @@ mymain(void) /* wait only 100ms for DEVICE_DELETED event */ qemuDomainRemoveDeviceWaitTime = 100; -#define DO_TEST(file, ACTION, dev, event, fial, kep, ...) \ +#define DO_TEST(file, ACTION, dev, event, fial, kep, liv, ...) \ do { \ const char *my_mon[] = { __VA_ARGS__, NULL}; \ const char *name = file " " #ACTION " " dev; \ @@ -381,25 +384,29 @@ mymain(void) data.fail = fial; \ data.mon = my_mon; \ data.keep = kep; \ + data.live = liv; \ data.deviceDeletedEvent = event; \ if (virtTestRun(name, testQemuHotplug, &data) < 0) \ ret = -1; \ } while (0) #define DO_TEST_ATTACH(file, dev, fial, kep, ...) \ - DO_TEST(file, ATTACH, dev, false, fial, kep, __VA_ARGS__) + DO_TEST(file, ATTACH, dev, false, fial, kep, false, __VA_ARGS__) + +#define DO_TEST_ATTACH_LIVE(file, dev, fial, kep, ...) \ + DO_TEST(file, ATTACH, dev, false, fial, kep, true, __VA_ARGS__) #define DO_TEST_DETACH(file, dev, fial, kep, ...) \ - DO_TEST(file, DETACH, dev, false, fial, kep, __VA_ARGS__) + DO_TEST(file, DETACH, dev, false, fial, kep, false, __VA_ARGS__) #define DO_TEST_ATTACH_EVENT(file, dev, fial, kep, ...) \ - DO_TEST(file, ATTACH, dev, true, fial, kep, __VA_ARGS__) + DO_TEST(file, ATTACH, dev, true, fial, kep, false, __VA_ARGS__) #define DO_TEST_DETACH_EVENT(file, dev, fial, kep, ...) \ - DO_TEST(file, DETACH, dev, true, fial, kep, __VA_ARGS__) + DO_TEST(file, DETACH, dev, true, fial, kep, false, __VA_ARGS__) #define DO_TEST_UPDATE(file, dev, fial, kep, ...) \ - DO_TEST(file, UPDATE, dev, false, fial, kep, __VA_ARGS__) + DO_TEST(file, UPDATE, dev, false, fial, kep, false, __VA_ARGS__) #define QMP_OK "{\"return\": {}}" -- 2.8.0
On 03/30/2016 11:13 AM, Martin Kletzander wrote:
For now, the test was dumping an XML of inactive domain (well, setting the id to '-1' to be precise) when checking the results. This patch enables future additions to test the live XML output as well.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tests/qemuhotplugtest.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 1eb2b6a881f2..2b0de94fb4a6 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -52,6 +52,7 @@ struct qemuHotplugTestData { bool keep; virDomainObjPtr vm; bool deviceDeletedEvent; + bool live; };
static int @@ -177,12 +178,13 @@ static int testQemuHotplugCheckResult(virDomainObjPtr vm, const char *expected, const char *expectedFile, - bool fail) + bool fail, bool live) { char *actual; int ret;
- vm->def->id = -1; + if (!live) + vm->def->id = -1; actual = virDomainDefFormat(vm->def, driver.caps, VIR_DOMAIN_DEF_FORMAT_SECURE); if (!actual) @@ -219,6 +221,7 @@ testQemuHotplug(const void *data) const char *const *tmp; bool fail = test->fail; bool keep = test->keep; + bool live = test->live; unsigned int device_parse_flags = 0; virDomainObjPtr vm = NULL; virDomainDeviceDefPtr dev = NULL; @@ -300,14 +303,14 @@ testQemuHotplug(const void *data) } if (ret == 0 || fail) ret = testQemuHotplugCheckResult(vm, result_xml, - result_filename, fail); + result_filename, fail, live); break;
case DETACH: ret = testQemuHotplugDetach(vm, dev); if (ret == 0 || fail) ret = testQemuHotplugCheckResult(vm, domain_xml, - domain_filename, fail); + domain_filename, fail, live); break;
case UPDATE: @@ -371,7 +374,7 @@ mymain(void) /* wait only 100ms for DEVICE_DELETED event */ qemuDomainRemoveDeviceWaitTime = 100;
-#define DO_TEST(file, ACTION, dev, event, fial, kep, ...) \ +#define DO_TEST(file, ACTION, dev, event, fial, kep, liv, ...) \
"Typically" rather than misspelling "fail", "keep", or "live" one would use an underscore, such as "_fail", "_keep", and "_live" in order to represent a macro argument... Not required for ACK, but nice nonetheless John
do { \ const char *my_mon[] = { __VA_ARGS__, NULL}; \ const char *name = file " " #ACTION " " dev; \ @@ -381,25 +384,29 @@ mymain(void) data.fail = fial; \ data.mon = my_mon; \ data.keep = kep; \ + data.live = liv; \ data.deviceDeletedEvent = event; \ if (virtTestRun(name, testQemuHotplug, &data) < 0) \ ret = -1; \ } while (0)
#define DO_TEST_ATTACH(file, dev, fial, kep, ...) \ - DO_TEST(file, ATTACH, dev, false, fial, kep, __VA_ARGS__) + DO_TEST(file, ATTACH, dev, false, fial, kep, false, __VA_ARGS__) + +#define DO_TEST_ATTACH_LIVE(file, dev, fial, kep, ...) \ + DO_TEST(file, ATTACH, dev, false, fial, kep, true, __VA_ARGS__)
#define DO_TEST_DETACH(file, dev, fial, kep, ...) \ - DO_TEST(file, DETACH, dev, false, fial, kep, __VA_ARGS__) + DO_TEST(file, DETACH, dev, false, fial, kep, false, __VA_ARGS__)
#define DO_TEST_ATTACH_EVENT(file, dev, fial, kep, ...) \ - DO_TEST(file, ATTACH, dev, true, fial, kep, __VA_ARGS__) + DO_TEST(file, ATTACH, dev, true, fial, kep, false, __VA_ARGS__)
#define DO_TEST_DETACH_EVENT(file, dev, fial, kep, ...) \ - DO_TEST(file, DETACH, dev, true, fial, kep, __VA_ARGS__) + DO_TEST(file, DETACH, dev, true, fial, kep, false, __VA_ARGS__)
#define DO_TEST_UPDATE(file, dev, fial, kep, ...) \ - DO_TEST(file, UPDATE, dev, false, fial, kep, __VA_ARGS__) + DO_TEST(file, UPDATE, dev, false, fial, kep, false, __VA_ARGS__)
#define QMP_OK "{\"return\": {}}"
On Thu, Mar 31, 2016 at 08:29:52AM -0400, John Ferlan wrote:
On 03/30/2016 11:13 AM, Martin Kletzander wrote:
For now, the test was dumping an XML of inactive domain (well, setting the id to '-1' to be precise) when checking the results. This patch enables future additions to test the live XML output as well.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tests/qemuhotplugtest.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 1eb2b6a881f2..2b0de94fb4a6 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -371,7 +374,7 @@ mymain(void) /* wait only 100ms for DEVICE_DELETED event */ qemuDomainRemoveDeviceWaitTime = 100;
-#define DO_TEST(file, ACTION, dev, event, fial, kep, ...) \ +#define DO_TEST(file, ACTION, dev, event, fial, kep, liv, ...) \
"Typically" rather than misspelling "fail", "keep", or "live" one would use an underscore, such as "_fail", "_keep", and "_live" in order to represent a macro argument...
Not required for ACK, but nice nonetheless
I didn't want to get to discussions about this so I followed what was already used in this file (and bunch of others as well). If I were to change this, it would go into separate patch anyway, so it doesn't make sense to change in this one, I believe.
Put it into separate function called qemuDomainPrepareChannel() and call it from the new qemuProcessPrepareDomain(). Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 25 +++---------------------- src/qemu/qemu_command.h | 5 ++--- src/qemu/qemu_domain.c | 19 +++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++++ src/qemu/qemu_process.c | 12 ++++++++---- 5 files changed, 36 insertions(+), 29 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 45c5398ad8b9..bb9357e41c3b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8468,8 +8468,7 @@ static int qemuBuildChannelsCommandLine(virLogManagerPtr logManager, virCommandPtr cmd, const virDomainDef *def, - virQEMUCapsPtr qemuCaps, - const char *domainChannelTargetDir) + virQEMUCapsPtr qemuCaps) { size_t i; @@ -8508,22 +8507,6 @@ qemuBuildChannelsCommandLine(virLogManagerPtr logManager, return -1; } - /* - * TODO: Refactor so that we generate this (and onther - * things) somewhere else then where we are building the - * command line. - */ - if (channel->source.type == VIR_DOMAIN_CHR_TYPE_UNIX && - !channel->source.data.nix.path) { - if (virAsprintf(&channel->source.data.nix.path, - "%s/%s", domainChannelTargetDir, - channel->target.name ? channel->target.name - : "unknown.sock") < 0) - return -1; - - channel->source.data.nix.listen = true; - } - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPICEVMC) && channel->source.type == VIR_DOMAIN_CHR_TYPE_SPICEVMC) { /* spicevmc was originally introduced via a -device @@ -9198,8 +9181,7 @@ qemuBuildCommandLine(virConnectPtr conn, virBitmapPtr nodeset, size_t *nnicindexes, int **nicindexes, - const char *domainLibDir, - const char *domainChannelTargetDir) + const char *domainLibDir) { size_t i; char uuid[VIR_UUID_STRING_BUFLEN]; @@ -9340,8 +9322,7 @@ qemuBuildCommandLine(virConnectPtr conn, if (qemuBuildParallelsCommandLine(logManager, cmd, def, qemuCaps) < 0) goto error; - if (qemuBuildChannelsCommandLine(logManager, cmd, def, qemuCaps, - domainChannelTargetDir) < 0) + if (qemuBuildChannelsCommandLine(logManager, cmd, def, qemuCaps) < 0) goto error; if (qemuBuildConsoleCommandLine(logManager, cmd, def, qemuCaps) < 0) diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 7c13d459f8c5..60dc76c45bdb 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -74,10 +74,9 @@ virCommandPtr qemuBuildCommandLine(virConnectPtr conn, virBitmapPtr nodeset, size_t *nnicindexes, int **nicindexes, - const char *domainLibDir, - const char *domainChannelTargetDir) + const char *domainLibDir) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(11) - ATTRIBUTE_NONNULL(17) ATTRIBUTE_NONNULL(18); + ATTRIBUTE_NONNULL(17); /* Generate '-device' string for chardev device */ int diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 403f01e75e46..645e1232d2f9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4639,3 +4639,22 @@ qemuDomainDiskByName(virDomainDefPtr def, return ret; } + +int +qemuDomainPrepareChannel(virDomainChrDefPtr channel, + const char *domainChannelTargetDir) +{ + if (channel->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO && + channel->source.type == VIR_DOMAIN_CHR_TYPE_UNIX && + !channel->source.data.nix.path) { + if (virAsprintf(&channel->source.data.nix.path, + "%s/%s", domainChannelTargetDir, + channel->target.name ? channel->target.name + : "unknown.sock") < 0) + return -1; + + channel->source.data.nix.listen = true; + } + + return 0; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 02c6012c9c87..e893aa17cacd 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -560,4 +560,8 @@ int qemuDomainSetPrivatePaths(char **domainLibDir, virDomainDiskDefPtr qemuDomainDiskByName(virDomainDefPtr def, const char *name); +int +qemuDomainPrepareChannel(virDomainChrDefPtr chr, + const char *domainChannelTargetDir); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 07b9df27397c..6a4fe17125b0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5008,6 +5008,12 @@ qemuProcessPrepareDomain(virConnectPtr conn, goto cleanup; } + for (i = 0; i < vm->def->nchannels; i++) { + if (qemuDomainPrepareChannel(vm->def->channels[i], + priv->channelTargetDir) < 0) + goto cleanup; + } + if (VIR_ALLOC(priv->monConfig) < 0) goto cleanup; @@ -5234,8 +5240,7 @@ qemuProcessLaunch(virConnectPtr conn, qemuCheckFips(), priv->autoNodeset, &nnicindexes, &nicindexes, - priv->libDir, - priv->channelTargetDir))) + priv->libDir))) goto cleanup; if (incoming && incoming->fd != -1) @@ -5656,8 +5661,7 @@ qemuProcessCreatePretendCmd(virConnectPtr conn, priv->autoNodeset, NULL, NULL, - priv->libDir, - priv->channelTargetDir); + priv->libDir); cleanup: return cmd; -- 2.8.0
On 03/30/2016 11:14 AM, Martin Kletzander wrote:
Put it into separate function called qemuDomainPrepareChannel() and call it from the new qemuProcessPrepareDomain().
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 25 +++---------------------- src/qemu/qemu_command.h | 5 ++--- src/qemu/qemu_domain.c | 19 +++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++++ src/qemu/qemu_process.c | 12 ++++++++---- 5 files changed, 36 insertions(+), 29 deletions(-)
[...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 403f01e75e46..645e1232d2f9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4639,3 +4639,22 @@ qemuDomainDiskByName(virDomainDefPtr def,
return ret; }
Two blank lines between functions. ACK with that. John
+ +int +qemuDomainPrepareChannel(virDomainChrDefPtr channel, + const char *domainChannelTargetDir) +{ + if (channel->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO && + channel->source.type == VIR_DOMAIN_CHR_TYPE_UNIX && + !channel->source.data.nix.path) { + if (virAsprintf(&channel->source.data.nix.path, + "%s/%s", domainChannelTargetDir, + channel->target.name ? channel->target.name + : "unknown.sock") < 0) + return -1; + + channel->source.data.nix.listen = true; + } + + return 0; +}
Since commit 714080791778e3dfbd484ccb3953bffd820b8ba9, qemu agent channel cannot be plugged in because we won't generate its path automatically. Let's not only fix that, but also add tests for it so next time it's checked for. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1322210 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_hotplug.c | 3 ++ tests/qemuhotplugtest.c | 15 ++++++ .../qemuhotplug-hotplug-base+qemu-agent-detach.xml | 58 ++++++++++++++++++++++ .../qemuhotplug-hotplug-base+qemu-agent.xml | 58 ++++++++++++++++++++++ .../qemuhotplug-qemu-agent-detach.xml | 5 ++ .../qemuhotplugtestdata/qemuhotplug-qemu-agent.xml | 5 ++ 6 files changed, 144 insertions(+) create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+qemu-agent-detach.xml create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+qemu-agent.xml create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-qemu-agent-detach.xml create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-qemu-agent.xml diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e82dbf5448dc..b7741e15d445 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1565,6 +1565,9 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, goto cleanup; } + if (qemuDomainPrepareChannel(chr, priv->channelTargetDir) < 0) + goto cleanup; + if (qemuAssignDeviceChrAlias(vmdef, chr, -1) < 0) goto cleanup; diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 2b0de94fb4a6..384b7b9592b9 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -98,6 +98,14 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt, (*vm)->def->id = QEMU_HOTPLUG_TEST_DOMAIN_ID; + if (qemuDomainSetPrivatePaths(&priv->libDir, + &priv->channelTargetDir, + "/var/lib/libvirt", + "/var/lib/libvirt/qemu/channel/target", + (*vm)->def->name, + (*vm)->def->id) < 0) + goto cleanup; + ret = 0; cleanup: return ret; @@ -495,6 +503,13 @@ mymain(void) "device_del", QMP_DEVICE_DELETED("scsi0-0-0-5") QMP_OK, "human-monitor-command", HMP("")); + DO_TEST_ATTACH_LIVE("hotplug-base", "qemu-agent", false, true, + "chardev-add", QMP_OK, + "device_add", QMP_OK); + DO_TEST_DETACH("hotplug-base", "qemu-agent-detach", false, false, + "device_del", QMP_OK, + "chardev-remove", QMP_OK); + qemuTestDriverFree(&driver); return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+qemu-agent-detach.xml b/tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+qemu-agent-detach.xml new file mode 100644 index 000000000000..2c449f1d49cd --- /dev/null +++ b/tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+qemu-agent-detach.xml @@ -0,0 +1,58 @@ +<domain type='kvm' id='7'> + <name>hotplug</name> + <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid> + <memory unit='KiB'>4194304</memory> + <currentMemory unit='KiB'>4194304</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <controller type='usb' index='0'> + <alias name='usb'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <alias name='ide'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='scsi' index='0' model='virtio-scsi'> + <alias name='scsi0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'> + <alias name='pci'/> + </controller> + <controller type='virtio-serial' index='0'> + <alias name='virtio-serial0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </controller> + <channel type='unix'> + <source mode='bind' path='/var/lib/libvirt/qemu/channel/target/domain-7-hotplug/org.qemu.guest_agent.0'/> + <target type='virtio' name='org.qemu.guest_agent.0'/> + <alias name='channel0'/> + <address type='virtio-serial' controller='0' bus='0' port='1'/> + </channel> + <input type='mouse' bus='ps2'> + <alias name='input0'/> + </input> + <input type='keyboard' bus='ps2'> + <alias name='input1'/> + </input> + <memballoon model='none'> + <alias name='balloon0'/> + </memballoon> + </devices> + <seclabel type='none' model='none'/> +</domain> diff --git a/tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+qemu-agent.xml b/tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+qemu-agent.xml new file mode 100644 index 000000000000..2c449f1d49cd --- /dev/null +++ b/tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+qemu-agent.xml @@ -0,0 +1,58 @@ +<domain type='kvm' id='7'> + <name>hotplug</name> + <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid> + <memory unit='KiB'>4194304</memory> + <currentMemory unit='KiB'>4194304</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <controller type='usb' index='0'> + <alias name='usb'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <alias name='ide'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='scsi' index='0' model='virtio-scsi'> + <alias name='scsi0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'> + <alias name='pci'/> + </controller> + <controller type='virtio-serial' index='0'> + <alias name='virtio-serial0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </controller> + <channel type='unix'> + <source mode='bind' path='/var/lib/libvirt/qemu/channel/target/domain-7-hotplug/org.qemu.guest_agent.0'/> + <target type='virtio' name='org.qemu.guest_agent.0'/> + <alias name='channel0'/> + <address type='virtio-serial' controller='0' bus='0' port='1'/> + </channel> + <input type='mouse' bus='ps2'> + <alias name='input0'/> + </input> + <input type='keyboard' bus='ps2'> + <alias name='input1'/> + </input> + <memballoon model='none'> + <alias name='balloon0'/> + </memballoon> + </devices> + <seclabel type='none' model='none'/> +</domain> diff --git a/tests/qemuhotplugtestdata/qemuhotplug-qemu-agent-detach.xml b/tests/qemuhotplugtestdata/qemuhotplug-qemu-agent-detach.xml new file mode 100644 index 000000000000..0c3c70a78e89 --- /dev/null +++ b/tests/qemuhotplugtestdata/qemuhotplug-qemu-agent-detach.xml @@ -0,0 +1,5 @@ + <channel type='unix'> + <source mode='bind' path='/var/lib/libvirt/qemu/channel/target/domain-7-hotplug/org.qemu.guest_agent.0'/> + <target type='virtio' name='org.qemu.guest_agent.0'/> + <address type='virtio-serial' controller='0' bus='0' port='1'/> + </channel> diff --git a/tests/qemuhotplugtestdata/qemuhotplug-qemu-agent.xml b/tests/qemuhotplugtestdata/qemuhotplug-qemu-agent.xml new file mode 100644 index 000000000000..f0e90dea3cc2 --- /dev/null +++ b/tests/qemuhotplugtestdata/qemuhotplug-qemu-agent.xml @@ -0,0 +1,5 @@ + <channel type='unix'> + <source mode='bind'/> + <target type='virtio' name='org.qemu.guest_agent.0'/> + <address type='virtio-serial' controller='0' bus='0' port='1'/> + </channel> -- 2.8.0
On 03/30/2016 11:14 AM, Martin Kletzander wrote:
Since commit 714080791778e3dfbd484ccb3953bffd820b8ba9, qemu agent channel cannot be plugged in because we won't generate its path automatically. Let's not only fix that, but also add tests for it so next time it's checked for.
Save some electrons, shorten the commit id
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1322210
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_hotplug.c | 3 ++ tests/qemuhotplugtest.c | 15 ++++++ .../qemuhotplug-hotplug-base+qemu-agent-detach.xml | 58 ++++++++++++++++++++++ .../qemuhotplug-hotplug-base+qemu-agent.xml | 58 ++++++++++++++++++++++ .../qemuhotplug-qemu-agent-detach.xml | 5 ++ .../qemuhotplugtestdata/qemuhotplug-qemu-agent.xml | 5 ++ 6 files changed, 144 insertions(+) create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+qemu-agent-detach.xml create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+qemu-agent.xml create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-qemu-agent-detach.xml create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-qemu-agent.xml
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e82dbf5448dc..b7741e15d445 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1565,6 +1565,9 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, goto cleanup; }
+ if (qemuDomainPrepareChannel(chr, priv->channelTargetDir) < 0) + goto cleanup; + if (qemuAssignDeviceChrAlias(vmdef, chr, -1) < 0) goto cleanup;
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 2b0de94fb4a6..384b7b9592b9 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -98,6 +98,14 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
(*vm)->def->id = QEMU_HOTPLUG_TEST_DOMAIN_ID;
+ if (qemuDomainSetPrivatePaths(&priv->libDir, + &priv->channelTargetDir, + "/var/lib/libvirt", + "/var/lib/libvirt/qemu/channel/target", + (*vm)->def->name, + (*vm)->def->id) < 0)
I believe this overwrites qemuTestDriverInit - since it's a test I'm not concerned about the memory leak, just the processing consistency since you're not really starting a guest, why change the paths? While it's fresh in my mind (still) using /tmp/* in *DriverInit when I was generating patches the domain master secret key file caused problems if I actually tried to check for the existence, especially since qemuProcessPrepareHost is where the qemuProcessMakeDir calls were made to create the directory structure. Perhaps if the tests driver created "tmp/*" paths rather than "/tmp/*" paths that'd work, but is more or less unrelated. I'll wait for your thoughts on this before an official ACK - John
+ goto cleanup; + ret = 0; cleanup: return ret; @@ -495,6 +503,13 @@ mymain(void) "device_del", QMP_DEVICE_DELETED("scsi0-0-0-5") QMP_OK, "human-monitor-command", HMP(""));
+ DO_TEST_ATTACH_LIVE("hotplug-base", "qemu-agent", false, true, + "chardev-add", QMP_OK, + "device_add", QMP_OK); + DO_TEST_DETACH("hotplug-base", "qemu-agent-detach", false, false, + "device_del", QMP_OK, + "chardev-remove", QMP_OK); + qemuTestDriverFree(&driver); return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; } [...]
On Thu, Mar 31, 2016 at 08:35:43AM -0400, John Ferlan wrote:
On 03/30/2016 11:14 AM, Martin Kletzander wrote:
Since commit 714080791778e3dfbd484ccb3953bffd820b8ba9, qemu agent channel cannot be plugged in because we won't generate its path automatically. Let's not only fix that, but also add tests for it so next time it's checked for.
Save some electrons, shorten the commit id
I did that, but I wonder whether you haven't used more electrons by mentioning that than I just shaved off =D
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1322210
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_hotplug.c | 3 ++ tests/qemuhotplugtest.c | 15 ++++++ .../qemuhotplug-hotplug-base+qemu-agent-detach.xml | 58 ++++++++++++++++++++++ .../qemuhotplug-hotplug-base+qemu-agent.xml | 58 ++++++++++++++++++++++ .../qemuhotplug-qemu-agent-detach.xml | 5 ++ .../qemuhotplugtestdata/qemuhotplug-qemu-agent.xml | 5 ++ 6 files changed, 144 insertions(+) create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+qemu-agent-detach.xml create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+qemu-agent.xml create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-qemu-agent-detach.xml create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-qemu-agent.xml
[...]
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 2b0de94fb4a6..384b7b9592b9 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -98,6 +98,14 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
(*vm)->def->id = QEMU_HOTPLUG_TEST_DOMAIN_ID;
+ if (qemuDomainSetPrivatePaths(&priv->libDir, + &priv->channelTargetDir, + "/var/lib/libvirt", + "/var/lib/libvirt/qemu/channel/target", + (*vm)->def->name, + (*vm)->def->id) < 0)
I believe this overwrites qemuTestDriverInit - since it's a test I'm not concerned about the memory leak, just the processing consistency since you're not really starting a guest, why change the paths?
Well, I just needed to initialize it to some string. I can change it to /tmp without any problems. However it is not used anywhere to access anything and we would need to change a lot of tests that make sense currently (as they generate those values -- one of them is even checking that it generates that particular value). Moreover, it should not introduce any leak as it is set only if the values are empty.
While it's fresh in my mind (still) using /tmp/* in *DriverInit when I was generating patches the domain master secret key file caused problems if I actually tried to check for the existence, especially since qemuProcessPrepareHost is where the qemuProcessMakeDir calls were made to create the directory structure. Perhaps if the tests driver created "tmp/*" paths rather than "/tmp/*" paths that'd work, but is more or less unrelated.
Oh, that reminded me that I should figure out why I can't start any machine since those patches went in... Anyway, I'm not aware about the fact that qemuProcessPrepareHost() is called in tests. And if it is, that's the problem by itself.
I'll wait for your thoughts on this before an official ACK -
John
On 04/12/2016 08:43 AM, Martin Kletzander wrote:
On Thu, Mar 31, 2016 at 08:35:43AM -0400, John Ferlan wrote:
On 03/30/2016 11:14 AM, Martin Kletzander wrote:
Since commit 714080791778e3dfbd484ccb3953bffd820b8ba9, qemu agent channel cannot be plugged in because we won't generate its path automatically. Let's not only fix that, but also add tests for it so next time it's checked for.
Save some electrons, shorten the commit id
I did that, but I wonder whether you haven't used more electrons by mentioning that than I just shaved off =D
touche
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1322210
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_hotplug.c | 3 ++ tests/qemuhotplugtest.c | 15 ++++++ .../qemuhotplug-hotplug-base+qemu-agent-detach.xml | 58 ++++++++++++++++++++++ .../qemuhotplug-hotplug-base+qemu-agent.xml | 58 ++++++++++++++++++++++ .../qemuhotplug-qemu-agent-detach.xml | 5 ++ .../qemuhotplugtestdata/qemuhotplug-qemu-agent.xml | 5 ++ 6 files changed, 144 insertions(+) create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+qemu-agent-detach.xml create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+qemu-agent.xml create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-qemu-agent-detach.xml create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-qemu-agent.xml
[...]
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 2b0de94fb4a6..384b7b9592b9 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -98,6 +98,14 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
(*vm)->def->id = QEMU_HOTPLUG_TEST_DOMAIN_ID;
+ if (qemuDomainSetPrivatePaths(&priv->libDir, + &priv->channelTargetDir, + "/var/lib/libvirt", + "/var/lib/libvirt/qemu/channel/target", + (*vm)->def->name, + (*vm)->def->id) < 0)
I believe this overwrites qemuTestDriverInit - since it's a test I'm not concerned about the memory leak, just the processing consistency since you're not really starting a guest, why change the paths?
Well, I just needed to initialize it to some string. I can change it to /tmp without any problems. However it is not used anywhere to access anything and we would need to change a lot of tests that make sense currently (as they generate those values -- one of them is even checking that it generates that particular value).
Moreover, it should not introduce any leak as it is set only if the values are empty.
oh right - although I imagine even this changes since commit id '1893b6df1' altered the API. I'm sure you know that already though ;-) ACK for this though John
While it's fresh in my mind (still) using /tmp/* in *DriverInit when I was generating patches the domain master secret key file caused problems if I actually tried to check for the existence, especially since qemuProcessPrepareHost is where the qemuProcessMakeDir calls were made to create the directory structure. Perhaps if the tests driver created "tmp/*" paths rather than "/tmp/*" paths that'd work, but is more or less unrelated.
Oh, that reminded me that I should figure out why I can't start any machine since those patches went in...
Anyway, I'm not aware about the fact that qemuProcessPrepareHost() is called in tests. And if it is, that's the problem by itself.
On Wed, Apr 13, 2016 at 07:25:17AM -0400, John Ferlan wrote:
On 04/12/2016 08:43 AM, Martin Kletzander wrote:
On Thu, Mar 31, 2016 at 08:35:43AM -0400, John Ferlan wrote:
On 03/30/2016 11:14 AM, Martin Kletzander wrote:
Since commit 714080791778e3dfbd484ccb3953bffd820b8ba9, qemu agent channel cannot be plugged in because we won't generate its path automatically. Let's not only fix that, but also add tests for it so next time it's checked for.
Save some electrons, shorten the commit id
I did that, but I wonder whether you haven't used more electrons by mentioning that than I just shaved off =D
touche
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1322210
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_hotplug.c | 3 ++ tests/qemuhotplugtest.c | 15 ++++++ .../qemuhotplug-hotplug-base+qemu-agent-detach.xml | 58 ++++++++++++++++++++++ .../qemuhotplug-hotplug-base+qemu-agent.xml | 58 ++++++++++++++++++++++ .../qemuhotplug-qemu-agent-detach.xml | 5 ++ .../qemuhotplugtestdata/qemuhotplug-qemu-agent.xml | 5 ++ 6 files changed, 144 insertions(+) create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+qemu-agent-detach.xml create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+qemu-agent.xml create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-qemu-agent-detach.xml create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-qemu-agent.xml
[...]
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 2b0de94fb4a6..384b7b9592b9 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -98,6 +98,14 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
(*vm)->def->id = QEMU_HOTPLUG_TEST_DOMAIN_ID;
+ if (qemuDomainSetPrivatePaths(&priv->libDir, + &priv->channelTargetDir, + "/var/lib/libvirt", + "/var/lib/libvirt/qemu/channel/target", + (*vm)->def->name, + (*vm)->def->id) < 0)
I believe this overwrites qemuTestDriverInit - since it's a test I'm not concerned about the memory leak, just the processing consistency since you're not really starting a guest, why change the paths?
Well, I just needed to initialize it to some string. I can change it to /tmp without any problems. However it is not used anywhere to access anything and we would need to change a lot of tests that make sense currently (as they generate those values -- one of them is even checking that it generates that particular value).
Moreover, it should not introduce any leak as it is set only if the values are empty.
oh right - although I imagine even this changes since commit id '1893b6df1' altered the API. I'm sure you know that already though ;-)
That commit didn't change it, fortunately =)
ACK for this though
Thanks for the review. Just to make sure, I can push the whole series?
John
While it's fresh in my mind (still) using /tmp/* in *DriverInit when I was generating patches the domain master secret key file caused problems if I actually tried to check for the existence, especially since qemuProcessPrepareHost is where the qemuProcessMakeDir calls were made to create the directory structure. Perhaps if the tests driver created "tmp/*" paths rather than "/tmp/*" paths that'd work, but is more or less unrelated.
Oh, that reminded me that I should figure out why I can't start any machine since those patches went in...
Anyway, I'm not aware about the fact that qemuProcessPrepareHost() is called in tests. And if it is, that's the problem by itself.
participants (2)
-
John Ferlan -
Martin Kletzander