[libvirt] [PATCH 1/9] tests: Run virmacmaptest iff WITH_YAJL

[Roman Bogorodskiy]

  Michal Privoznik wrote:

> Since the internal implementation relies on a json parser being
> available, it make no sense to run this test if there's none
> available.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  tests/Makefile.am | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

Not directly related to this patch (which looks OK to me BTW), I still
cannot run virmacmaptest on FreeBSD: it segfaults on testMACRemove with
a backtrace like this:

* thread #1: tid = 100493, 0x0000000803949401 libc.so.7`__je_arena_salloc(tsdn=0x000000080062ea88, ptr=0x5a5a5a5a5a5a5a5a, demote=false) + 97 at arena.h:1384, stop reason = signal SIGBUS: hardware error
    frame #0: 0x0000000803949401 libc.so.7`__je_arena_salloc(tsdn=0x000000080062ea88, ptr=0x5a5a5a5a5a5a5a5a, demote=false) + 97 at arena.h:1384
   1381 /* Return the size of the allocation pointed to by ptr. */
   1382 JEMALLOC_ALWAYS_INLINE size_t
   1383 arena_salloc(tsdn_t *tsdn, const void *ptr, bool demote)
-> 1384 {
   1385         size_t ret;
   1386         arena_chunk_t *chunk;
   1387         size_t pageind;
(lldb) bt
* thread #1: tid = 100493, 0x0000000803949401 libc.so.7`__je_arena_salloc(tsdn=0x000000080062ea88, ptr=0x5a5a5a5a5a5a5a5a, demote=false) + 97 at arena.h:1384, stop reason = signal SIGBUS: hardware error
  * frame #0: 0x0000000803949401 libc.so.7`__je_arena_salloc(tsdn=0x000000080062ea88, ptr=0x5a5a5a5a5a5a5a5a, demote=false) + 97 at arena.h:1384
    frame #1: 0x00000008039557cb libc.so.7`ifree [inlined] __je_isalloc(tsdn=<unavailable>, ptr=0x5a5a5a5a5a5a5a5a, demote=false) + 13 at jemalloc_internal.h:1045
    frame #2: 0x00000008039557be libc.so.7`ifree(tsd=0x000000080062ea88, ptr=0x5a5a5a5a5a5a5a5a, tcache=0x000000080720d000, slow_path=true) + 78 at jemalloc_jemalloc.c:1870
    frame #3: 0x00000008039558f0 libc.so.7`__free(ptr=0x5a5a5a5a5a5a5a5a) + 112 at jemalloc_jemalloc.c:1997
    frame #4: 0x0000000800c71672 libvirt.so.0`virFree(ptrptr=0x000000080721cf80) + 34 at viralloc.c:582
    frame #5: 0x0000000800ce5a60 libvirt.so.0`virStringListFree(strings=0x000000080721cf80) + 80 at virstring.c:251
    frame #6: 0x0000000800cb973c libvirt.so.0`virMacMapHashFree(payload=0x000000080721cf80, name=0x000000080723f268) + 28 at virmacmap.c:67
    frame #7: 0x0000000800ca0179 libvirt.so.0`virHashAddOrUpdateEntry(table=0x000000080721d420, name=0x00000000004071c0, userdata=0x000000080721cf80, is_update=true) + 297 at virhash.c:352
    frame #8: 0x0000000800ca032a libvirt.so.0`virHashUpdateEntry(table=0x000000080721d420, name=0x00000000004071c0, userdata=0x000000080721cf80) + 42 at virhash.c:415
    frame #9: 0x0000000800cb9c8f libvirt.so.0`virMacMapRemoveLocked(mgr=0x000000080721cc80, domain="f24", mac="aa:bb:cc:dd:ee:ff") + 175 at virmacmap.c:129
    frame #10: 0x0000000800cb9bc1 libvirt.so.0`virMacMapRemove(mgr=0x000000080721cc80, domain="f24", mac="aa:bb:cc:dd:ee:ff") + 49 at virmacmap.c:346
    frame #11: 0x00000000004040a0 virmacmaptest`testMACRemove(opaque=0x00007fffffffe260) + 288 at virmacmaptest.c:109
    frame #12: 0x00000000004043ac virmacmaptest`virTestRun(title="Remove \"f24\" in \"simple2\"", body=(virmacmaptest`testMACRemove at virmacmaptest.c:91), data=0x00007fffffffe260) + 284 at testutils.c:180
    frame #13: 0x0000000000403074 virmacmaptest`mymain + 468 at virmacmaptest.c:207
    frame #14: 0x000000000040629d virmacmaptest`virTestMain(argc=1, argv=0x00007fffffffe610, func=(virmacmaptest`mymain at virmacmaptest.c:158)) + 2189 at testutils.c:992
    frame #15: 0x0000000000402e8d virmacmaptest`main(argc=1, argv=0x00007fffffffe610) + 61 at virmacmaptest.c:239
    frame #16: 0x0000000000402d4f virmacmaptest`_start + 383
(lldb) 

It looks like a problem in virMacMapRemoveLocked() to me, because it
calls virStringListRemove() on the existing macs list which modifies a
pointer to the macs string list. Later, when updating a hash table entry
for the existing domain, it tries to clean up the old data by calling
virStringListFree (via virMacMapHashFree) on the wrong pointer. I'm not
sure why it doesn't show up on Linux though, maybe my analysis is wrong.

I was able to fix this with a change like this:

diff --git a/src/util/virmacmap.c b/src/util/virmacmap.c
index 36c364e10..b328ba714 100644
--- a/src/util/virmacmap.c
+++ b/src/util/virmacmap.c
@@ -116,21 +116,31 @@ virMacMapRemoveLocked(virMacMapPtr mgr,
 {
     char **macsList = NULL;
     char **newMacsList = NULL;
+    int i = 0, ret = -1;
 
     if (!(macsList = virHashLookup(mgr->macs, domain)))
         return 0;
 
-    newMacsList = macsList;
-    virStringListRemove(&macsList, mac);
-    if (!macsList) {
+    for (i = 0; macsList && macsList[i]; i++) {
+        if (!STREQ(macsList[i], mac))
+           newMacsList = virStringListAdd((const char **)newMacsList,
+                                          macsList[i]);
+    }
+
+    if (!newMacsList) {
         virHashSteal(mgr->macs, domain);
     } else {
         if (macsList != newMacsList &&
             virHashUpdateEntry(mgr->macs, domain, newMacsList) < 0)
-            return -1;
+            goto cleanup;
     }
 
-    return 0;
+    newMacsList = NULL;
+    ret = 0;
+
+ cleanup:
+    virStringListFree(newMacsList);
+    return ret;
 }
 
PS According to valgrind, it leaks 34 (16 direct, 18 indirect) bytes. I
haven't figured out where exactly so far.

Roman Bogorodskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170101/872c6fe1/attachment-0001.sig>