[libvirt] [RFC] [PATCH] Fix flawed testcases in qemuhotplugtest.c

[Michal Privoznik]

On 19.03.2014 00:08, Nehal J Wani wrote:
> While running qemuhotplugtest, it was found that valgrind pointed out
> the following memory leak:
> 
> ==7906== 5 bytes in 1 blocks are definitely lost in loss record 7 of 121
> ==7906==    at 0x4A069EE: malloc (vg_replace_malloc.c:270)
> ==7906==    by 0x3E782A754D: xmlStrndup (in /usr/lib64/libxml2.so.2.7.6)
> ==7906==    by 0x4CDAE03: virDomainDeviceInfoParseXML.isra.32 (domain_conf.c:3685)
> ==7906==    by 0x4CE3BB9: virDomainNetDefParseXML (domain_conf.c:6707)
> ==7906==    by 0x4CFBA08: virDomainDefParseXML (domain_conf.c:12235)
> ==7906==    by 0x4CFBC1E: virDomainDefParseNode (domain_conf.c:13039)
> ==7906==    by 0x4CFBD95: virDomainDefParse (domain_conf.c:12981)
> ==7906==    by 0x41FEB4: testQemuHotplug (qemuhotplugtest.c:66)
> ==7906==    by 0x420F41: virtTestRun (testutils.c:201)
> ==7906==    by 0x41F287: mymain (qemuhotplugtest.c:422)
> ==7906==    by 0x4216BD: virtTestMain (testutils.c:784)
> ==7906==    by 0x3E6CE1ED1C: (below main) (libc-start.c:226)
> ...and 10 more.
> 
> The 'alias' attribute should *not* be parsed from the XML provided by
> the user. It should only be parsed in the live state XML. In the latter
> case no codepath should take us to qemuAssignDeviceAliases (which, in
> this case, is causing the above leaks).

You're right, in case of domains, alias should not be parsed from user supplied XMLs. But the XMLs used in the test are not user supplied ones. In fact, we want domain to contain all information that a running domain does (e.g. all aliases) - so for the hotplug code it looks like domain is running (coldplug code just don't care).

Having said that, I agree that there's no need to have aliases in the XMLs but generate them on the fly. That's what is happening now, well since 20745748 which introduced the generation at runtime.

So I guess your comment is not quite correct.

> 
> ---
>   Caused by the test cases:
>      DO_TEST_ATTACH("console-compat-2", "console-virtio", false, true,
>                     "chardev-add", "{\"return\": {\"pty\": \"/dev/pts/26\"}}",
>                     "device_add", QMP_OK);
> 
>      DO_TEST_DETACH("console-compat-2", "console-virtio", false, false,
>                     "device_del", QMP_OK,
>                     "chardev-remove", QMP_OK);
> 
>   Refer: http://www.redhat.com/archives/libvir-list/2013-November/msg01187.html
> 
>   ...qemuhotplug-console-compat-2+console-virtio.xml |   34 +++-----------------
>   .../qemuhotplug-console-virtio.xml                 |    1 -
>   .../qemuxml2argv-console-compat-2.xml              |   32 +++---------------
>   3 files changed, 10 insertions(+), 57 deletions(-)
> 

The patch is incomplete. We need this as well:

diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 8036adc..4ef81e0 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -67,7 +67,7 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
                                                driver.caps,
                                                driver.xmlopt,
                                                QEMU_EXPECTED_VIRT_TYPES,
-                                               0)))
+                                               VIR_DOMAIN_XML_INACTIVE)))
         goto cleanup;
 
     priv = (*vm)->privateData;

Anyway, I would not call the testcases flawed, just mem-leaking. There's still one mem-leak that I'll investigate further.

ACK with the qemuhotplugtest change squashed in. I'm rewording both the subject and commit message and pushing this in. Thanks for sorting this out.

Michal