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

Michal Privoznik wrote: 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