Re: [libvirt] [PATCH 3/3] qemu: Generate channel target paths on hotplug as well

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(a)redhat.com&gt; >>> --- >>> 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. >