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