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

[John Ferlan]

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