February 2016 - Devel - libvirt List Archives

[libvirt] [PATCH 0/2] tests: Fix mock libraries linking
by Andrea Bolognani 12 Feb '16

12 Feb '16

This patches make sure mock libraries do not link against either too many or too few libraries. Cheers. Andrea Bolognani (2): tests: Use plain close() in mock code tests: Link mock libraries against gnulib and gnulib only tests/Makefile.am | 21 +++++++++++++++------ tests/virportallocatortest.c | 2 +- 2 files changed, 16 insertions(+), 7 deletions(-) -- 2.5.0

2 5

[libvirt] [PATCH 0/4] tests: Fix syntax-check failure, plus extras
by Andrea Bolognani 12 Feb '16

12 Feb '16

Commit a03cbfe0fb introduced a syntax-check failure: patch 1/4 prepares for 2/4, where the failure is actually fixed. Patches 3/4 and 4/4 improve the naming consistency for mock libraries by shuffling files around a bit. Cheers. Andrea Bolognani (4): tests: Split off the mock part of the port allocator test tests: Allow use of close() in mock libraries tests: Don't use "lib" prefix for mock libraries tests: Rename virmockdbus -> virdbusmock for consistency cfg.mk | 2 +- tests/Makefile.am | 26 ++++---- tests/{virmockdbus.c => virdbusmock.c} | 2 +- tests/virfirewalltest.c | 2 +- tests/virpolkittest.c | 2 +- tests/virportallocatormock.c | 108 +++++++++++++++++++++++++++++++ tests/virportallocatortest.c | 112 +++------------------------------ tests/virsystemdtest.c | 2 +- 8 files changed, 134 insertions(+), 122 deletions(-) rename tests/{virmockdbus.c => virdbusmock.c} (97%) create mode 100644 tests/virportallocatormock.c -- 2.5.0

2 7

[libvirt] Found mem leak in livirt, need help to debug
by Piotr Rybicki 12 Feb '16

12 Feb '16

Hi. There is a mem leak in libvirt, when doing external snapshot (for backup purposes). My KVM domain uses raw storage images via libgfapi. I'm using latest 1.2.21 libvirt (although previous versions act the same). My bash script for snapshot backup uses series of shell commands (virsh connect to a remote libvirt host): * virsh domblklist KVM * qemu-img create -f qcow2 -o backing_file=gluster(...) - precreate backing file * virsh snapshot-create KVM SNAP.xml (...) - create snapshot from precreated XML snapshot file * cp main img file * virsh blockcommit KVM disk (...) Backup script works fine, however libvirtd process gets bigger and bigger each time I run this script. Some proof of memleak: 32017 - libvirtd pid When libvirt started: # ps p 32017 o vsz,rss VSZ RSS 585736 15220 When I start KVM via virsh start KVM # ps p 32017 o vsz,rss VSZ RSS 1327968 125956 When i start backup script, after snapshot is created (lots of mem allocated) # ps p 32017 o vsz,rss VSZ RSS 3264544 537632 After backup script finished # ps p 32017 o vsz,rss VSZ RSS 3715920 644940 When i start backup script for a second time, after snapshot is created # ps p 32017 o vsz,rss VSZ RSS 5521424 1056352 And so on, until libvirt spills 'Out of memory' when connecting, ane being really huge process. Now, I would like to diagnose it further, to provide detailed information about memleak. I tried to use valgrind, but unfortunatelly I'm on Opteron 6380 platform, and valgrind doesn't support XOP quitting witch SIGILL. If someone could provide me with detailed information on how to get some usefull debug info about this memleak, i'll be more than happy to do it, and share results here. Thanks in advance and best regards Piotr Rybicki

3 13

[libvirt] [PATCH 0/4] conf: snapshot: refactor and fix autogenerated duplicate snapshot targets
by Peter Krempa 12 Feb '16

12 Feb '16

Peter Krempa (4): conf: snapshot: Rename disksorter to virDomainSnapshotCompareDiskIndex snapshot: conf: Extract code to generate default external file names conf: snapshot: Refactor virDomainSnapshotDefAssignExternalNames conf: snapshot: Avoid autogenerating duplicate snapshot names src/conf/snapshot_conf.c | 142 ++++++++++++++++++++++++++++------------------- 1 file changed, 86 insertions(+), 56 deletions(-) -- 2.6.2

2 9

