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@xxxxxxxxxx>
>> ---
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
