On 01.01.2017 17:11, Roman Bogorodskiy wrote:
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@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.
Ah. Running the test under valgrind shows the same problem except the process is not getting SIGSEGV. The problem is indeed in virMacMapRemoveLocked(). But it is slightly more complicated. If a MAC address that caller wants to remove is in the list, virStringListRemove() is called which modifies the string list. Then either virHashSteal() or virHashUpdateEntry() is called to update the pointer to the list in the hash table. However, virHashUpdateEntry() calls the dataFree callback - but over pointer to the old list. Looks to me the best would be to not set dataFree callback at all. I'm testing this approach now. Will post the patches shortly.
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;
Yeah, I had it this way originally, but some people on the list disliked the idea of allocating more memory on an element remove. Michal