[libvirt] [RFC] vhost-user + shared memory + NUMA
by Pavel Fedin 12 Feb '16

12 Feb '16

Hello! vhost-user has a small limitation: guest memory must be shared. However, this simple requirement is satisfied by Libvirt only in very complicated case: 1. We have to specify NUMA configuration, because we can have "shared" attribute only for node descriptors inside "NUMA" section. 2. We have to specify huge page size, because memory-backend-file is used only in this case. Isn't it a problem? In order to do a simple thing (use vhost-user) we have to add two more quote unobvious things, making the whole stuff significantly more complicated. This creates even more problems on the level above, for example in order to get OpenStack working with userspace networking, we have to edit all flavors and rebuild all instances. And no single documentation mentions it. Shouldn't Libvirt simply detect usage of vhost-user, and build some minimal configuration with shared memory? For example, it could be memory-backend-file on /dev/mem. If the community agrees that it's a good idea, improving the usability, i can propose patches. Kind regards, Pavel Fedin Senior Engineer Samsung Electronics Research center Russia

2 4

[libvirt] [PATCH] syntax-check: Allow plain close in virportallocatortest
by Michal Privoznik 11 Feb '16

11 Feb '16

After a03cbfe0fb9 we switched to a plain close() instead of VIR_FORCE_CLOSE(). This makes sense because it's a mocking library and thus should not require anything from libvirt. However, we forgot add an exemption to our 'forbid close' rule. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- cfg.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cfg.mk b/cfg.mk index 3f78842..dbd22a7 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1158,7 +1158,7 @@ exclude_file_name_regexp--sc_prohibit_strdup = \ ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$) exclude_file_name_regexp--sc_prohibit_close = \ - (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir(cgroup|pci)mock\.c)$$) + (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir((cgroup|pci)mock|portallocatortest)\.c)$$) exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF = \ (^tests/(qemuhelp|nodeinfo|virpcitest)data/|\.diff$$) -- 2.4.10

2 1

[libvirt] [PATCH] dbus: Don't unref NULL messages
by Michal Privoznik 11 Feb '16

11 Feb '16

Apparently we are not the only ones with dumb free functions because dbus_message_unref() does not accept NULL either. But if I were to vote, this one is even more evil. Instead of returning an error just like we do it immediately dereference any pointer passed and thus crash you app. Well done DBus! Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7f878ebda700 (LWP 31264)] 0x00007f87be4016e5 in ?? () from /usr/lib64/libdbus-1.so.3 (gdb) bt #0 0x00007f87be4016e5 in ?? () from /usr/lib64/libdbus-1.so.3 #1 0x00007f87be3f004e in dbus_message_unref () from /usr/lib64/libdbus-1.so.3 #2 0x00007f87bf6ecf95 in virSystemdGetMachineNameByPID (pid=9849) at util/virsystemd.c:228 #3 0x00007f879761bd4d in qemuConnectCgroup (driver=0x7f87600a32a0, vm=0x7f87600c7550) at qemu/qemu_cgroup.c:909 #4 0x00007f87976386b7 in qemuProcessReconnect (opaque=0x7f87600db840) at qemu/qemu_process.c:3386 #5 0x00007f87bf6edfff in virThreadHelper (data=0x7f87600d5580) at util/virthread.c:206 #6 0x00007f87bb602334 in start_thread (arg=0x7f878ebda700) at pthread_create.c:333 #7 0x00007f87bb3481bd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109 (gdb) frame 2 #2 0x00007f87bf6ecf95 in virSystemdGetMachineNameByPID (pid=9849) at util/virsystemd.c:228 228 dbus_message_unref(reply); (gdb) p reply $1 = (DBusMessage *) 0x0 Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- src/libvirt_private.syms | 1 + src/rpc/virnetdaemon.c | 4 ++-- src/util/virdbus.c | 14 +++++++------- src/util/virdbus.h | 1 - src/util/virfirewall.c | 3 +-- src/util/virsystemd.c | 2 +- tests/virdbustest.c | 20 ++++++++++---------- tests/virfirewalltest.c | 3 +-- tests/virpolkittest.c | 2 +- tests/virsystemdtest.c | 3 ++- 10 files changed, 26 insertions(+), 27 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5ae3618..4cfaed5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1370,6 +1370,7 @@ virDBusHasSystemBus; virDBusMessageDecode; virDBusMessageEncode; virDBusMessageRead; +virDBusMessageUnref; virDBusSetSharedBus; diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c index 910f266..18c962c 100644 --- a/src/rpc/virnetdaemon.c +++ b/src/rpc/virnetdaemon.c @@ -374,7 +374,7 @@ virNetDaemonGotInhibitReply(DBusPendingCall *pending, VIR_FORCE_CLOSE(fd); } } - dbus_message_unref(reply); + virDBusMessageUnref(reply); cleanup: virObjectUnlock(dmn); @@ -426,7 +426,7 @@ virNetDaemonCallInhibit(virNetDaemonPtr dmn, dmn, NULL); dmn->autoShutdownCallingInhibit = true; } - dbus_message_unref(message); + virDBusMessageUnref(message); } #endif diff --git a/src/util/virdbus.c b/src/util/virdbus.c index 78fb795..3f4dbe3 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -1398,7 +1398,7 @@ int virDBusCreateMethodV(DBusMessage **call, } if (virDBusMessageEncodeArgs(*call, types, args) < 0) { - dbus_message_unref(*call); + virDBusMessageUnref(*call); *call = NULL; goto cleanup; } @@ -1467,7 +1467,7 @@ int virDBusCreateReplyV(DBusMessage **reply, } if (virDBusMessageEncodeArgs(*reply, types, args) < 0) { - dbus_message_unref(*reply); + virDBusMessageUnref(*reply); *reply = NULL; goto cleanup; } @@ -1586,7 +1586,7 @@ virDBusCall(DBusConnection *conn, if (ret == 0 && replyout) *replyout = reply; else - dbus_message_unref(reply); + virDBusMessageUnref(reply); } return ret; } @@ -1650,8 +1650,7 @@ int virDBusCallMethod(DBusConnection *conn, ret = virDBusCall(conn, call, replyout, error); cleanup: - if (call) - dbus_message_unref(call); + virDBusMessageUnref(call); return ret; } @@ -1727,7 +1726,7 @@ static int virDBusIsServiceInList(const char *listMethod, const char *name) } cleanup: - dbus_message_unref(reply); + virDBusMessageUnref(reply); return ret; } @@ -1763,7 +1762,8 @@ int virDBusIsServiceRegistered(const char *name) void virDBusMessageUnref(DBusMessage *msg) { - dbus_message_unref(msg); + if (msg) + dbus_message_unref(msg); } #else /* ! WITH_DBUS */ diff --git a/src/util/virdbus.h b/src/util/virdbus.h index 9e86538..86b4223 100644 --- a/src/util/virdbus.h +++ b/src/util/virdbus.h @@ -28,7 +28,6 @@ # else # define DBusConnection void # define DBusMessage void -# define dbus_message_unref(m) do {} while (0) # endif # include "internal.h" diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index a972c05..f26fd86 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -822,8 +822,7 @@ virFirewallApplyRuleFirewallD(virFirewallRulePtr rule, cleanup: virResetError(&error); - if (reply) - dbus_message_unref(reply); + virDBusMessageUnref(reply); return ret; } diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index 6677798..4883f94 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -225,7 +225,7 @@ virSystemdGetMachineNameByPID(pid_t pid) cleanup: VIR_FREE(object); - dbus_message_unref(reply); + virDBusMessageUnref(reply); return name; } diff --git a/tests/virdbustest.c b/tests/virdbustest.c index 4ec3c0d..1622b03 100644 --- a/tests/virdbustest.c +++ b/tests/virdbustest.c @@ -121,7 +121,7 @@ static int testMessageSimple(const void *args ATTRIBUTE_UNUSED) VIR_FREE(out_string); VIR_FREE(out_signature); VIR_FREE(out_objectpath); - dbus_message_unref(msg); + virDBusMessageUnref(msg); return ret; } @@ -171,7 +171,7 @@ static int testMessageVariant(const void *args ATTRIBUTE_UNUSED) cleanup: VIR_FREE(out_str1); VIR_FREE(out_str2); - dbus_message_unref(msg); + virDBusMessageUnref(msg); return ret; } @@ -230,7 +230,7 @@ static int testMessageArray(const void *args ATTRIBUTE_UNUSED) cleanup: VIR_FREE(out_str1); VIR_FREE(out_str2); - dbus_message_unref(msg); + virDBusMessageUnref(msg); return ret; } @@ -274,7 +274,7 @@ static int testMessageEmptyArrayRef(const void *args ATTRIBUTE_UNUSED) ret = 0; cleanup: - dbus_message_unref(msg); + virDBusMessageUnref(msg); return ret; } @@ -323,7 +323,7 @@ static int testMessageSingleArrayRef(const void *args ATTRIBUTE_UNUSED) cleanup: if (out_strv1) VIR_FREE(out_strv1[0]); - dbus_message_unref(msg); + virDBusMessageUnref(msg); return ret; } @@ -436,7 +436,7 @@ static int testMessageArrayRef(const void *args ATTRIBUTE_UNUSED) for (i = 0; i < out_nstrv2; i++) VIR_FREE(out_strv2[i]); VIR_FREE(out_strv2); - dbus_message_unref(msg); + virDBusMessageUnref(msg); return ret; } @@ -511,7 +511,7 @@ static int testMessageStruct(const void *args ATTRIBUTE_UNUSED) VIR_FREE(out_string); VIR_FREE(out_signature); VIR_FREE(out_objectpath); - dbus_message_unref(msg); + virDBusMessageUnref(msg); return ret; } @@ -581,7 +581,7 @@ static int testMessageDict(const void *args ATTRIBUTE_UNUSED) VIR_FREE(out_key1); VIR_FREE(out_key2); VIR_FREE(out_key3); - dbus_message_unref(msg); + virDBusMessageUnref(msg); return ret; } @@ -652,7 +652,7 @@ static int testMessageDictRef(const void *args ATTRIBUTE_UNUSED) VIR_FREE(out_strv1[5]); } VIR_FREE(out_strv1); - dbus_message_unref(msg); + virDBusMessageUnref(msg); return ret; } @@ -695,7 +695,7 @@ static int testMessageEmptyDictRef(const void *args ATTRIBUTE_UNUSED) ret = 0; cleanup: - dbus_message_unref(msg); + virDBusMessageUnref(msg); return ret; } diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c index 1f8d8f1..8f6fc9e 100644 --- a/tests/virfirewalltest.c +++ b/tests/virfirewalltest.c @@ -179,8 +179,7 @@ VIR_MOCK_WRAP_RET_ARGS(dbus_connection_send_with_reply_and_block, return reply; error: - if (reply) - dbus_message_unref(reply); + virDBusMessageUnref(reply); reply = NULL; if (error && !dbus_error_is_set(error)) dbus_set_error_const(error, diff --git a/tests/virpolkittest.c b/tests/virpolkittest.c index cdf78f5..b39beed 100644 --- a/tests/virpolkittest.c +++ b/tests/virpolkittest.c @@ -140,7 +140,7 @@ VIR_MOCK_WRAP_RET_ARGS(dbus_connection_send_with_reply_and_block, return reply; error: - dbus_message_unref(reply); + virDBusMessageUnref(reply); return NULL; } diff --git a/tests/virsystemdtest.c b/tests/virsystemdtest.c index 46452dd..101f5e0 100644 --- a/tests/virsystemdtest.c +++ b/tests/virsystemdtest.c @@ -28,6 +28,7 @@ # include <dbus/dbus.h> # include "virsystemd.h" +# include "virdbus.h" # include "virlog.h" # include "virmock.h" # define VIR_FROM_THIS VIR_FROM_NONE @@ -151,7 +152,7 @@ VIR_MOCK_WRAP_RET_ARGS(dbus_connection_send_with_reply_and_block, return reply; error: - dbus_message_unref(reply); + virDBusMessageUnref(reply); return NULL; } -- 2.4.10

2 1

[libvirt] [PATCH] Revert "tests: Don't link mock libraries against libvirt and gnulib"
by Andrea Bolognani 11 Feb '16

11 Feb '16

This reverts commit 6aa90452aa63cb1e1ffa84ff5f93f5873bf810a0. Turns out that not linking against libvirt and gnulib is okay for regular Linux (and FreeBSD) builds, but makes mingw very unhappy. .../virnetserverclientmock_la-virnetserverclientmock.o: In function `virNetSocketGetSELinuxContext': .../virnetserverclientmock.c:61: undefined reference to `rpl_strdup' .../libvirportallocatormock_la-virportallocatortest.o: In function `init_syms': .../virportallocatortest.c:61: undefined reference to `virFileClose' --- Pushed as build breaker. Bunny ears of shame will be deployed in the morning. tests/Makefile.am | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/Makefile.am b/tests/Makefile.am index fb862cd..c5986f0 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -946,6 +946,7 @@ virnetserverclientmock_la_SOURCES = \ virnetserverclientmock.c virnetserverclientmock_la_CFLAGS = $(AM_CFLAGS) virnetserverclientmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS) +virnetserverclientmock_la_LIBADD = $(GNULIB_LIBS) if WITH_GNUTLS virnettlscontexttest_SOURCES = \ @@ -1024,6 +1025,7 @@ libvirportallocatormock_la_SOURCES = \ virportallocatortest.c libvirportallocatormock_la_CFLAGS = $(AM_CFLAGS) -DMOCK_HELPER=1 libvirportallocatormock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS) +libvirportallocatormock_la_LIBADD = ../src/libvirt.la vircgrouptest_SOURCES = \ vircgrouptest.c testutils.h testutils.c @@ -1049,6 +1051,8 @@ virpcitest_LDADD = $(LDADDS) virpcimock_la_SOURCES = \ virpcimock.c virpcimock_la_CFLAGS = $(AM_CFLAGS) +virpcimock_la_LIBADD = $(GNULIB_LIBS) \ + ../src/libvirt.la virpcimock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS) nodeinfomock_la_SOURCES = \ @@ -1064,6 +1068,8 @@ virnetdevtest_LDADD = $(LDADDS) virnetdevmock_la_SOURCES = \ virnetdevmock.c virnetdevmock_la_CFLAGS = $(AM_CFLAGS) $(LIBNL_CFLAGS) +virnetdevmock_la_LIBADD = $(GNULIB_LIBS) \ + ../src/libvirt.la virnetdevmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS) virrotatingfiletest_SOURCES = \ -- 2.5.0

2 4

[libvirt] [PATCH] Prohibit verbose strcat
by Ján Tomko 11 Feb '16

11 Feb '16

Using strcat directly is more readable than passing strlen of the copied string to strncat. --- cfg.mk | 5 +++++ src/storage/storage_backend_logical.c | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/cfg.mk b/cfg.mk index 71b0866..be9e475 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1031,6 +1031,11 @@ sc_prohibit_not_strneq: halt='Use STREQ instead of !STRNEQ' \ $(_sc_search_regexp) +sc_prohibit_verbose_strcat: + @prohibit='strncat$[^,]*,\s+([^,]*),\s+strlen\(\1$\)' \ + halt='Use strcat(a, b) instead of strncat(a, b, strlen())' \ + $(_sc_search_regexp) + # We don't use this feature of maint.mk. prev_version_file = /dev/null diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index ba26223..f0d6f80 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -123,11 +123,11 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, /* Allocate space for 'nextents' regex_unit strings plus a comma for each */ if (VIR_ALLOC_N(regex, nextents * (strlen(regex_unit) + 1) + 1) < 0) goto cleanup; - strncat(regex, regex_unit, strlen(regex_unit)); + strcat(regex, regex_unit); for (i = 1; i < nextents; i++) { /* "," is the separator of "devices" field */ strcat(regex, ","); - strncat(regex, regex_unit, strlen(regex_unit)); + strcat(regex, regex_unit); } if (VIR_ALLOC(reg) < 0) -- 2.4.10

2 3

[libvirt] [PATCH] virhook: do not save the return value of virBuildPath
by Ján Tomko 11 Feb '16

11 Feb '16

This function returns -1 on allocation error, there's no need to check the path for NULL again. --- src/util/virhook.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/util/virhook.c b/src/util/virhook.c index ee19382..ba50598 100644 --- a/src/util/virhook.c +++ b/src/util/virhook.c @@ -119,8 +119,7 @@ virHookCheck(int no, const char *driver) return -1; } - ret = virBuildPath(&path, LIBVIRT_HOOK_DIR, driver); - if ((ret < 0) || (path == NULL)) { + if (virBuildPath(&path, LIBVIRT_HOOK_DIR, driver) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to build path for %s hook"), driver); @@ -276,8 +275,7 @@ virHookCall(int driver, if (extra == NULL) extra = "-"; - ret = virBuildPath(&path, LIBVIRT_HOOK_DIR, drvstr); - if ((ret < 0) || (path == NULL)) { + if (virBuildPath(&path, LIBVIRT_HOOK_DIR, drvstr) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to build path for %s hook"), drvstr); -- 2.4.10

2 2