[Qemu-devel] [PATCH v2 00/30] Various memory leak fixes

[Qemu-devel] [PATCH v2 00/30] Various memory leak fixes

[Marc-André Lureau]

After this series removing a few memory leaks, make check with ASAN
enabled gives a "clean" run with x86_64-softmmu target (there are
warnings related to coroutine makecontext/swapcontext).

This should help spot memory related regressions when introducing one.

Note: some of these patches were sent and reviewed previously but not
yet applied, I rebased them and included them here for completeness.

v2:
- add r-b tags
- fix gtest compatibility with old glib
- add "migration: fix id leak regression" patch

Igor Mammedov (1):
  pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice

Marc-André Lureau (29):
  qtest: fix a memory leak
  tests: fix qmp response leak
  tests: fix leaks in test-io-channel-command
  timer: use an inline function for free
  glib-compat: add g_test_add_data_func_full fallback
  tests: fix ptimer leaks
  tests: fix endianness-test leaks
  tests: fix q35-test leaks
  tests: fix vhost-user-test leaks
  tests: fix ide-test leaks
  tests: fix hd-geo-test leaks
  tests: fix bios-tables-test leak
  tests: fix ipmi-kcs-test leak
  tests: fix ipmi-bt-test leak
  tests: fix eepro100-test leak
  tests: fix tco-test leaks
  tests: fix e1000-test leak
  tests: fix i440fx-test leaks
  tests: fix e1000e leaks
  tests: fix virtio-scsi-test leak
  tests: fix virtio-9p-test leaks
  bus: do not unref hotplug handler
  usb: replace handle_destroy with unrealize
  usb: release the created buses
  tests: allows to run single test in usb-hcd-ehci-test
  tests: fix usb-test leaks
  tests: add specialized device_find function
  tests: fix virtio-blk-test leaks
  migration: fix id leak regression

 include/glib-compat.h           |  21 +++++++
 include/hw/ptimer.h             |   1 +
 include/hw/usb.h                |   5 --
 include/qemu/timer.h            |   5 +-
 tests/libqos/usb.h              |   1 +
 tests/libqos/virtio-pci.h       |   6 +-
 hw/acpi/pcihp.c                 |  11 ----
 hw/core/bus.c                   |   2 +-
 hw/core/ptimer.c                |   8 +++
 hw/i386/acpi-build.c            |   4 +-
 hw/usb/bus.c                    |   9 +--
 hw/usb/dev-audio.c              |   4 +-
 hw/usb/dev-bluetooth.c          |   4 +-
 hw/usb/dev-hid.c                |   4 +-
 hw/usb/dev-hub.c                |   4 +-
 hw/usb/dev-network.c            |   4 +-
 hw/usb/dev-smartcard-reader.c   |   4 +-
 hw/usb/dev-storage.c            |  16 ++++++
 hw/usb/dev-uas.c                |   6 +-
 hw/usb/dev-wacom.c              |   4 +-
 hw/usb/host-libusb.c            |   4 +-
 hw/usb/redirect.c               |   4 +-
 migration/savevm.c              |   1 +
 qemu-timer.c                    |   5 --
 qtest.c                         |   1 +
 tests/bios-tables-test.c        |   2 +-
 tests/e1000-test.c              |   1 +
 tests/e1000e-test.c             |   6 +-
 tests/eepro100-test.c           |   1 +
 tests/endianness-test.c         |   3 +
 tests/hd-geo-test.c             |  47 ++++++++++------
 tests/i440fx-test.c             |   5 ++
 tests/ide-test.c                |  12 ++++
 tests/ipmi-bt-test.c            |   1 +
 tests/ipmi-kcs-test.c           |   1 +
 tests/libqos/usb.c              |   6 ++
 tests/libqos/virtio-pci.c       |  38 +++++++++++--
 tests/libqtest.c                |  10 ----
 tests/postcopy-test.c           |   2 +-
 tests/ptimer-test-stubs.c       |   5 ++
 tests/ptimer-test.c             | 122 ++++++++++++++++++++++++----------------
 tests/pvpanic-test.c            |   1 +
 tests/q35-test.c                |   3 +
 tests/tco-test.c                |  35 +++++++-----
 tests/test-filter-mirror.c      |   2 +-
 tests/test-filter-redirector.c  |   4 +-
 tests/test-io-channel-command.c |   6 +-
 tests/usb-hcd-ehci-test.c       |  19 +++++--
 tests/usb-hcd-uhci-test.c       |   1 +
 tests/vhost-user-test.c         |  11 ++--
 tests/virtio-9p-test.c          |   2 +-
 tests/virtio-blk-test.c         |  29 +++++-----
 tests/virtio-scsi-test.c        |   2 +-
 53 files changed, 338 insertions(+), 177 deletions(-)

-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 01/30] qtest: fix a memory leak

[Marc-André Lureau]

Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qtest.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qtest.c b/qtest.c
index 1446719e8d..6f4d26832a 100644
--- a/qtest.c
+++ b/qtest.c
@@ -240,6 +240,7 @@ static void GCC_FMT_ATTR(2, 3) qtest_sendf(CharBackend *chr,
     va_start(ap, fmt);
     buffer = g_strdup_vprintf(fmt, ap);
     qtest_send(chr, buffer);
+    g_free(buffer);
     va_end(ap);
 }
 
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 02/30] tests: fix qmp response leak

[Marc-André Lureau]

Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqos/usb.c             |  1 +
 tests/postcopy-test.c          |  2 +-
 tests/pvpanic-test.c           |  1 +
 tests/test-filter-mirror.c     |  2 +-
 tests/test-filter-redirector.c |  4 ++--
 tests/virtio-blk-test.c        | 15 +++++++++------
 6 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/tests/libqos/usb.c b/tests/libqos/usb.c
index 72d7a961fe..98408d93f3 100644
--- a/tests/libqos/usb.c
+++ b/tests/libqos/usb.c
@@ -64,4 +64,5 @@ void usb_test_hotplug(const char *hcd_id, const int port,
     g_assert(response);
     g_assert(qdict_haskey(response, "event"));
     g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
+    QDECREF(response);
 }
diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
index dafe8beba4..de35a18903 100644
--- a/tests/postcopy-test.c
+++ b/tests/postcopy-test.c
@@ -482,7 +482,7 @@ static void test_migrate(void)
         usleep(10 * 1000);
     } while (dest_byte_a == dest_byte_b);
 
-    qmp("{ 'execute' : 'stop'}");
+    qmp_discard_response("{ 'execute' : 'stop'}");
     /* With it stopped, check nothing changes */
     qtest_memread(to, start_address, &dest_byte_c, 1);
     sleep(1);
diff --git a/tests/pvpanic-test.c b/tests/pvpanic-test.c
index 3bfa678667..71ebb5c02c 100644
--- a/tests/pvpanic-test.c
+++ b/tests/pvpanic-test.c
@@ -27,6 +27,7 @@ static void test_panic(void)
     data = qdict_get_qdict(response, "data");
     g_assert(qdict_haskey(data, "action"));
     g_assert_cmpstr(qdict_get_str(data, "action"), ==, "pause");
+    QDECREF(response);
 }
 
 int main(int argc, char **argv)
diff --git a/tests/test-filter-mirror.c b/tests/test-filter-mirror.c
index ffaaffabd0..9f84402493 100644
--- a/tests/test-filter-mirror.c
+++ b/tests/test-filter-mirror.c
@@ -57,7 +57,7 @@ static void test_mirror(void)
     };
 
     /* send a qmp command to guarantee that 'connected' is setting to true. */
-    qmp("{ 'execute' : 'query-status'}");
+    qmp_discard_response("{ 'execute' : 'query-status'}");
     ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) + sizeof(send_buf));
     g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size));
     close(send_sock[0]);
diff --git a/tests/test-filter-redirector.c b/tests/test-filter-redirector.c
index c63b68f03a..0c4b8d52ef 100644
--- a/tests/test-filter-redirector.c
+++ b/tests/test-filter-redirector.c
@@ -99,7 +99,7 @@ static void test_redirector_tx(void)
     g_assert_cmpint(recv_sock, !=, -1);
 
     /* send a qmp command to guarantee that 'connected' is setting to true. */
-    qmp("{ 'execute' : 'query-status'}");
+    qmp_discard_response("{ 'execute' : 'query-status'}");
 
     struct iovec iov[] = {
         {
@@ -184,7 +184,7 @@ static void test_redirector_rx(void)
     send_sock = unix_connect(sock_path1, NULL);
     g_assert_cmpint(send_sock, !=, -1);
     /* send a qmp command to guarantee that 'connected' is setting to true. */
-    qmp("{ 'execute' : 'query-status'}");
+    qmp_discard_response("{ 'execute' : 'query-status'}");
 
     ret = iov_send(send_sock, iov, 2, 0, sizeof(size) + sizeof(send_buf));
     g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size));
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 0e32e416dd..10a92b410a 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -409,8 +409,9 @@ static void pci_config(void)
 
     qvirtio_set_driver_ok(&dev->vdev);
 
-    qmp("{ 'execute': 'block_resize', 'arguments': { 'device': 'drive0', "
-                                                    " 'size': %d } }", n_size);
+    qmp_discard_response("{ 'execute': 'block_resize', "
+                         " 'arguments': { 'device': 'drive0', "
+                         " 'size': %d } }", n_size);
     qvirtio_wait_config_isr(&dev->vdev, QVIRTIO_BLK_TIMEOUT_US);
 
     capacity = qvirtio_config_readq(&dev->vdev, 0);
@@ -458,8 +459,9 @@ static void pci_msix(void)
 
     qvirtio_set_driver_ok(&dev->vdev);
 
-    qmp("{ 'execute': 'block_resize', 'arguments': { 'device': 'drive0', "
-                                                    " 'size': %d } }", n_size);
+    qmp_discard_response("{ 'execute': 'block_resize', "
+                         " 'arguments': { 'device': 'drive0', "
+                         " 'size': %d } }", n_size);
 
     qvirtio_wait_config_isr(&dev->vdev, QVIRTIO_BLK_TIMEOUT_US);
 
@@ -691,8 +693,9 @@ static void mmio_basic(void)
 
     test_basic(&dev->vdev, alloc, vq);
 
-    qmp("{ 'execute': 'block_resize', 'arguments': { 'device': 'drive0', "
-                                                    " 'size': %d } }", n_size);
+    qmp_discard_response("{ 'execute': 'block_resize', "
+                         " 'arguments': { 'device': 'drive0', "
+                         " 'size': %d } }", n_size);
 
     qvirtio_wait_queue_isr(&dev->vdev, vq, QVIRTIO_BLK_TIMEOUT_US);
 
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 03/30] tests: fix leaks in test-io-channel-command

[Marc-André Lureau]

No need for strdup, fix leaks when socat is missing.

Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: "Daniel P. Berrange" <berrange@redhat.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
 tests/test-io-channel-command.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tests/test-io-channel-command.c b/tests/test-io-channel-command.c
index 1d1f461bed..46ce1ff01c 100644
--- a/tests/test-io-channel-command.c
+++ b/tests/test-io-channel-command.c
@@ -29,8 +29,8 @@ static void test_io_channel_command_fifo(bool async)
 #define TEST_FIFO "tests/test-io-channel-command.fifo"
     QIOChannel *src, *dst;
     QIOChannelTest *test;
-    char *srcfifo = g_strdup_printf("PIPE:%s,wronly", TEST_FIFO);
-    char *dstfifo = g_strdup_printf("PIPE:%s,rdonly", TEST_FIFO);
+    const char *srcfifo = "PIPE:" TEST_FIFO ",wronly";
+    const char *dstfifo = "PIPE:" TEST_FIFO ",rdonly";
     const char *srcargv[] = {
         "/bin/socat", "-", srcfifo, NULL,
     };
@@ -59,8 +59,6 @@ static void test_io_channel_command_fifo(bool async)
     object_unref(OBJECT(src));
     object_unref(OBJECT(dst));
 
-    g_free(srcfifo);
-    g_free(dstfifo);
     unlink(TEST_FIFO);
 }
 
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 04/30] timer: use an inline function for free

[Marc-André Lureau]

Similarly to allocation, do it from an inline function. This allows
tests to only use the headers for allocation/free of timer.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
 include/qemu/timer.h | 5 ++++-
 qemu-timer.c         | 5 -----
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 9abed51ae8..26e628584c 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -610,7 +610,10 @@ void timer_deinit(QEMUTimer *ts);
  *
  * Free a timer (it must not be on the active list)
  */
-void timer_free(QEMUTimer *ts);
+static inline void timer_free(QEMUTimer *ts)
+{
+    g_free(ts);
+}
 
 /**
  * timer_del:
diff --git a/qemu-timer.c b/qemu-timer.c
index ff620ecff7..6cf70b96f6 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -355,11 +355,6 @@ void timer_deinit(QEMUTimer *ts)
     ts->timer_list = NULL;
 }
 
-void timer_free(QEMUTimer *ts)
-{
-    g_free(ts);
-}
-
 static void timer_del_locked(QEMUTimerList *timer_list, QEMUTimer *ts)
 {
     QEMUTimer **pt, *t;
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 05/30] glib-compat: add g_test_add_data_func_full fallback

[Marc-André Lureau]

Move the fallback from qtest_add_data_func_full() to glib-compat.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/glib-compat.h | 21 +++++++++++++++++++++
 tests/libqtest.c      | 10 ----------
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/include/glib-compat.h b/include/glib-compat.h
index 0cd24ffbe9..863c8cf73d 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -328,4 +328,25 @@ static inline void g_source_set_name_by_id(guint tag, const char *name)
 #define g_test_subprocess() (0)
 #endif
 
+
+#if !GLIB_CHECK_VERSION(2, 34, 0)
+static inline void
+g_test_add_data_func_full(const char *path,
+                          gpointer data,
+                          gpointer fn,
+                          gpointer data_free_func)
+{
+#if GLIB_CHECK_VERSION(2, 26, 0)
+    /* back-compat casts, remove this once we can require new-enough glib */
+    g_test_add_vtable(path, 0, data, NULL,
+                      (GTestFixtureFunc)fn, (GTestFixtureFunc) data_free_func);
+#else
+    /* back-compat casts, remove this once we can require new-enough glib */
+    g_test_add_vtable(path, 0, data, NULL,
+                      (void (*)(void)) fn, (void (*)(void)) data_free_func);
+#endif
+}
+#endif
+
+
 #endif
diff --git a/tests/libqtest.c b/tests/libqtest.c
index d8fba6647a..93b5deac23 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -805,17 +805,7 @@ void qtest_add_data_func_full(const char *str, void *data,
                               GDestroyNotify data_free_func)
 {
     gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
-#if GLIB_CHECK_VERSION(2, 34, 0)
     g_test_add_data_func_full(path, data, fn, data_free_func);
-#elif GLIB_CHECK_VERSION(2, 26, 0)
-    /* back-compat casts, remove this once we can require new-enough glib */
-    g_test_add_vtable(path, 0, data, NULL,
-                      (GTestFixtureFunc)fn, (GTestFixtureFunc) data_free_func);
-#else
-    /* back-compat casts, remove this once we can require new-enough glib */
-    g_test_add_vtable(path, 0, data, NULL,
-                      (void (*)(void)) fn, (void (*)(void)) data_free_func);
-#endif
     g_free(path);
 }
 
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 06/30] tests: fix ptimer leaks

[Marc-André Lureau]

Spotted by ASAN.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/hw/ptimer.h       |   1 +
 hw/core/ptimer.c          |   8 +++
 tests/ptimer-test-stubs.c |   5 ++
 tests/ptimer-test.c       | 122 ++++++++++++++++++++++++++++------------------
 4 files changed, 89 insertions(+), 47 deletions(-)

diff --git a/include/hw/ptimer.h b/include/hw/ptimer.h
index 48cccbdb51..eafc3f0a86 100644
--- a/include/hw/ptimer.h
+++ b/include/hw/ptimer.h
@@ -60,6 +60,7 @@ typedef struct ptimer_state ptimer_state;
 typedef void (*ptimer_cb)(void *opaque);
 
 ptimer_state *ptimer_init(QEMUBH *bh, uint8_t policy_mask);
+void ptimer_free(ptimer_state *s);
 void ptimer_set_period(ptimer_state *s, int64_t period);
 void ptimer_set_freq(ptimer_state *s, uint32_t freq);
 uint64_t ptimer_get_limit(ptimer_state *s);
diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
index 3af82afe78..59ccb00550 100644
--- a/hw/core/ptimer.c
+++ b/hw/core/ptimer.c
@@ -12,6 +12,7 @@
 #include "qemu/host-utils.h"
 #include "sysemu/replay.h"
 #include "sysemu/qtest.h"
+#include "block/aio.h"
 
 #define DELTA_ADJUST     1
 #define DELTA_NO_ADJUST -1
@@ -353,3 +354,10 @@ ptimer_state *ptimer_init(QEMUBH *bh, uint8_t policy_mask)
     s->policy_mask = policy_mask;
     return s;
 }
+
+void ptimer_free(ptimer_state *s)
+{
+    qemu_bh_delete(s->bh);
+    timer_free(s->timer);
+    g_free(s);
+}
diff --git a/tests/ptimer-test-stubs.c b/tests/ptimer-test-stubs.c
index 21d4ebb0fe..8a1b0a336c 100644
--- a/tests/ptimer-test-stubs.c
+++ b/tests/ptimer-test-stubs.c
@@ -108,6 +108,11 @@ QEMUBH *qemu_bh_new(QEMUBHFunc *cb, void *opaque)
     return bh;
 }
 
+void qemu_bh_delete(QEMUBH *bh)
+{
+    g_free(bh);
+}
+
 void replay_bh_schedule_event(QEMUBH *bh)
 {
     bh->cb(bh->opaque);
diff --git a/tests/ptimer-test.c b/tests/ptimer-test.c
index b36a476483..5d1a2a8188 100644
--- a/tests/ptimer-test.c
+++ b/tests/ptimer-test.c
@@ -73,6 +73,7 @@ static void check_set_count(gconstpointer arg)
     ptimer_set_count(ptimer, 1000);
     g_assert_cmpuint(ptimer_get_count(ptimer), ==, 1000);
     g_assert_false(triggered);
+    ptimer_free(ptimer);
 }
 
 static void check_set_limit(gconstpointer arg)
@@ -92,6 +93,7 @@ static void check_set_limit(gconstpointer arg)
     g_assert_cmpuint(ptimer_get_count(ptimer), ==, 2000);
     g_assert_cmpuint(ptimer_get_limit(ptimer), ==, 2000);
     g_assert_false(triggered);
+    ptimer_free(ptimer);
 }
 
 static void check_oneshot(gconstpointer arg)
@@ -194,6 +196,7 @@ static void check_oneshot(gconstpointer arg)
 
     g_assert_cmpuint(ptimer_get_count(ptimer), ==, 0);
     g_assert_false(triggered);
+    ptimer_free(ptimer);
 }
 
 static void check_periodic(gconstpointer arg)
@@ -360,6 +363,7 @@ static void check_periodic(gconstpointer arg)
     g_assert_cmpuint(ptimer_get_count(ptimer), ==,
                     (no_round_down ? 8 : 7) + (wrap_policy ? 1 : 0));
     g_assert_false(triggered);
+    ptimer_free(ptimer);
 }
 
 static void check_on_the_fly_mode_change(gconstpointer arg)
@@ -406,6 +410,7 @@ static void check_on_the_fly_mode_change(gconstpointer arg)
 
     g_assert_cmpuint(ptimer_get_count(ptimer), ==, 0);
     g_assert_true(triggered);
+    ptimer_free(ptimer);
 }
 
 static void check_on_the_fly_period_change(gconstpointer arg)
@@ -438,6 +443,7 @@ static void check_on_the_fly_period_change(gconstpointer arg)
 
     g_assert_cmpuint(ptimer_get_count(ptimer), ==, 0);
     g_assert_true(triggered);
+    ptimer_free(ptimer);
 }
 
 static void check_on_the_fly_freq_change(gconstpointer arg)
@@ -470,6 +476,7 @@ static void check_on_the_fly_freq_change(gconstpointer arg)
 
     g_assert_cmpuint(ptimer_get_count(ptimer), ==, 0);
     g_assert_true(triggered);
+    ptimer_free(ptimer);
 }
 
 static void check_run_with_period_0(gconstpointer arg)
@@ -487,6 +494,7 @@ static void check_run_with_period_0(gconstpointer arg)
 
     g_assert_cmpuint(ptimer_get_count(ptimer), ==, 99);
     g_assert_false(triggered);
+    ptimer_free(ptimer);
 }
 
 static void check_run_with_delta_0(gconstpointer arg)
@@ -591,6 +599,7 @@ static void check_run_with_delta_0(gconstpointer arg)
     g_assert_true(triggered);
 
     ptimer_stop(ptimer);
+    ptimer_free(ptimer);
 }
 
 static void check_periodic_with_load_0(gconstpointer arg)
@@ -649,6 +658,7 @@ static void check_periodic_with_load_0(gconstpointer arg)
     }
 
     ptimer_stop(ptimer);
+    ptimer_free(ptimer);
 }
 
 static void check_oneshot_with_load_0(gconstpointer arg)
@@ -682,14 +692,14 @@ static void check_oneshot_with_load_0(gconstpointer arg)
     } else {
         g_assert_false(triggered);
     }
+
+    ptimer_free(ptimer);
 }
 
 static void add_ptimer_tests(uint8_t policy)
 {
-    uint8_t *ppolicy = g_malloc(1);
-    char *policy_name = g_malloc0(256);
-
-    *ppolicy = policy;
+    char policy_name[256] = "";
+    char *tmp;
 
     if (policy == PTIMER_POLICY_DEFAULT) {
         g_sprintf(policy_name, "default");
@@ -715,49 +725,67 @@ static void add_ptimer_tests(uint8_t policy)
         g_strlcat(policy_name, "no_counter_rounddown,", 256);
     }
 
-    g_test_add_data_func(
-        g_strdup_printf("/ptimer/set_count policy=%s", policy_name),
-        ppolicy, check_set_count);
-
-    g_test_add_data_func(
-        g_strdup_printf("/ptimer/set_limit policy=%s", policy_name),
-        ppolicy, check_set_limit);
-
-    g_test_add_data_func(
-        g_strdup_printf("/ptimer/oneshot policy=%s", policy_name),
-        ppolicy, check_oneshot);
-
-    g_test_add_data_func(
-        g_strdup_printf("/ptimer/periodic policy=%s", policy_name),
-        ppolicy, check_periodic);
-
-    g_test_add_data_func(
-        g_strdup_printf("/ptimer/on_the_fly_mode_change policy=%s", policy_name),
-        ppolicy, check_on_the_fly_mode_change);
-
-    g_test_add_data_func(
-        g_strdup_printf("/ptimer/on_the_fly_period_change policy=%s", policy_name),
-        ppolicy, check_on_the_fly_period_change);
-
-    g_test_add_data_func(
-        g_strdup_printf("/ptimer/on_the_fly_freq_change policy=%s", policy_name),
-        ppolicy, check_on_the_fly_freq_change);
-
-    g_test_add_data_func(
-        g_strdup_printf("/ptimer/run_with_period_0 policy=%s", policy_name),
-        ppolicy, check_run_with_period_0);
-
-    g_test_add_data_func(
-        g_strdup_printf("/ptimer/run_with_delta_0 policy=%s", policy_name),
-        ppolicy, check_run_with_delta_0);
-
-    g_test_add_data_func(
-        g_strdup_printf("/ptimer/periodic_with_load_0 policy=%s", policy_name),
-        ppolicy, check_periodic_with_load_0);
-
-    g_test_add_data_func(
-        g_strdup_printf("/ptimer/oneshot_with_load_0 policy=%s", policy_name),
-        ppolicy, check_oneshot_with_load_0);
+    g_test_add_data_func_full(
+        tmp = g_strdup_printf("/ptimer/set_count policy=%s", policy_name),
+        g_memdup(&policy, 1), check_set_count, g_free);
+    g_free(tmp);
+
+    g_test_add_data_func_full(
+        tmp = g_strdup_printf("/ptimer/set_limit policy=%s", policy_name),
+        g_memdup(&policy, 1), check_set_limit, g_free);
+    g_free(tmp);
+
+    g_test_add_data_func_full(
+        tmp = g_strdup_printf("/ptimer/oneshot policy=%s", policy_name),
+        g_memdup(&policy, 1), check_oneshot, g_free);
+    g_free(tmp);
+
+    g_test_add_data_func_full(
+        tmp = g_strdup_printf("/ptimer/periodic policy=%s", policy_name),
+        g_memdup(&policy, 1), check_periodic, g_free);
+    g_free(tmp);
+
+    g_test_add_data_func_full(
+        tmp = g_strdup_printf("/ptimer/on_the_fly_mode_change policy=%s",
+                              policy_name),
+        g_memdup(&policy, 1), check_on_the_fly_mode_change, g_free);
+    g_free(tmp);
+
+    g_test_add_data_func_full(
+        tmp = g_strdup_printf("/ptimer/on_the_fly_period_change policy=%s",
+                              policy_name),
+        g_memdup(&policy, 1), check_on_the_fly_period_change, g_free);
+    g_free(tmp);
+
+    g_test_add_data_func_full(
+        tmp = g_strdup_printf("/ptimer/on_the_fly_freq_change policy=%s",
+                              policy_name),
+        g_memdup(&policy, 1), check_on_the_fly_freq_change, g_free);
+    g_free(tmp);
+
+    g_test_add_data_func_full(
+        tmp = g_strdup_printf("/ptimer/run_with_period_0 policy=%s",
+                              policy_name),
+        g_memdup(&policy, 1), check_run_with_period_0, g_free);
+    g_free(tmp);
+
+    g_test_add_data_func_full(
+        tmp = g_strdup_printf("/ptimer/run_with_delta_0 policy=%s",
+                              policy_name),
+        g_memdup(&policy, 1), check_run_with_delta_0, g_free);
+    g_free(tmp);
+
+    g_test_add_data_func_full(
+        tmp = g_strdup_printf("/ptimer/periodic_with_load_0 policy=%s",
+                              policy_name),
+        g_memdup(&policy, 1), check_periodic_with_load_0, g_free);
+    g_free(tmp);
+
+    g_test_add_data_func_full(
+        tmp = g_strdup_printf("/ptimer/oneshot_with_load_0 policy=%s",
+                              policy_name),
+        g_memdup(&policy, 1), check_oneshot_with_load_0, g_free);
+    g_free(tmp);
 }
 
 static void add_all_ptimer_policies_comb_tests(void)
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 07/30] tests: fix endianness-test leaks

[Marc-André Lureau]

Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/endianness-test.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/endianness-test.c b/tests/endianness-test.c
index cf8d41b7b4..ed0bf52019 100644
--- a/tests/endianness-test.c
+++ b/tests/endianness-test.c
@@ -295,14 +295,17 @@ int main(int argc, char **argv)
         path = g_strdup_printf("endianness/%s",
                                test_cases[i].machine);
         qtest_add_data_func(path, &test_cases[i], test_endianness);
+        g_free(path);
 
         path = g_strdup_printf("endianness/split/%s",
                                test_cases[i].machine);
         qtest_add_data_func(path, &test_cases[i], test_endianness_split);
+        g_free(path);
 
         path = g_strdup_printf("endianness/combine/%s",
                                test_cases[i].machine);
         qtest_add_data_func(path, &test_cases[i], test_endianness_combine);
+        g_free(path);
     }
 
     return g_test_run();
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 08/30] tests: fix q35-test leaks

[Marc-André Lureau]

Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/q35-test.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/q35-test.c b/tests/q35-test.c
index 763fe3d6ae..e010d781e7 100644
--- a/tests/q35-test.c
+++ b/tests/q35-test.c
@@ -71,6 +71,9 @@ static void test_smram_lock(void)
     g_assert(smram_test_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN) == false);
     smram_set_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN, true);
     g_assert(smram_test_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN) == true);
+
+    qpci_free_pc(pcibus);
+    g_free(pcidev);
 }
 
 int main(int argc, char **argv)
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 09/30] tests: fix vhost-user-test leaks

[Marc-André Lureau]

Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/libqos/virtio-pci.h |  2 ++
 tests/libqos/virtio-pci.c |  6 ++++++
 tests/vhost-user-test.c   | 11 +++++++----
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/tests/libqos/virtio-pci.h b/tests/libqos/virtio-pci.h
index 38c54c63ea..0fab916cf8 100644
--- a/tests/libqos/virtio-pci.h
+++ b/tests/libqos/virtio-pci.h
@@ -34,6 +34,8 @@ extern const QVirtioBus qvirtio_pci;
 void qvirtio_pci_foreach(QPCIBus *bus, uint16_t device_type,
                 void (*func)(QVirtioDevice *d, void *data), void *data);
 QVirtioPCIDevice *qvirtio_pci_device_find(QPCIBus *bus, uint16_t device_type);
+void qvirtio_pci_device_free(QVirtioPCIDevice *dev);
+
 void qvirtio_pci_device_enable(QVirtioPCIDevice *d);
 void qvirtio_pci_device_disable(QVirtioPCIDevice *d);
 
diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
index d4bf841f23..456cccdc7b 100644
--- a/tests/libqos/virtio-pci.c
+++ b/tests/libqos/virtio-pci.c
@@ -27,6 +27,12 @@ typedef struct QVirtioPCIForeachData {
     void *user_data;
 } QVirtioPCIForeachData;
 
+void qvirtio_pci_device_free(QVirtioPCIDevice *dev)
+{
+    g_free(dev->pdev);
+    g_free(dev);
+}
+
 static QVirtioPCIDevice *qpcidevice_to_qvirtiodevice(QPCIDevice *pdev)
 {
     QVirtioPCIDevice *vpcidev;
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 2c45c7b29f..a61896c32d 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -139,6 +139,7 @@ enum {
 };
 
 typedef struct TestServer {
+    QPCIBus *bus;
     gchar *socket_path;
     gchar *mig_path;
     gchar *chr_name;
@@ -160,14 +161,13 @@ static const char *root;
 
 static void init_virtio_dev(TestServer *s)
 {
-    QPCIBus *bus;
     QVirtioPCIDevice *dev;
     uint32_t features;
 
-    bus = qpci_init_pc(NULL);
-    g_assert_nonnull(bus);
+    s->bus = qpci_init_pc(NULL);
+    g_assert_nonnull(s->bus);
 
-    dev = qvirtio_pci_device_find(bus, VIRTIO_ID_NET);
+    dev = qvirtio_pci_device_find(s->bus, VIRTIO_ID_NET);
     g_assert_nonnull(dev);
 
     qvirtio_pci_device_enable(dev);
@@ -180,6 +180,7 @@ static void init_virtio_dev(TestServer *s)
     qvirtio_set_features(&dev->vdev, features);
 
     qvirtio_set_driver_ok(&dev->vdev);
+    qvirtio_pci_device_free(dev);
 }
 
 static void wait_for_fds(TestServer *s)
@@ -507,6 +508,8 @@ static gboolean _test_server_free(TestServer *server)
     g_free(server->mig_path);
 
     g_free(server->chr_name);
+    qpci_free_pc(server->bus);
+
     g_free(server);
 
     return FALSE;
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 10/30] tests: fix ide-test leaks

[Marc-André Lureau]

Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 tests/ide-test.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tests/ide-test.c b/tests/ide-test.c
index fb541f88b5..2fa97bc36b 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -339,6 +339,7 @@ static void test_bmdma_simple_rw(void)
     g_assert(memcmp(buf, cmpbuf, len) == 0);
 
 
+    free_pci_device(dev);
     g_free(buf);
     g_free(cmpbuf);
 }
@@ -369,6 +370,7 @@ static void test_bmdma_short_prdt(void)
                               prdt, ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, 0);
     assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
+    free_pci_device(dev);
 }
 
 static void test_bmdma_one_sector_short_prdt(void)
@@ -398,6 +400,7 @@ static void test_bmdma_one_sector_short_prdt(void)
                               prdt, ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, 0);
     assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
+    free_pci_device(dev);
 }
 
 static void test_bmdma_long_prdt(void)
@@ -426,6 +429,7 @@ static void test_bmdma_long_prdt(void)
                               prdt, ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, BM_STS_INTR);
     assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
+    free_pci_device(dev);
 }
 
 static void test_bmdma_no_busmaster(void)
@@ -449,6 +453,7 @@ static void test_bmdma_no_busmaster(void)
      * in practice. At least we want to be aware of any changes. */
     g_assert_cmphex(status, ==, BM_STS_ACTIVE | BM_STS_INTR);
     assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
+    free_pci_device(dev);
 }
 
 static void test_bmdma_setup(void)
@@ -525,6 +530,7 @@ static void test_identify(void)
     assert_bit_set(buf[85], 0x20);
 
     ide_test_quit();
+    free_pci_device(dev);
 }
 
 /*
@@ -562,6 +568,7 @@ static void make_dirty(uint8_t device)
     assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
 
     g_free(buf);
+    free_pci_device(dev);
 }
 
 static void test_flush(void)
@@ -608,6 +615,7 @@ static void test_flush(void)
     assert_bit_clear(data, BSY | DF | ERR | DRQ);
 
     ide_test_quit();
+    free_pci_device(dev);
 }
 
 static void test_retry_flush(const char *machine)
@@ -658,6 +666,7 @@ static void test_retry_flush(const char *machine)
     assert_bit_clear(data, BSY | DF | ERR | DRQ);
 
     ide_test_quit();
+    free_pci_device(dev);
 }
 
 static void test_flush_nodev(void)
@@ -675,6 +684,7 @@ static void test_flush_nodev(void)
 
     /* Just testing that qemu doesn't crash... */
 
+    free_pci_device(dev);
     ide_test_quit();
 }
 
@@ -741,6 +751,7 @@ static uint8_t ide_wait_clear(uint8_t flag)
     while (true) {
         data = qpci_io_readb(dev, ide_bar, reg_status);
         if (!(data & flag)) {
+            free_pci_device(dev);
             return data;
         }
         if (difftime(time(NULL), st) > 5.0) {
@@ -850,6 +861,7 @@ static void cdrom_pio_impl(int nblocks)
     g_free(pattern);
     g_free(rx);
     test_bmdma_teardown();
+    free_pci_device(dev);
 }
 
 static void test_cdrom_pio(void)
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 11/30] tests: fix hd-geo-test leaks

[Marc-André Lureau]

Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/hd-geo-test.c | 47 +++++++++++++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
index 6176e81ab2..88f8d76d32 100644
--- a/tests/hd-geo-test.c
+++ b/tests/hd-geo-test.c
@@ -19,6 +19,8 @@
 #include "qemu-common.h"
 #include "libqtest.h"
 
+#define ARGV_SIZE 256
+
 static char *create_test_img(int secs)
 {
     char *template = strdup("/tmp/qtest.XXXXXX");
@@ -66,7 +68,7 @@ static const CHST hd_chst[backend_last][mbr_last] = {
     },
 };
 
-static const char *img_file_name[backend_last];
+static char *img_file_name[backend_last];
 
 static const CHST *cur_ide[4];
 
@@ -234,28 +236,34 @@ static int setup_ide(int argc, char *argv[], int argv_sz,
  */
 static void test_ide_none(void)
 {
-    char *argv[256];
+    char **argv = g_new0(char *, ARGV_SIZE);
+    char *tmp;
 
-    setup_common(argv, ARRAY_SIZE(argv));
-    qtest_start(g_strjoinv(" ", argv));
+    setup_common(argv, ARGV_SIZE);
+    qtest_start(tmp = g_strjoinv(" ", argv));
+    g_strfreev(argv);
+    g_free(tmp);
     test_cmos();
     qtest_end();
 }
 
 static void test_ide_mbr(bool use_device, MBRcontents mbr)
 {
-    char *argv[256];
+    char **argv = g_new0(char *, ARGV_SIZE);
+    char *tmp;
     int argc;
     Backend i;
     const char *dev;
 
-    argc = setup_common(argv, ARRAY_SIZE(argv));
+    argc = setup_common(argv, ARGV_SIZE);
     for (i = 0; i < backend_last; i++) {
         cur_ide[i] = &hd_chst[i][mbr];
         dev = use_device ? (is_hd(cur_ide[i]) ? "ide-hd" : "ide-cd") : NULL;
-        argc = setup_ide(argc, argv, ARRAY_SIZE(argv), i, dev, i, mbr, "");
+        argc = setup_ide(argc, argv, ARGV_SIZE, i, dev, i, mbr, "");
     }
-    qtest_start(g_strjoinv(" ", argv));
+    qtest_start(tmp = g_strjoinv(" ", argv));
+    g_strfreev(argv);
+    g_free(tmp);
     test_cmos();
     qtest_end();
 }
@@ -310,12 +318,13 @@ static void test_ide_device_mbr_chs(void)
 
 static void test_ide_drive_user(const char *dev, bool trans)
 {
-    char *argv[256], *opts;
+    char **argv = g_new0(char *, ARGV_SIZE);
+    char *tmp, *opts;
     int argc;
     int secs = img_secs[backend_small];
     const CHST expected_chst = { secs / (4 * 32) , 4, 32, trans };
 
-    argc = setup_common(argv, ARRAY_SIZE(argv));
+    argc = setup_common(argv, ARGV_SIZE);
     opts = g_strdup_printf("%s,%s%scyls=%d,heads=%d,secs=%d",
                            dev ?: "",
                            trans && dev ? "bios-chs-" : "",
@@ -323,11 +332,13 @@ static void test_ide_drive_user(const char *dev, bool trans)
                            expected_chst.cyls, expected_chst.heads,
                            expected_chst.secs);
     cur_ide[0] = &expected_chst;
-    argc = setup_ide(argc, argv, ARRAY_SIZE(argv),
+    argc = setup_ide(argc, argv, ARGV_SIZE,
                      0, dev ? opts : NULL, backend_small, mbr_chs,
                      dev ? "" : opts);
     g_free(opts);
-    qtest_start(g_strjoinv(" ", argv));
+    qtest_start(tmp = g_strjoinv(" ", argv));
+    g_strfreev(argv);
+    g_free(tmp);
     test_cmos();
     qtest_end();
 }
@@ -369,18 +380,21 @@ static void test_ide_device_user_chst(void)
  */
 static void test_ide_drive_cd_0(void)
 {
-    char *argv[256];
+    char **argv = g_new0(char *, ARGV_SIZE);
+    char *tmp;
     int argc, ide_idx;
     Backend i;
 
-    argc = setup_common(argv, ARRAY_SIZE(argv));
+    argc = setup_common(argv, ARGV_SIZE);
     for (i = 0; i <= backend_empty; i++) {
         ide_idx = backend_empty - i;
         cur_ide[ide_idx] = &hd_chst[i][mbr_blank];
-        argc = setup_ide(argc, argv, ARRAY_SIZE(argv),
+        argc = setup_ide(argc, argv, ARGV_SIZE,
                          ide_idx, NULL, i, mbr_blank, "");
     }
-    qtest_start(g_strjoinv(" ", argv));
+    qtest_start(tmp = g_strjoinv(" ", argv));
+    g_strfreev(argv);
+    g_free(tmp);
     test_cmos();
     qtest_end();
 }
@@ -418,6 +432,7 @@ int main(int argc, char **argv)
     for (i = 0; i < backend_last; i++) {
         if (img_file_name[i]) {
             unlink(img_file_name[i]);
+            free(img_file_name[i]);
         }
     }
 
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 12/30] tests: fix bios-tables-test leak

[Marc-André Lureau]

The inside array should be free too.
Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/bios-tables-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 54048050c0..d54018da73 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -144,7 +144,7 @@ static void free_test_data(test_data *data)
         g_free(temp->asl_file);
     }
 
-    g_array_free(data->tables, false);
+    g_array_free(data->tables, true);
 }
 
 static uint8_t acpi_checksum(const uint8_t *data, int len)
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 13/30] tests: fix ipmi-kcs-test leak

[Marc-André Lureau]

Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/ipmi-kcs-test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/ipmi-kcs-test.c b/tests/ipmi-kcs-test.c
index 9cf0b34a33..178ffc1797 100644
--- a/tests/ipmi-kcs-test.c
+++ b/tests/ipmi-kcs-test.c
@@ -279,6 +279,7 @@ int main(int argc, char **argv)
     cmdline = g_strdup_printf("-device ipmi-bmc-sim,id=bmc0"
                               " -device isa-ipmi-kcs,bmc=bmc0");
     qtest_start(cmdline);
+    g_free(cmdline);
     qtest_irq_intercept_in(global_qtest, "ioapic");
     qtest_add_func("/ipmi/local/kcs_base", test_kcs_base);
     qtest_add_func("/ipmi/local/kcs_abort", test_kcs_abort);
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 14/30] tests: fix ipmi-bt-test leak

[Marc-André Lureau]

Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/ipmi-bt-test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/ipmi-bt-test.c b/tests/ipmi-bt-test.c
index e84dd6889b..7e21a9bbcb 100644
--- a/tests/ipmi-bt-test.c
+++ b/tests/ipmi-bt-test.c
@@ -420,6 +420,7 @@ int main(int argc, char **argv)
           " -device ipmi-bmc-extern,chardev=ipmi0,id=bmc0"
           " -device isa-ipmi-bt,bmc=bmc0", emu_port);
     qtest_start(cmdline);
+    g_free(cmdline);
     qtest_irq_intercept_in(global_qtest, "ioapic");
     qtest_add_func("/ipmi/extern/connect", test_connect);
     qtest_add_func("/ipmi/extern/bt_base", test_bt_base);
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 15/30] pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice

[Marc-André Lureau]

From: Igor Mammedov <imammedo@redhat.com>

PCI hotplug for bridges was introduced only since 2.0 however
  acpi_set_bsel()->object_property_add_uint32_ptr(bus, ACPI_PCIHP_PROP_BSEL)
didn't take in account that for legacy mode (1.7) when
PCI hotplug for bridges is unavailable and ACPI_PCIHP_PROP_BSEL property
the only bus "PCI.0' has been created earlier at acpi_pcihp_init() time.

We managed to live with it only because of error rised by adding
a duplicate property in acpi_set_bsel() has been ignored which
resulted in useless leaking of just allocated (int)bus_bsel.

Issue affects only 1.7 machine type as ACPI tables supported by
QEMU were introduced at that time, but there wasn't PCI hotplug
for bridges till the next release (2.0).

Fix it by removing duplicate ACPI_PCIHP_PROP_BSEL intialization
in acpi_pcihp_init() and doing it only in one place acpi_set_pci_info().

PS:
do not ignore error returned by object_property_add_uint32_ptr()
and abort QEMU since it's programming error which should be fixed
instead of being ignored.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <1470211497-116801-1-git-send-email-imammedo@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
[ Marc-André - Remove now unused ACPI_PCIHP_LEGACY_SIZE ]
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/acpi/pcihp.c      | 11 -----------
 hw/i386/acpi-build.c |  4 ++--
 2 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index d957d1e30d..2b0f3e1bfb 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -49,7 +49,6 @@
 
 #define ACPI_PCIHP_ADDR 0xae00
 #define ACPI_PCIHP_SIZE 0x0014
-#define ACPI_PCIHP_LEGACY_SIZE 0x000f
 #define PCI_UP_BASE 0x0000
 #define PCI_DOWN_BASE 0x0004
 #define PCI_EJ_BASE 0x0008
@@ -302,16 +301,6 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
     s->root= root_bus;
     s->legacy_piix = !bridges_enabled;
 
-    if (s->legacy_piix) {
-        unsigned *bus_bsel = g_malloc(sizeof *bus_bsel);
-
-        s->io_len = ACPI_PCIHP_LEGACY_SIZE;
-
-        *bus_bsel = ACPI_PCIHP_BSEL_DEFAULT;
-        object_property_add_uint32_ptr(OBJECT(root_bus), ACPI_PCIHP_PROP_BSEL,
-                                       bus_bsel, NULL);
-    }
-
     memory_region_init_io(&s->io, owner, &acpi_pcihp_io_ops, s,
                           "acpi-pci-hotplug", s->io_len);
     memory_region_add_subregion(address_space_io, s->io_base, &s->io);
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 1c928abb28..f44767b9be 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -462,7 +462,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
 
         *bus_bsel = (*bsel_alloc)++;
         object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
-                                       bus_bsel, NULL);
+                                       bus_bsel, &error_abort);
     }
 
     return bsel_alloc;
@@ -471,7 +471,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
 static void acpi_set_pci_info(void)
 {
     PCIBus *bus = find_i440fx(); /* TODO: Q35 support */
-    unsigned bsel_alloc = 0;
+    unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
 
     if (bus) {
         /* Scan all PCI buses. Set property to enable acpi based hotplug. */
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 16/30] tests: fix eepro100-test leak

[Marc-André Lureau]

Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/eepro100-test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/eepro100-test.c b/tests/eepro100-test.c
index ed23258b0f..bdc8a67d57 100644
--- a/tests/eepro100-test.c
+++ b/tests/eepro100-test.c
@@ -54,6 +54,7 @@ int main(int argc, char **argv)
 
         path = g_strdup_printf("eepro100/%s", models[i]);
         qtest_add_data_func(path, models[i], test_device);
+        g_free(path);
     }
 
     return g_test_run();
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 17/30] tests: fix tco-test leaks

[Marc-André Lureau]

Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/tco-test.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/tests/tco-test.c b/tests/tco-test.c
index ef02ec5903..c4c264eb3d 100644
--- a/tests/tco-test.c
+++ b/tests/tco-test.c
@@ -42,11 +42,18 @@ typedef struct {
     bool noreboot;
     QPCIDevice *dev;
     QPCIBar tco_io_bar;
+    QPCIBus *bus;
 } TestData;
 
+static void test_end(TestData *d)
+{
+    g_free(d->dev);
+    qpci_free_pc(d->bus);
+    qtest_end();
+}
+
 static void test_init(TestData *d)
 {
-    QPCIBus *bus;
     QTestState *qs;
     char *s;
 
@@ -57,8 +64,8 @@ static void test_init(TestData *d)
     qtest_irq_intercept_in(qs, "ioapic");
     g_free(s);
 
-    bus = qpci_init_pc(NULL);
-    d->dev = qpci_device_find(bus, QPCI_DEVFN(0x1f, 0x00));
+    d->bus = qpci_init_pc(NULL);
+    d->dev = qpci_device_find(d->bus, QPCI_DEVFN(0x1f, 0x00));
     g_assert(d->dev != NULL);
 
     qpci_device_enable(d->dev);
@@ -148,7 +155,7 @@ static void test_tco_defaults(void)
                     SW_IRQ_GEN_DEFAULT);
     g_assert_cmpint(qpci_io_readw(d.dev, d.tco_io_bar, TCO_TMR), ==,
                     TCO_TMR_DEFAULT);
-    qtest_end();
+    test_end(&d);
 }
 
 static void test_tco_timeout(void)
@@ -192,7 +199,7 @@ static void test_tco_timeout(void)
     g_assert(ret == 1);
 
     stop_tco(&d);
-    qtest_end();
+    test_end(&d);
 }
 
 static void test_tco_max_timeout(void)
@@ -225,7 +232,7 @@ static void test_tco_max_timeout(void)
     g_assert(ret == 1);
 
     stop_tco(&d);
-    qtest_end();
+    test_end(&d);
 }
 
 static QDict *get_watchdog_action(void)
@@ -262,7 +269,7 @@ static void test_tco_second_timeout_pause(void)
     QDECREF(ad);
 
     stop_tco(&td);
-    qtest_end();
+    test_end(&td);
 }
 
 static void test_tco_second_timeout_reset(void)
@@ -287,7 +294,7 @@ static void test_tco_second_timeout_reset(void)
     QDECREF(ad);
 
     stop_tco(&td);
-    qtest_end();
+    test_end(&td);
 }
 
 static void test_tco_second_timeout_shutdown(void)
@@ -312,7 +319,7 @@ static void test_tco_second_timeout_shutdown(void)
     QDECREF(ad);
 
     stop_tco(&td);
-    qtest_end();
+    test_end(&td);
 }
 
 static void test_tco_second_timeout_none(void)
@@ -337,7 +344,7 @@ static void test_tco_second_timeout_none(void)
     QDECREF(ad);
 
     stop_tco(&td);
-    qtest_end();
+    test_end(&td);
 }
 
 static void test_tco_ticks_counter(void)
@@ -365,7 +372,7 @@ static void test_tco_ticks_counter(void)
     } while (!(qpci_io_readw(d.dev, d.tco_io_bar, TCO1_STS) & TCO_TIMEOUT));
 
     stop_tco(&d);
-    qtest_end();
+    test_end(&d);
 }
 
 static void test_tco1_control_bits(void)
@@ -383,7 +390,7 @@ static void test_tco1_control_bits(void)
     qpci_io_writew(d.dev, d.tco_io_bar, TCO1_CNT, val);
     g_assert_cmpint(qpci_io_readw(d.dev, d.tco_io_bar, TCO1_CNT), ==,
                     TCO_LOCK);
-    qtest_end();
+    test_end(&d);
 }
 
 static void test_tco1_status_bits(void)
@@ -412,7 +419,7 @@ static void test_tco1_status_bits(void)
     g_assert(ret == 1);
     qpci_io_writew(d.dev, d.tco_io_bar, TCO1_STS, val);
     g_assert_cmpint(qpci_io_readw(d.dev, d.tco_io_bar, TCO1_STS), ==, 0);
-    qtest_end();
+    test_end(&d);
 }
 
 static void test_tco2_status_bits(void)
@@ -439,7 +446,7 @@ static void test_tco2_status_bits(void)
     g_assert(ret == 1);
     qpci_io_writew(d.dev, d.tco_io_bar, TCO2_STS, val);
     g_assert_cmpint(qpci_io_readw(d.dev, d.tco_io_bar, TCO2_STS), ==, 0);
-    qtest_end();
+    test_end(&d);
 }
 
 int main(int argc, char **argv)
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 18/30] tests: fix e1000-test leak

[Marc-André Lureau]

Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/e1000-test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/e1000-test.c b/tests/e1000-test.c
index 59cab68a60..0c5fcdcc44 100644
--- a/tests/e1000-test.c
+++ b/tests/e1000-test.c
@@ -44,6 +44,7 @@ int main(int argc, char **argv)
 
         path = g_strdup_printf("e1000/%s", models[i]);
         qtest_add_data_func(path, models[i], test_device);
+        g_free(path);
     }
 
     return g_test_run();
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 19/30] tests: fix i440fx-test leaks

[Marc-André Lureau]

Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/i440fx-test.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c
index da2d5a53f0..e9d05c87d1 100644
--- a/tests/i440fx-test.c
+++ b/tests/i440fx-test.c
@@ -134,6 +134,8 @@ static void test_i440fx_defaults(gconstpointer opaque)
     /* 3.2.26 */
     g_assert_cmpint(qpci_config_readb(dev, 0x93), ==, 0x00); /* TRC */
 
+    g_free(dev);
+    qpci_free_pc(bus);
     qtest_end();
 }
 
@@ -270,6 +272,9 @@ static void test_i440fx_pam(gconstpointer opaque)
         /* Verify the area is not our new mask */
         g_assert(!verify_area(pam_area[i].start, pam_area[i].end, 0x82));
     }
+
+    g_free(dev);
+    qpci_free_pc(bus);
     qtest_end();
 }
 
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 20/30] tests: fix e1000e leaks

[Marc-André Lureau]

Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/e1000e-test.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/e1000e-test.c b/tests/e1000e-test.c
index 8c42ca919f..c612dc64ec 100644
--- a/tests/e1000e-test.c
+++ b/tests/e1000e-test.c
@@ -99,7 +99,10 @@ static QPCIBus *test_bus;
 
 static void e1000e_pci_foreach_callback(QPCIDevice *dev, int devfn, void *data)
 {
-    *(QPCIDevice **) data = dev;
+    QPCIDevice **res = data;
+
+    g_assert_null(*res);
+    *res = dev;
 }
 
 static QPCIDevice *e1000e_device_find(QPCIBus *bus)
@@ -403,6 +406,7 @@ static void data_test_clear(e1000e_device *d)
     e1000e_device_clear(test_bus, d);
     close(test_sockets[0]);
     pc_alloc_uninit(test_alloc);
+    g_free(d->pci_dev);
     qpci_free_pc(test_bus);
     qtest_end();
 }
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 21/30] tests: fix virtio-scsi-test leak

[Marc-André Lureau]

Spotted by ASAN.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
 tests/virtio-scsi-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
index 69220ef07b..0eabd56fd9 100644
--- a/tests/virtio-scsi-test.c
+++ b/tests/virtio-scsi-test.c
@@ -63,7 +63,7 @@ static void qvirtio_scsi_pci_free(QVirtIOSCSI *vs)
         qvirtqueue_cleanup(vs->dev->bus, vs->vq[i], vs->qs->alloc);
     }
     qvirtio_pci_device_disable(container_of(vs->dev, QVirtioPCIDevice, vdev));
-    g_free(vs->dev);
+    qvirtio_pci_device_free((QVirtioPCIDevice *)vs->dev);
     qvirtio_scsi_stop(vs->qs);
     g_free(vs);
 }
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 22/30] tests: fix virtio-9p-test leaks

[Marc-André Lureau]

Spotted by ASAN.

Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
 tests/virtio-9p-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
index 9556291567..43a1ad813f 100644
--- a/tests/virtio-9p-test.c
+++ b/tests/virtio-9p-test.c
@@ -80,7 +80,7 @@ static void qvirtio_9p_pci_stop(QVirtIO9P *v9p)
 {
     qvirtqueue_cleanup(v9p->dev->bus, v9p->vq, v9p->qs->alloc);
     qvirtio_pci_device_disable(container_of(v9p->dev, QVirtioPCIDevice, vdev));
-    g_free(v9p->dev);
+    qvirtio_pci_device_free((QVirtioPCIDevice *)v9p->dev);
     qvirtio_9p_stop(v9p);
 }
 
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 23/30] bus: do not unref hotplug handler

[Marc-André Lureau]

Apparently, none of the bus owner give a reference to the hotplug
handler property, do not unref it on bus release.

Furthermore, a bus is allowed to be its own hotplug handler, which can
be seen in qbus_set_bus_hotplug_handler() function. However, in this
case, the reference can't be given to the property, or this will create
a cyclic dependency and the bus will never be free.

Each bus owner should manage the lifecycle of the hotplug handler.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/core/bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/core/bus.c b/hw/core/bus.c
index cf383fc1af..4651f24486 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -197,7 +197,7 @@ static void qbus_initfn(Object *obj)
                              TYPE_HOTPLUG_HANDLER,
                              (Object **)&bus->hotplug_handler,
                              object_property_allow_set_link,
-                             OBJ_PROP_LINK_UNREF_ON_RELEASE,
+                             0,
                              NULL);
     object_property_add_bool(obj, "realized",
                              bus_get_realized, bus_set_realized, NULL);
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 24/30] usb: replace handle_destroy with unrealize

[Marc-André Lureau]

Curiously, unrealize() is not being used, but it seems more
appropriate than handle_destroy() together with realize(). It is more
ubiquitous destroy name in qemu code base and may throw errors.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/hw/usb.h              | 5 -----
 hw/usb/bus.c                  | 9 +++++----
 hw/usb/dev-audio.c            | 4 ++--
 hw/usb/dev-bluetooth.c        | 4 ++--
 hw/usb/dev-hid.c              | 4 ++--
 hw/usb/dev-hub.c              | 4 ++--
 hw/usb/dev-network.c          | 4 ++--
 hw/usb/dev-smartcard-reader.c | 4 ++--
 hw/usb/dev-uas.c              | 4 ++--
 hw/usb/dev-wacom.c            | 4 ++--
 hw/usb/host-libusb.c          | 4 ++--
 hw/usb/redirect.c             | 4 ++--
 12 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/include/hw/usb.h b/include/hw/usb.h
index c42b29c866..eb28655270 100644
--- a/include/hw/usb.h
+++ b/include/hw/usb.h
@@ -291,11 +291,6 @@ typedef struct USBDeviceClass {
     void (*cancel_packet)(USBDevice *dev, USBPacket *p);
 
     /*
-     * Called when device is destroyed.
-     */
-    void (*handle_destroy)(USBDevice *dev);
-
-    /*
      * Attach the device
      */
     void (*handle_attach)(USBDevice *dev);
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 1dcc35c8f8..efe4b8e1a6 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -136,11 +136,12 @@ USBDevice *usb_device_find_device(USBDevice *dev, uint8_t addr)
     return NULL;
 }
 
-static void usb_device_handle_destroy(USBDevice *dev)
+static void usb_device_unrealize(USBDevice *dev, Error **errp)
 {
     USBDeviceClass *klass = USB_DEVICE_GET_CLASS(dev);
-    if (klass->handle_destroy) {
-        klass->handle_destroy(dev);
+
+    if (klass->unrealize) {
+        klass->unrealize(dev, errp);
     }
 }
 
@@ -291,7 +292,7 @@ static void usb_qdev_unrealize(DeviceState *qdev, Error **errp)
     if (dev->attached) {
         usb_device_detach(dev);
     }
-    usb_device_handle_destroy(dev);
+    usb_device_unrealize(dev, errp);
     if (dev->port) {
         usb_release_port(dev);
     }
diff --git a/hw/usb/dev-audio.c b/hw/usb/dev-audio.c
index 87cab0a3d1..343345235c 100644
--- a/hw/usb/dev-audio.c
+++ b/hw/usb/dev-audio.c
@@ -617,7 +617,7 @@ static void usb_audio_handle_data(USBDevice *dev, USBPacket *p)
     }
 }
 
-static void usb_audio_handle_destroy(USBDevice *dev)
+static void usb_audio_unrealize(USBDevice *dev, Error **errp)
 {
     USBAudioState *s = USB_AUDIO(dev);
 
@@ -683,7 +683,7 @@ static void usb_audio_class_init(ObjectClass *klass, void *data)
     k->handle_reset   = usb_audio_handle_reset;
     k->handle_control = usb_audio_handle_control;
     k->handle_data    = usb_audio_handle_data;
-    k->handle_destroy = usb_audio_handle_destroy;
+    k->unrealize      = usb_audio_unrealize;
     k->set_interface  = usb_audio_set_interface;
 }
 
diff --git a/hw/usb/dev-bluetooth.c b/hw/usb/dev-bluetooth.c
index 91a4a0b8b9..443e3c301d 100644
--- a/hw/usb/dev-bluetooth.c
+++ b/hw/usb/dev-bluetooth.c
@@ -496,7 +496,7 @@ static void usb_bt_out_hci_packet_acl(void *opaque,
     usb_bt_fifo_enqueue(&s->acl, data, len);
 }
 
-static void usb_bt_handle_destroy(USBDevice *dev)
+static void usb_bt_unrealize(USBDevice *dev, Error **errp)
 {
     struct USBBtState *s = (struct USBBtState *) dev->opaque;
 
@@ -559,7 +559,7 @@ static void usb_bt_class_initfn(ObjectClass *klass, void *data)
     uc->handle_reset   = usb_bt_handle_reset;
     uc->handle_control = usb_bt_handle_control;
     uc->handle_data    = usb_bt_handle_data;
-    uc->handle_destroy = usb_bt_handle_destroy;
+    uc->unrealize      = usb_bt_unrealize;
     dc->vmsd = &vmstate_usb_bt;
     set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
 }
diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
index dda0bf0df0..c40019df96 100644
--- a/hw/usb/dev-hid.c
+++ b/hw/usb/dev-hid.c
@@ -690,7 +690,7 @@ static void usb_hid_handle_data(USBDevice *dev, USBPacket *p)
     }
 }
 
-static void usb_hid_handle_destroy(USBDevice *dev)
+static void usb_hid_unrealize(USBDevice *dev, Error **errp)
 {
     USBHIDState *us = USB_HID(dev);
 
@@ -785,7 +785,7 @@ static void usb_hid_class_initfn(ObjectClass *klass, void *data)
     uc->handle_reset   = usb_hid_handle_reset;
     uc->handle_control = usb_hid_handle_control;
     uc->handle_data    = usb_hid_handle_data;
-    uc->handle_destroy = usb_hid_handle_destroy;
+    uc->unrealize      = usb_hid_unrealize;
     uc->handle_attach  = usb_desc_attach;
 }
 
diff --git a/hw/usb/dev-hub.c b/hw/usb/dev-hub.c
index a33f21cb38..9fe7333946 100644
--- a/hw/usb/dev-hub.c
+++ b/hw/usb/dev-hub.c
@@ -497,7 +497,7 @@ static void usb_hub_handle_data(USBDevice *dev, USBPacket *p)
     }
 }
 
-static void usb_hub_handle_destroy(USBDevice *dev)
+static void usb_hub_unrealize(USBDevice *dev, Error **errp)
 {
     USBHubState *s = (USBHubState *)dev;
     int i;
@@ -575,7 +575,7 @@ static void usb_hub_class_initfn(ObjectClass *klass, void *data)
     uc->handle_reset   = usb_hub_handle_reset;
     uc->handle_control = usb_hub_handle_control;
     uc->handle_data    = usb_hub_handle_data;
-    uc->handle_destroy = usb_hub_handle_destroy;
+    uc->unrealize      = usb_hub_unrealize;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     dc->fw_name = "hub";
     dc->vmsd = &vmstate_usb_hub;
diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index c0f1193ba9..85fc81bf43 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -1324,7 +1324,7 @@ static void usbnet_cleanup(NetClientState *nc)
     s->nic = NULL;
 }
 
-static void usb_net_handle_destroy(USBDevice *dev)
+static void usb_net_unrealize(USBDevice *dev, Error **errp)
 {
     USBNetState *s = (USBNetState *) dev;
 
@@ -1428,7 +1428,7 @@ static void usb_net_class_initfn(ObjectClass *klass, void *data)
     uc->handle_reset   = usb_net_handle_reset;
     uc->handle_control = usb_net_handle_control;
     uc->handle_data    = usb_net_handle_data;
-    uc->handle_destroy = usb_net_handle_destroy;
+    uc->unrealize      = usb_net_unrealize;
     set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
     dc->fw_name = "network";
     dc->vmsd = &vmstate_usb_net;
diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 7cd4ed0d17..757b8b3f5a 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -1163,7 +1163,7 @@ static void ccid_handle_data(USBDevice *dev, USBPacket *p)
     }
 }
 
-static void ccid_handle_destroy(USBDevice *dev)
+static void ccid_unrealize(USBDevice *dev, Error **errp)
 {
     USBCCIDState *s = USB_CCID_DEV(dev);
 
@@ -1470,7 +1470,7 @@ static void ccid_class_initfn(ObjectClass *klass, void *data)
     uc->handle_reset   = ccid_handle_reset;
     uc->handle_control = ccid_handle_control;
     uc->handle_data    = ccid_handle_data;
-    uc->handle_destroy = ccid_handle_destroy;
+    uc->unrealize      = ccid_unrealize;
     dc->desc = "CCID Rev 1.1 smartcard reader";
     dc->vmsd = &ccid_vmstate;
     dc->props = ccid_properties;
diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index da2fb7017e..3b26655889 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -891,7 +891,7 @@ static void usb_uas_handle_data(USBDevice *dev, USBPacket *p)
     }
 }
 
-static void usb_uas_handle_destroy(USBDevice *dev)
+static void usb_uas_unrealize(USBDevice *dev, Error **errp)
 {
     UASDevice *uas = USB_UAS(dev);
 
@@ -944,7 +944,7 @@ static void usb_uas_class_initfn(ObjectClass *klass, void *data)
     uc->handle_reset   = usb_uas_handle_reset;
     uc->handle_control = usb_uas_handle_control;
     uc->handle_data    = usb_uas_handle_data;
-    uc->handle_destroy = usb_uas_handle_destroy;
+    uc->unrealize      = usb_uas_unrealize;
     uc->attached_settable = true;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->fw_name = "storage";
diff --git a/hw/usb/dev-wacom.c b/hw/usb/dev-wacom.c
index c4702dbba0..bf70013059 100644
--- a/hw/usb/dev-wacom.c
+++ b/hw/usb/dev-wacom.c
@@ -329,7 +329,7 @@ static void usb_wacom_handle_data(USBDevice *dev, USBPacket *p)
     }
 }
 
-static void usb_wacom_handle_destroy(USBDevice *dev)
+static void usb_wacom_unrealize(USBDevice *dev, Error **errp)
 {
     USBWacomState *s = (USBWacomState *) dev;
 
@@ -364,7 +364,7 @@ static void usb_wacom_class_init(ObjectClass *klass, void *data)
     uc->handle_reset   = usb_wacom_handle_reset;
     uc->handle_control = usb_wacom_handle_control;
     uc->handle_data    = usb_wacom_handle_data;
-    uc->handle_destroy = usb_wacom_handle_destroy;
+    uc->unrealize      = usb_wacom_unrealize;
     set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
     dc->desc = "QEMU PenPartner Tablet";
     dc->vmsd = &vmstate_usb_wacom;
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 7791c6d520..c9876a5b0f 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1065,7 +1065,7 @@ static void usb_host_instance_init(Object *obj)
                                   &udev->qdev, NULL);
 }
 
-static void usb_host_handle_destroy(USBDevice *udev)
+static void usb_host_unrealize(USBDevice *udev, Error **errp)
 {
     USBHostDevice *s = USB_HOST_DEVICE(udev);
 
@@ -1568,7 +1568,7 @@ static void usb_host_class_initfn(ObjectClass *klass, void *data)
     uc->handle_data    = usb_host_handle_data;
     uc->handle_control = usb_host_handle_control;
     uc->handle_reset   = usb_host_handle_reset;
-    uc->handle_destroy = usb_host_handle_destroy;
+    uc->unrealize      = usb_host_unrealize;
     uc->flush_ep_queue = usb_host_flush_ep_queue;
     uc->alloc_streams  = usb_host_alloc_streams;
     uc->free_streams   = usb_host_free_streams;
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 860f5c35eb..0efe62f725 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1427,7 +1427,7 @@ static void usbredir_cleanup_device_queues(USBRedirDevice *dev)
     }
 }
 
-static void usbredir_handle_destroy(USBDevice *udev)
+static void usbredir_unrealize(USBDevice *udev, Error **errp)
 {
     USBRedirDevice *dev = USB_REDIRECT(udev);
     Chardev *chr = qemu_chr_fe_get_driver(&dev->cs);
@@ -2513,7 +2513,7 @@ static void usbredir_class_initfn(ObjectClass *klass, void *data)
 
     uc->realize        = usbredir_realize;
     uc->product_desc   = "USB Redirection Device";
-    uc->handle_destroy = usbredir_handle_destroy;
+    uc->unrealize      = usbredir_unrealize;
     uc->cancel_packet  = usbredir_cancel_packet;
     uc->handle_reset   = usbredir_handle_reset;
     uc->handle_data    = usbredir_handle_data;
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 25/30] usb: release the created buses

[Marc-André Lureau]

Leaks spotted by ASAN.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/usb/dev-storage.c | 16 ++++++++++++++++
 hw/usb/dev-uas.c     |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index c607f7606d..74b7856278 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -589,6 +589,13 @@ static const struct SCSIBusInfo usb_msd_scsi_info_bot = {
     .load_request = usb_msd_load_request,
 };
 
+static void usb_msd_unrealize_storage(USBDevice *dev, Error **errp)
+{
+    MSDState *s = USB_STORAGE_DEV(dev);
+
+    object_unref(OBJECT(&s->bus));
+}
+
 static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
 {
     MSDState *s = USB_STORAGE_DEV(dev);
@@ -635,6 +642,13 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
     s->scsi_dev = scsi_dev;
 }
 
+static void usb_msd_unrealize_bot(USBDevice *dev, Error **errp)
+{
+    MSDState *s = USB_STORAGE_DEV(dev);
+
+    object_unref(OBJECT(&s->bus));
+}
+
 static void usb_msd_realize_bot(USBDevice *dev, Error **errp)
 {
     MSDState *s = USB_STORAGE_DEV(dev);
@@ -755,6 +769,7 @@ static void usb_msd_class_initfn_storage(ObjectClass *klass, void *data)
     USBDeviceClass *uc = USB_DEVICE_CLASS(klass);
 
     uc->realize = usb_msd_realize_storage;
+    uc->unrealize = usb_msd_unrealize_storage;
     dc->props = msd_properties;
 }
 
@@ -817,6 +832,7 @@ static void usb_msd_class_initfn_bot(ObjectClass *klass, void *data)
     USBDeviceClass *uc = USB_DEVICE_CLASS(klass);
 
     uc->realize = usb_msd_realize_bot;
+    uc->unrealize = usb_msd_unrealize_bot;
     uc->attached_settable = true;
 }
 
diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index 3b26655889..fffc424396 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -896,6 +896,8 @@ static void usb_uas_unrealize(USBDevice *dev, Error **errp)
     UASDevice *uas = USB_UAS(dev);
 
     qemu_bh_delete(uas->status_bh);
+
+    object_unref(OBJECT(&uas->bus));
 }
 
 static void usb_uas_realize(USBDevice *dev, Error **errp)
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 26/30] tests: allows to run single test in usb-hcd-ehci-test

[Marc-André Lureau]

pci_init() shouldn't be a test function, but instead called before any
test. This allows to run a single test with -p /x86_64/ehci/....

Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/usb-hcd-ehci-test.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tests/usb-hcd-ehci-test.c b/tests/usb-hcd-ehci-test.c
index 57af8a034e..fba84b4bee 100644
--- a/tests/usb-hcd-ehci-test.c
+++ b/tests/usb-hcd-ehci-test.c
@@ -50,11 +50,8 @@ static void ehci_port_test(struct qhc *hc, int port, uint32_t expect)
 
 /* tests */
 
-static void pci_init(void)
+static void test_init(void)
 {
-    if (pcibus) {
-        return;
-    }
     pcibus = qpci_init_pc(NULL);
     g_assert(pcibus != NULL);
 
@@ -142,7 +139,7 @@ int main(int argc, char **argv)
     int ret;
 
     g_test_init(&argc, &argv, NULL);
-    qtest_add_func("/ehci/pci/init", pci_init);
+
     qtest_add_func("/ehci/pci/uhci-port-1", pci_uhci_port_1);
     qtest_add_func("/ehci/pci/ehci-port-1", pci_ehci_port_1);
     qtest_add_func("/ehci/pci/ehci-config", pci_ehci_config);
@@ -161,6 +158,8 @@ int main(int argc, char **argv)
                 "-drive if=none,id=usbcdrom,media=cdrom "
                 "-device usb-tablet,bus=ich9-ehci-1.0,port=1,usb_version=1 "
                 "-device usb-storage,bus=ich9-ehci-1.0,port=2,drive=usbcdrom ");
+
+    test_init();
     ret = g_test_run();
 
     qtest_end();
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 27/30] tests: fix usb-test leaks

[Marc-André Lureau]

Fix the usb tests leaks.

Spotted by ASAN.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/libqos/usb.h        |  1 +
 tests/libqos/usb.c        |  5 +++++
 tests/usb-hcd-ehci-test.c | 10 ++++++++++
 tests/usb-hcd-uhci-test.c |  1 +
 4 files changed, 17 insertions(+)

diff --git a/tests/libqos/usb.h b/tests/libqos/usb.h
index 423dcfd82f..297cfc564d 100644
--- a/tests/libqos/usb.h
+++ b/tests/libqos/usb.h
@@ -11,6 +11,7 @@ struct qhc {
 void qusb_pci_init_one(QPCIBus *pcibus, struct qhc *hc,
                        uint32_t devfn, int bar);
 void uhci_port_test(struct qhc *hc, int port, uint16_t expect);
+void uhci_deinit(struct qhc *hc);
 
 void usb_test_hotplug(const char *bus_name, const int port,
                       void (*port_check)(void));
diff --git a/tests/libqos/usb.c b/tests/libqos/usb.c
index 98408d93f3..0cdfaecda7 100644
--- a/tests/libqos/usb.c
+++ b/tests/libqos/usb.c
@@ -24,6 +24,11 @@ void qusb_pci_init_one(QPCIBus *pcibus, struct qhc *hc, uint32_t devfn, int bar)
     hc->bar = qpci_iomap(hc->dev, bar, NULL);
 }
 
+void uhci_deinit(struct qhc *hc)
+{
+    g_free(hc->dev);
+}
+
 void uhci_port_test(struct qhc *hc, int port, uint16_t expect)
 {
     uint16_t value = qpci_io_readw(hc->dev, hc->bar, 0x10 + 2 * port);
diff --git a/tests/usb-hcd-ehci-test.c b/tests/usb-hcd-ehci-test.c
index fba84b4bee..944eb1c088 100644
--- a/tests/usb-hcd-ehci-test.c
+++ b/tests/usb-hcd-ehci-test.c
@@ -61,6 +61,15 @@ static void test_init(void)
     qusb_pci_init_one(pcibus, &ehci1, QPCI_DEVFN(0x1d, 7), 0);
 }
 
+static void test_deinit(void)
+{
+    uhci_deinit(&uhci1);
+    uhci_deinit(&uhci2);
+    uhci_deinit(&uhci3);
+    uhci_deinit(&ehci1);
+    qpci_free_pc(pcibus);
+}
+
 static void pci_uhci_port_1(void)
 {
     g_assert(pcibus != NULL);
@@ -161,6 +170,7 @@ int main(int argc, char **argv)
 
     test_init();
     ret = g_test_run();
+    test_deinit();
 
     qtest_end();
 
diff --git a/tests/usb-hcd-uhci-test.c b/tests/usb-hcd-uhci-test.c
index e956b9ccb7..f25bae5e6c 100644
--- a/tests/usb-hcd-uhci-test.c
+++ b/tests/usb-hcd-uhci-test.c
@@ -28,6 +28,7 @@ static void test_port(int port)
     g_assert(port > 0);
     qusb_pci_init_one(qs->pcibus, &uhci, QPCI_DEVFN(0x1d, 0), 4);
     uhci_port_test(&uhci, port - 1, UHCI_PORT_CCS);
+    uhci_deinit(&uhci);
 }
 
 static void test_port_1(void)
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 28/30] tests: add specialized device_find function

[Marc-André Lureau]

Allows to specify which slot to look for the device.

This will be used in the following patch to avoid leaking when multiple
devices exists and we want to lookup the hotplug one.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/libqos/virtio-pci.h |  4 ++--
 tests/libqos/virtio-pci.c | 31 ++++++++++++++++++++++++++-----
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/tests/libqos/virtio-pci.h b/tests/libqos/virtio-pci.h
index 0fab916cf8..6ef19094cb 100644
--- a/tests/libqos/virtio-pci.h
+++ b/tests/libqos/virtio-pci.h
@@ -31,9 +31,9 @@ typedef struct QVirtQueuePCI {
 
 extern const QVirtioBus qvirtio_pci;
 
-void qvirtio_pci_foreach(QPCIBus *bus, uint16_t device_type,
-                void (*func)(QVirtioDevice *d, void *data), void *data);
 QVirtioPCIDevice *qvirtio_pci_device_find(QPCIBus *bus, uint16_t device_type);
+QVirtioPCIDevice *qvirtio_pci_device_find_slot(QPCIBus *bus,
+                                               uint16_t device_type, int slot);
 void qvirtio_pci_device_free(QVirtioPCIDevice *dev);
 
 void qvirtio_pci_device_enable(QVirtioPCIDevice *d);
diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
index 456cccdc7b..8a0904bbf0 100644
--- a/tests/libqos/virtio-pci.c
+++ b/tests/libqos/virtio-pci.c
@@ -24,6 +24,8 @@
 typedef struct QVirtioPCIForeachData {
     void (*func)(QVirtioDevice *d, void *data);
     uint16_t device_type;
+    bool has_slot;
+    int slot;
     void *user_data;
 } QVirtioPCIForeachData;
 
@@ -55,10 +57,11 @@ static void qvirtio_pci_foreach_callback(
     QVirtioPCIForeachData *d = data;
     QVirtioPCIDevice *vpcidev = qpcidevice_to_qvirtiodevice(dev);
 
-    if (vpcidev->vdev.device_type == d->device_type) {
+    if (vpcidev->vdev.device_type == d->device_type &&
+        (!d->has_slot || vpcidev->pdev->devfn == d->slot << 3)) {
         d->func(&vpcidev->vdev, d->user_data);
     } else {
-        g_free(vpcidev);
+        qvirtio_pci_device_free(vpcidev);
     }
 }
 
@@ -290,21 +293,39 @@ const QVirtioBus qvirtio_pci = {
     .virtqueue_kick = qvirtio_pci_virtqueue_kick,
 };
 
-void qvirtio_pci_foreach(QPCIBus *bus, uint16_t device_type,
+static void qvirtio_pci_foreach(QPCIBus *bus, uint16_t device_type,
+                bool has_slot, int slot,
                 void (*func)(QVirtioDevice *d, void *data), void *data)
 {
     QVirtioPCIForeachData d = { .func = func,
                                 .device_type = device_type,
+                                .has_slot = has_slot,
+                                .slot = slot,
                                 .user_data = data };
 
     qpci_device_foreach(bus, PCI_VENDOR_ID_REDHAT_QUMRANET, -1,
-                                qvirtio_pci_foreach_callback, &d);
+                        qvirtio_pci_foreach_callback, &d);
 }
 
 QVirtioPCIDevice *qvirtio_pci_device_find(QPCIBus *bus, uint16_t device_type)
 {
     QVirtioPCIDevice *dev = NULL;
-    qvirtio_pci_foreach(bus, device_type, qvirtio_pci_assign_device, &dev);
+
+    qvirtio_pci_foreach(bus, device_type, false, 0,
+                        qvirtio_pci_assign_device, &dev);
+
+    dev->vdev.bus = &qvirtio_pci;
+
+    return dev;
+}
+
+QVirtioPCIDevice *qvirtio_pci_device_find_slot(QPCIBus *bus,
+                                               uint16_t device_type, int slot)
+{
+    QVirtioPCIDevice *dev = NULL;
+
+    qvirtio_pci_foreach(bus, device_type, true, slot,
+                        qvirtio_pci_assign_device, &dev);
 
     dev->vdev.bus = &qvirtio_pci;
 
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 29/30] tests: fix virtio-blk-test leaks

[Marc-André Lureau]

Use qvirtio_pci_device_find_slot() to avoid leaking the non-hp
device. Add assert() to avoid further leaks in the future.

Use qvirtio_pci_device_free() to correctly free QVirtioPCIDevice.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/libqos/virtio-pci.c |  1 +
 tests/virtio-blk-test.c   | 14 +++++++-------
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
index 8a0904bbf0..7ac15c04e1 100644
--- a/tests/libqos/virtio-pci.c
+++ b/tests/libqos/virtio-pci.c
@@ -68,6 +68,7 @@ static void qvirtio_pci_foreach_callback(
 static void qvirtio_pci_assign_device(QVirtioDevice *d, void *data)
 {
     QVirtioPCIDevice **vpcidev = data;
+    assert(!*vpcidev);
     *vpcidev = (QVirtioPCIDevice *)d;
 }
 
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 10a92b410a..1eee95df49 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -108,7 +108,7 @@ static QVirtioPCIDevice *virtio_blk_pci_init(QPCIBus *bus, int slot)
 {
     QVirtioPCIDevice *dev;
 
-    dev = qvirtio_pci_device_find(bus, VIRTIO_ID_BLOCK);
+    dev = qvirtio_pci_device_find_slot(bus, VIRTIO_ID_BLOCK, slot);
     g_assert(dev != NULL);
     g_assert_cmphex(dev->vdev.device_type, ==, VIRTIO_ID_BLOCK);
     g_assert_cmphex(dev->pdev->devfn, ==, ((slot << 3) | PCI_FN));
@@ -296,7 +296,7 @@ static void pci_basic(void)
     /* End test */
     qvirtqueue_cleanup(dev->vdev.bus, &vqpci->vq, qs->alloc);
     qvirtio_pci_device_disable(dev);
-    g_free(dev);
+    qvirtio_pci_device_free(dev);
     qtest_shutdown(qs);
 }
 
@@ -389,7 +389,7 @@ static void pci_indirect(void)
     /* End test */
     qvirtqueue_cleanup(dev->vdev.bus, &vqpci->vq, qs->alloc);
     qvirtio_pci_device_disable(dev);
-    g_free(dev);
+    qvirtio_pci_device_free(dev);
     qtest_shutdown(qs);
 }
 
@@ -418,7 +418,7 @@ static void pci_config(void)
     g_assert_cmpint(capacity, ==, n_size / 512);
 
     qvirtio_pci_device_disable(dev);
-    g_free(dev);
+    qvirtio_pci_device_free(dev);
 
     qtest_shutdown(qs);
 }
@@ -526,7 +526,7 @@ static void pci_msix(void)
     qvirtqueue_cleanup(dev->vdev.bus, &vqpci->vq, qs->alloc);
     qpci_msix_disable(dev->pdev);
     qvirtio_pci_device_disable(dev);
-    g_free(dev);
+    qvirtio_pci_device_free(dev);
     qtest_shutdown(qs);
 }
 
@@ -642,7 +642,7 @@ static void pci_idx(void)
     qvirtqueue_cleanup(dev->vdev.bus, &vqpci->vq, qs->alloc);
     qpci_msix_disable(dev->pdev);
     qvirtio_pci_device_disable(dev);
-    g_free(dev);
+    qvirtio_pci_device_free(dev);
     qtest_shutdown(qs);
 }
 
@@ -661,7 +661,7 @@ static void pci_hotplug(void)
     dev = virtio_blk_pci_init(qs->pcibus, PCI_SLOT_HP);
     g_assert(dev);
     qvirtio_pci_device_disable(dev);
-    g_free(dev);
+    qvirtio_pci_device_free(dev);
 
     /* unplug secondary disk */
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
-- 
2.11.0.295.gd7dffce1c.dirty

[Qemu-devel] [PATCH v2 30/30] migration: fix id leak regression

[Marc-André Lureau]

This leak was introduced in commit
581f08bac22bdd5e081ae07f68071a0fc3c5c2c7.

(it stands out quickly with ASAN once the rest of the leaks are also
removed from make check with this series)

Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 migration/savevm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index 5ecd264134..87c7a00832 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -688,6 +688,7 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
 
                 return -1;
             }
+            g_free(id);
 
             se->compat = g_new0(CompatEntry, 1);
             pstrcpy(se->compat->idstr, sizeof(se->compat->idstr), vmsd->name);
-- 
2.11.0.295.gd7dffce1c.dirty

Re: [Qemu-devel] [PATCH v2 30/30] migration: fix id leak regression

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
> This leak was introduced in commit
> 581f08bac22bdd5e081ae07f68071a0fc3c5c2c7.
> 
> (it stands out quickly with ASAN once the rest of the leaks are also
> removed from make check with this series)

Oops yes.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> 
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  migration/savevm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 5ecd264134..87c7a00832 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -688,6 +688,7 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
>  
>                  return -1;
>              }
> +            g_free(id);
>  
>              se->compat = g_new0(CompatEntry, 1);
>              pstrcpy(se->compat->idstr, sizeof(se->compat->idstr), vmsd->name);
> -- 
> 2.11.0.295.gd7dffce1c.dirty
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 16/30] tests: fix eepro100-test leak

[Stefan Weil]

Am 21.02.2017 um 15:14 schrieb Marc-André Lureau:
> Spotted by ASAN.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/eepro100-test.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/eepro100-test.c b/tests/eepro100-test.c
> index ed23258b0f..bdc8a67d57 100644
> --- a/tests/eepro100-test.c
> +++ b/tests/eepro100-test.c
> @@ -54,6 +54,7 @@ int main(int argc, char **argv)
>  
>          path = g_strdup_printf("eepro100/%s", models[i]);
>          qtest_add_data_func(path, models[i], test_device);
> +        g_free(path);
>      }
>  
>      return g_test_run();
> 

Reviewed-by: Stefan Weil <sw@weilnetz.de>

Re: [Qemu-devel] [PATCH v2 01/30] qtest: fix a memory leak

[Philippe Mathieu-Daudé]

On 02/21/2017 11:14 AM, Marc-André Lureau wrote:
> Spotted by ASAN.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  qtest.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/qtest.c b/qtest.c
> index 1446719e8d..6f4d26832a 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -240,6 +240,7 @@ static void GCC_FMT_ATTR(2, 3) qtest_sendf(CharBackend *chr,
>      va_start(ap, fmt);
>      buffer = g_strdup_vprintf(fmt, ap);
>      qtest_send(chr, buffer);
> +    g_free(buffer);
>      va_end(ap);
>  }
>
>

Re: [Qemu-devel] [PATCH v2 07/30] tests: fix endianness-test leaks

[Philippe Mathieu-Daudé]

On 02/21/2017 11:14 AM, Marc-André Lureau wrote:
> Spotted by ASAN.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/endianness-test.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/tests/endianness-test.c b/tests/endianness-test.c
> index cf8d41b7b4..ed0bf52019 100644
> --- a/tests/endianness-test.c
> +++ b/tests/endianness-test.c
> @@ -295,14 +295,17 @@ int main(int argc, char **argv)
>          path = g_strdup_printf("endianness/%s",
>                                 test_cases[i].machine);
>          qtest_add_data_func(path, &test_cases[i], test_endianness);
> +        g_free(path);
>
>          path = g_strdup_printf("endianness/split/%s",
>                                 test_cases[i].machine);
>          qtest_add_data_func(path, &test_cases[i], test_endianness_split);
> +        g_free(path);
>
>          path = g_strdup_printf("endianness/combine/%s",
>                                 test_cases[i].machine);
>          qtest_add_data_func(path, &test_cases[i], test_endianness_combine);
> +        g_free(path);
>      }
>
>      return g_test_run();
>

Re: [Qemu-devel] [PATCH v2 13/30] tests: fix ipmi-kcs-test leak

[Philippe Mathieu-Daudé]

On 02/21/2017 11:14 AM, Marc-André Lureau wrote:
> Spotted by ASAN.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/ipmi-kcs-test.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tests/ipmi-kcs-test.c b/tests/ipmi-kcs-test.c
> index 9cf0b34a33..178ffc1797 100644
> --- a/tests/ipmi-kcs-test.c
> +++ b/tests/ipmi-kcs-test.c
> @@ -279,6 +279,7 @@ int main(int argc, char **argv)
>      cmdline = g_strdup_printf("-device ipmi-bmc-sim,id=bmc0"
>                                " -device isa-ipmi-kcs,bmc=bmc0");
>      qtest_start(cmdline);
> +    g_free(cmdline);
>      qtest_irq_intercept_in(global_qtest, "ioapic");
>      qtest_add_func("/ipmi/local/kcs_base", test_kcs_base);
>      qtest_add_func("/ipmi/local/kcs_abort", test_kcs_abort);
>

Re: [Qemu-devel] [PATCH v2 14/30] tests: fix ipmi-bt-test leak

[Philippe Mathieu-Daudé]

On 02/21/2017 11:14 AM, Marc-André Lureau wrote:
> Spotted by ASAN.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/ipmi-bt-test.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tests/ipmi-bt-test.c b/tests/ipmi-bt-test.c
> index e84dd6889b..7e21a9bbcb 100644
> --- a/tests/ipmi-bt-test.c
> +++ b/tests/ipmi-bt-test.c
> @@ -420,6 +420,7 @@ int main(int argc, char **argv)
>            " -device ipmi-bmc-extern,chardev=ipmi0,id=bmc0"
>            " -device isa-ipmi-bt,bmc=bmc0", emu_port);
>      qtest_start(cmdline);
> +    g_free(cmdline);
>      qtest_irq_intercept_in(global_qtest, "ioapic");
>      qtest_add_func("/ipmi/extern/connect", test_connect);
>      qtest_add_func("/ipmi/extern/bt_base", test_bt_base);
>

Re: [Qemu-devel] [PATCH v2 18/30] tests: fix e1000-test leak

[Philippe Mathieu-Daudé]

On 02/21/2017 11:14 AM, Marc-André Lureau wrote:
> Spotted by ASAN.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/e1000-test.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tests/e1000-test.c b/tests/e1000-test.c
> index 59cab68a60..0c5fcdcc44 100644
> --- a/tests/e1000-test.c
> +++ b/tests/e1000-test.c
> @@ -44,6 +44,7 @@ int main(int argc, char **argv)
>
>          path = g_strdup_printf("e1000/%s", models[i]);
>          qtest_add_data_func(path, models[i], test_device);
> +        g_free(path);
>      }
>
>      return g_test_run();
>

Re: [Qemu-devel] [PATCH v2 26/30] tests: allows to run single test in usb-hcd-ehci-test

[Philippe Mathieu-Daudé]

On 02/21/2017 11:14 AM, Marc-André Lureau wrote:
> pci_init() shouldn't be a test function, but instead called before any
> test. This allows to run a single test with -p /x86_64/ehci/....

good point.

>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/usb-hcd-ehci-test.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/tests/usb-hcd-ehci-test.c b/tests/usb-hcd-ehci-test.c
> index 57af8a034e..fba84b4bee 100644
> --- a/tests/usb-hcd-ehci-test.c
> +++ b/tests/usb-hcd-ehci-test.c
> @@ -50,11 +50,8 @@ static void ehci_port_test(struct qhc *hc, int port, uint32_t expect)
>
>  /* tests */
>
> -static void pci_init(void)
> +static void test_init(void)
>  {
> -    if (pcibus) {
> -        return;
> -    }
>      pcibus = qpci_init_pc(NULL);
>      g_assert(pcibus != NULL);
>
> @@ -142,7 +139,7 @@ int main(int argc, char **argv)
>      int ret;
>
>      g_test_init(&argc, &argv, NULL);
> -    qtest_add_func("/ehci/pci/init", pci_init);
> +
>      qtest_add_func("/ehci/pci/uhci-port-1", pci_uhci_port_1);
>      qtest_add_func("/ehci/pci/ehci-port-1", pci_ehci_port_1);
>      qtest_add_func("/ehci/pci/ehci-config", pci_ehci_config);
> @@ -161,6 +158,8 @@ int main(int argc, char **argv)
>                  "-drive if=none,id=usbcdrom,media=cdrom "
>                  "-device usb-tablet,bus=ich9-ehci-1.0,port=1,usb_version=1 "
>                  "-device usb-storage,bus=ich9-ehci-1.0,port=2,drive=usbcdrom ");
> +
> +    test_init();
>      ret = g_test_run();
>
>      qtest_end();
>

Re: [Qemu-devel] [PATCH v2 30/30] migration: fix id leak regression

[Philippe Mathieu-Daudé]

Hi Marc-André,

There is no documentation in "hw/qdev-core.h" for qdev_get_dev_path() 
and his name doesn't suggest any Heap allocation (like many others).
Is there a recommended way in Qemu codebase to give to the user/caller a 
hit to care about releasing memory? "Learn to Read the Source, Luke."

On 02/21/2017 11:19 AM, Dr. David Alan Gilbert wrote:
> * Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
>> This leak was introduced in commit
>> 581f08bac22bdd5e081ae07f68071a0fc3c5c2c7.
>>
>> (it stands out quickly with ASAN once the rest of the leaks are also
>> removed from make check with this series)
>
> Oops yes.
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>
>>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Cc: Juan Quintela <quintela@redhat.com>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  migration/savevm.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 5ecd264134..87c7a00832 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -688,6 +688,7 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
>>
>>                  return -1;
>>              }
>> +            g_free(id);
>>
>>              se->compat = g_new0(CompatEntry, 1);
>>              pstrcpy(se->compat->idstr, sizeof(se->compat->idstr), vmsd->name);
>> --
>> 2.11.0.295.gd7dffce1c.dirty
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>

Re: [Qemu-devel] [PATCH v2 05/30] glib-compat: add g_test_add_data_func_full fallback

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2199 bytes --]

On 02/21/2017 08:14 AM, Marc-André Lureau wrote:
> Move the fallback from qtest_add_data_func_full() to glib-compat.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/glib-compat.h | 21 +++++++++++++++++++++
>  tests/libqtest.c      | 10 ----------
>  2 files changed, 21 insertions(+), 10 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/include/glib-compat.h b/include/glib-compat.h
> index 0cd24ffbe9..863c8cf73d 100644
> --- a/include/glib-compat.h
> +++ b/include/glib-compat.h
> @@ -328,4 +328,25 @@ static inline void g_source_set_name_by_id(guint tag, const char *name)
>  #define g_test_subprocess() (0)
>  #endif
>  
> +
> +#if !GLIB_CHECK_VERSION(2, 34, 0)
> +static inline void
> +g_test_add_data_func_full(const char *path,
> +                          gpointer data,
> +                          gpointer fn,
> +                          gpointer data_free_func)
> +{
> +#if GLIB_CHECK_VERSION(2, 26, 0)
> +    /* back-compat casts, remove this once we can require new-enough glib */
> +    g_test_add_vtable(path, 0, data, NULL,
> +                      (GTestFixtureFunc)fn, (GTestFixtureFunc) data_free_func);
> +#else
> +    /* back-compat casts, remove this once we can require new-enough glib */

The comments are somewhat dead here, since they are guarded by
GLIB_CHECK_VERSION that states what the new-enough glib is (and the
older versions won't be changing).

>      gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
> -#if GLIB_CHECK_VERSION(2, 34, 0)
>      g_test_add_data_func_full(path, data, fn, data_free_func);
> -#elif GLIB_CHECK_VERSION(2, 26, 0)
> -    /* back-compat casts, remove this once we can require new-enough glib */
> -    g_test_add_vtable(path, 0, data, NULL,
> -                      (GTestFixtureFunc)fn, (GTestFixtureFunc) data_free_func);
> -#else
> -    /* back-compat casts, remove this once we can require new-enough glib */

But since it is code motion, I don't care if you keep or remove the
comments.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 25/30] usb: release the created buses

[Gerd Hoffmann]

On Di, 2017-02-21 at 18:14 +0400, Marc-André Lureau wrote:
> Leaks spotted by ASAN.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

Re: [Qemu-devel] [PATCH v2 27/30] tests: fix usb-test leaks

[Gerd Hoffmann]

On Di, 2017-02-21 at 18:14 +0400, Marc-André Lureau wrote:
> Fix the usb tests leaks.
> 
> Spotted by ASAN.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

Re: [Qemu-devel] [PATCH v2 26/30] tests: allows to run single test in usb-hcd-ehci-test

[Gerd Hoffmann]

On Di, 2017-02-21 at 18:14 +0400, Marc-André Lureau wrote:
> pci_init() shouldn't be a test function, but instead called before any
> test. This allows to run a single test with -p /x86_64/ehci/....

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

/me assumes you'll go send a pull request for the whole series?
Or do you want me cherry-pick the usb patches into the usb queue?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 26/30] tests: allows to run single test in usb-hcd-ehci-test

[Marc-André Lureau]

Hi

On Wed, Feb 22, 2017 at 3:05 AM Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Di, 2017-02-21 at 18:14 +0400, Marc-André Lureau wrote:
> > pci_init() shouldn't be a test function, but instead called before any
> > test. This allows to run a single test with -p /x86_64/ehci/....
>
> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
>
> /me assumes you'll go send a pull request for the whole series?
> Or do you want me cherry-pick the usb patches into the usb queue?
>

Since the whole series is pending for reviews, feel free to pick the usb
patches, I'll rebase regularly.

thanks
-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 30/30] migration: fix id leak regression

[Marc-André Lureau]

Hi

On Tue, Feb 21, 2017 at 9:43 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> Hi Marc-André,
>
> There is no documentation in "hw/qdev-core.h" for qdev_get_dev_path()
> and his name doesn't suggest any Heap allocation (like many others).
> Is there a recommended way in Qemu codebase to give to the user/caller a
> hit to care about releasing memory? "Learn to Read the Source, Luke."
>

Not much hint, except that the returned value is not const. But I did
verify this when I wrote the patch.

thanks


> On 02/21/2017 11:19 AM, Dr. David Alan Gilbert wrote:
> > * Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
> >> This leak was introduced in commit
> >> 581f08bac22bdd5e081ae07f68071a0fc3c5c2c7.
> >>
> >> (it stands out quickly with ASAN once the rest of the leaks are also
> >> removed from make check with this series)
> >
> > Oops yes.
> >
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> >
> >>
> >> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >> Cc: Juan Quintela <quintela@redhat.com>
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> ---
> >>  migration/savevm.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/migration/savevm.c b/migration/savevm.c
> >> index 5ecd264134..87c7a00832 100644
> >> --- a/migration/savevm.c
> >> +++ b/migration/savevm.c
> >> @@ -688,6 +688,7 @@ int vmstate_register_with_alias_id(DeviceState
> *dev, int instance_id,
> >>
> >>                  return -1;
> >>              }
> >> +            g_free(id);
> >>
> >>              se->compat = g_new0(CompatEntry, 1);
> >>              pstrcpy(se->compat->idstr, sizeof(se->compat->idstr),
> vmsd->name);
> >> --
> >> 2.11.0.295.gd7dffce1c.dirty
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
>
> --
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 23/30] bus: do not unref hotplug handler

[Paolo Bonzini]

On 21/02/2017 15:14, Marc-André Lureau wrote:
> Apparently, none of the bus owner give a reference to the hotplug
> handler property, do not unref it on bus release.
> 
> Furthermore, a bus is allowed to be its own hotplug handler, which can
> be seen in qbus_set_bus_hotplug_handler() function. However, in this
> case, the reference can't be given to the property, or this will create
> a cyclic dependency and the bus will never be free.
> 
> Each bus owner should manage the lifecycle of the hotplug handler.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Almost all qbus_set_hotplug_handler callers are using it to set the
parent device (that is, the "adapter" or "bridge", whatever you want to
call it) as the hotplug handler.

The exception is piix4_update_bus_hotplug.  This one is the only case
where OBJ_PROP_LINK_UNREF_ON_RELEASE would be right.  Luckily, there
_is_ a reference to that device somewhere else to keep it alive, namely
in /machine's acpi-device prop.  So this case is a bit hacky (not your
fault) but works as well.  In addition the PIIX4_PM device is not
hot(un)pluggable.

Can you please add a comment to piix4_update_bus_hotplug explaining that
the PIIX4PMState cannot outlive the PCIBus, because /machine keeps it alive?

> 
> diff --git a/hw/core/bus.c b/hw/core/bus.c
> index cf383fc1af..4651f24486 100644
> --- a/hw/core/bus.c
> +++ b/hw/core/bus.c
> @@ -197,7 +197,7 @@ static void qbus_initfn(Object *obj)
>                               TYPE_HOTPLUG_HANDLER,
>                               (Object **)&bus->hotplug_handler,
>                               object_property_allow_set_link,
> -                             OBJ_PROP_LINK_UNREF_ON_RELEASE,
> +                             0,
>                               NULL);
>      object_property_add_bool(obj, "realized",
>                               bus_get_realized, bus_set_realized, NULL);
> -- 
> 2.11.0.295.gd7dffce1c.dirty
> 
> 
> 

Re: [Qemu-devel] [PATCH v2 21/30] tests: fix virtio-scsi-test leak

[Paolo Bonzini]

On 21/02/2017 15:14, Marc-André Lureau wrote:
> Spotted by ASAN.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> ---
>  tests/virtio-scsi-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
> index 69220ef07b..0eabd56fd9 100644
> --- a/tests/virtio-scsi-test.c
> +++ b/tests/virtio-scsi-test.c
> @@ -63,7 +63,7 @@ static void qvirtio_scsi_pci_free(QVirtIOSCSI *vs)
>          qvirtqueue_cleanup(vs->dev->bus, vs->vq[i], vs->qs->alloc);
>      }
>      qvirtio_pci_device_disable(container_of(vs->dev, QVirtioPCIDevice, vdev));
> -    g_free(vs->dev);
> +    qvirtio_pci_device_free((QVirtioPCIDevice *)vs->dev);
>      qvirtio_scsi_stop(vs->qs);
>      g_free(vs);
>  }
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v2 06/30] tests: fix ptimer leaks

[Paolo Bonzini]

On 21/02/2017 15:14, Marc-André Lureau wrote:
> Spotted by ASAN.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/hw/ptimer.h       |   1 +
>  hw/core/ptimer.c          |   8 +++
>  tests/ptimer-test-stubs.c |   5 ++
>  tests/ptimer-test.c       | 122 ++++++++++++++++++++++++++++------------------
>  4 files changed, 89 insertions(+), 47 deletions(-)
> 
> diff --git a/include/hw/ptimer.h b/include/hw/ptimer.h
> index 48cccbdb51..eafc3f0a86 100644
> --- a/include/hw/ptimer.h
> +++ b/include/hw/ptimer.h
> @@ -60,6 +60,7 @@ typedef struct ptimer_state ptimer_state;
>  typedef void (*ptimer_cb)(void *opaque);
>  
>  ptimer_state *ptimer_init(QEMUBH *bh, uint8_t policy_mask);
> +void ptimer_free(ptimer_state *s);
>  void ptimer_set_period(ptimer_state *s, int64_t period);
>  void ptimer_set_freq(ptimer_state *s, uint32_t freq);
>  uint64_t ptimer_get_limit(ptimer_state *s);
> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
> index 3af82afe78..59ccb00550 100644
> --- a/hw/core/ptimer.c
> +++ b/hw/core/ptimer.c
> @@ -12,6 +12,7 @@
>  #include "qemu/host-utils.h"
>  #include "sysemu/replay.h"
>  #include "sysemu/qtest.h"
> +#include "block/aio.h"
>  
>  #define DELTA_ADJUST     1
>  #define DELTA_NO_ADJUST -1
> @@ -353,3 +354,10 @@ ptimer_state *ptimer_init(QEMUBH *bh, uint8_t policy_mask)
>      s->policy_mask = policy_mask;
>      return s;
>  }
> +
> +void ptimer_free(ptimer_state *s)
> +{
> +    qemu_bh_delete(s->bh);
> +    timer_free(s->timer);
> +    g_free(s);
> +}
> diff --git a/tests/ptimer-test-stubs.c b/tests/ptimer-test-stubs.c
> index 21d4ebb0fe..8a1b0a336c 100644
> --- a/tests/ptimer-test-stubs.c
> +++ b/tests/ptimer-test-stubs.c
> @@ -108,6 +108,11 @@ QEMUBH *qemu_bh_new(QEMUBHFunc *cb, void *opaque)
>      return bh;
>  }
>  
> +void qemu_bh_delete(QEMUBH *bh)
> +{
> +    g_free(bh);
> +}
> +
>  void replay_bh_schedule_event(QEMUBH *bh)
>  {
>      bh->cb(bh->opaque);
> diff --git a/tests/ptimer-test.c b/tests/ptimer-test.c
> index b36a476483..5d1a2a8188 100644
> --- a/tests/ptimer-test.c
> +++ b/tests/ptimer-test.c
> @@ -73,6 +73,7 @@ static void check_set_count(gconstpointer arg)
>      ptimer_set_count(ptimer, 1000);
>      g_assert_cmpuint(ptimer_get_count(ptimer), ==, 1000);
>      g_assert_false(triggered);
> +    ptimer_free(ptimer);
>  }
>  
>  static void check_set_limit(gconstpointer arg)
> @@ -92,6 +93,7 @@ static void check_set_limit(gconstpointer arg)
>      g_assert_cmpuint(ptimer_get_count(ptimer), ==, 2000);
>      g_assert_cmpuint(ptimer_get_limit(ptimer), ==, 2000);
>      g_assert_false(triggered);
> +    ptimer_free(ptimer);
>  }
>  
>  static void check_oneshot(gconstpointer arg)
> @@ -194,6 +196,7 @@ static void check_oneshot(gconstpointer arg)
>  
>      g_assert_cmpuint(ptimer_get_count(ptimer), ==, 0);
>      g_assert_false(triggered);
> +    ptimer_free(ptimer);
>  }
>  
>  static void check_periodic(gconstpointer arg)
> @@ -360,6 +363,7 @@ static void check_periodic(gconstpointer arg)
>      g_assert_cmpuint(ptimer_get_count(ptimer), ==,
>                      (no_round_down ? 8 : 7) + (wrap_policy ? 1 : 0));
>      g_assert_false(triggered);
> +    ptimer_free(ptimer);
>  }
>  
>  static void check_on_the_fly_mode_change(gconstpointer arg)
> @@ -406,6 +410,7 @@ static void check_on_the_fly_mode_change(gconstpointer arg)
>  
>      g_assert_cmpuint(ptimer_get_count(ptimer), ==, 0);
>      g_assert_true(triggered);
> +    ptimer_free(ptimer);
>  }
>  
>  static void check_on_the_fly_period_change(gconstpointer arg)
> @@ -438,6 +443,7 @@ static void check_on_the_fly_period_change(gconstpointer arg)
>  
>      g_assert_cmpuint(ptimer_get_count(ptimer), ==, 0);
>      g_assert_true(triggered);
> +    ptimer_free(ptimer);
>  }
>  
>  static void check_on_the_fly_freq_change(gconstpointer arg)
> @@ -470,6 +476,7 @@ static void check_on_the_fly_freq_change(gconstpointer arg)
>  
>      g_assert_cmpuint(ptimer_get_count(ptimer), ==, 0);
>      g_assert_true(triggered);
> +    ptimer_free(ptimer);
>  }
>  
>  static void check_run_with_period_0(gconstpointer arg)
> @@ -487,6 +494,7 @@ static void check_run_with_period_0(gconstpointer arg)
>  
>      g_assert_cmpuint(ptimer_get_count(ptimer), ==, 99);
>      g_assert_false(triggered);
> +    ptimer_free(ptimer);
>  }
>  
>  static void check_run_with_delta_0(gconstpointer arg)
> @@ -591,6 +599,7 @@ static void check_run_with_delta_0(gconstpointer arg)
>      g_assert_true(triggered);
>  
>      ptimer_stop(ptimer);
> +    ptimer_free(ptimer);
>  }
>  
>  static void check_periodic_with_load_0(gconstpointer arg)
> @@ -649,6 +658,7 @@ static void check_periodic_with_load_0(gconstpointer arg)
>      }
>  
>      ptimer_stop(ptimer);
> +    ptimer_free(ptimer);
>  }
>  
>  static void check_oneshot_with_load_0(gconstpointer arg)
> @@ -682,14 +692,14 @@ static void check_oneshot_with_load_0(gconstpointer arg)
>      } else {
>          g_assert_false(triggered);
>      }
> +
> +    ptimer_free(ptimer);
>  }
>  
>  static void add_ptimer_tests(uint8_t policy)
>  {
> -    uint8_t *ppolicy = g_malloc(1);
> -    char *policy_name = g_malloc0(256);
> -
> -    *ppolicy = policy;
> +    char policy_name[256] = "";
> +    char *tmp;
>  
>      if (policy == PTIMER_POLICY_DEFAULT) {
>          g_sprintf(policy_name, "default");
> @@ -715,49 +725,67 @@ static void add_ptimer_tests(uint8_t policy)
>          g_strlcat(policy_name, "no_counter_rounddown,", 256);
>      }
>  
> -    g_test_add_data_func(
> -        g_strdup_printf("/ptimer/set_count policy=%s", policy_name),
> -        ppolicy, check_set_count);
> -
> -    g_test_add_data_func(
> -        g_strdup_printf("/ptimer/set_limit policy=%s", policy_name),
> -        ppolicy, check_set_limit);
> -
> -    g_test_add_data_func(
> -        g_strdup_printf("/ptimer/oneshot policy=%s", policy_name),
> -        ppolicy, check_oneshot);
> -
> -    g_test_add_data_func(
> -        g_strdup_printf("/ptimer/periodic policy=%s", policy_name),
> -        ppolicy, check_periodic);
> -
> -    g_test_add_data_func(
> -        g_strdup_printf("/ptimer/on_the_fly_mode_change policy=%s", policy_name),
> -        ppolicy, check_on_the_fly_mode_change);
> -
> -    g_test_add_data_func(
> -        g_strdup_printf("/ptimer/on_the_fly_period_change policy=%s", policy_name),
> -        ppolicy, check_on_the_fly_period_change);
> -
> -    g_test_add_data_func(
> -        g_strdup_printf("/ptimer/on_the_fly_freq_change policy=%s", policy_name),
> -        ppolicy, check_on_the_fly_freq_change);
> -
> -    g_test_add_data_func(
> -        g_strdup_printf("/ptimer/run_with_period_0 policy=%s", policy_name),
> -        ppolicy, check_run_with_period_0);
> -
> -    g_test_add_data_func(
> -        g_strdup_printf("/ptimer/run_with_delta_0 policy=%s", policy_name),
> -        ppolicy, check_run_with_delta_0);
> -
> -    g_test_add_data_func(
> -        g_strdup_printf("/ptimer/periodic_with_load_0 policy=%s", policy_name),
> -        ppolicy, check_periodic_with_load_0);
> -
> -    g_test_add_data_func(
> -        g_strdup_printf("/ptimer/oneshot_with_load_0 policy=%s", policy_name),
> -        ppolicy, check_oneshot_with_load_0);
> +    g_test_add_data_func_full(
> +        tmp = g_strdup_printf("/ptimer/set_count policy=%s", policy_name),
> +        g_memdup(&policy, 1), check_set_count, g_free);
> +    g_free(tmp);
> +
> +    g_test_add_data_func_full(
> +        tmp = g_strdup_printf("/ptimer/set_limit policy=%s", policy_name),
> +        g_memdup(&policy, 1), check_set_limit, g_free);
> +    g_free(tmp);
> +
> +    g_test_add_data_func_full(
> +        tmp = g_strdup_printf("/ptimer/oneshot policy=%s", policy_name),
> +        g_memdup(&policy, 1), check_oneshot, g_free);
> +    g_free(tmp);
> +
> +    g_test_add_data_func_full(
> +        tmp = g_strdup_printf("/ptimer/periodic policy=%s", policy_name),
> +        g_memdup(&policy, 1), check_periodic, g_free);
> +    g_free(tmp);
> +
> +    g_test_add_data_func_full(
> +        tmp = g_strdup_printf("/ptimer/on_the_fly_mode_change policy=%s",
> +                              policy_name),
> +        g_memdup(&policy, 1), check_on_the_fly_mode_change, g_free);
> +    g_free(tmp);
> +
> +    g_test_add_data_func_full(
> +        tmp = g_strdup_printf("/ptimer/on_the_fly_period_change policy=%s",
> +                              policy_name),
> +        g_memdup(&policy, 1), check_on_the_fly_period_change, g_free);
> +    g_free(tmp);
> +
> +    g_test_add_data_func_full(
> +        tmp = g_strdup_printf("/ptimer/on_the_fly_freq_change policy=%s",
> +                              policy_name),
> +        g_memdup(&policy, 1), check_on_the_fly_freq_change, g_free);
> +    g_free(tmp);
> +
> +    g_test_add_data_func_full(
> +        tmp = g_strdup_printf("/ptimer/run_with_period_0 policy=%s",
> +                              policy_name),
> +        g_memdup(&policy, 1), check_run_with_period_0, g_free);
> +    g_free(tmp);
> +
> +    g_test_add_data_func_full(
> +        tmp = g_strdup_printf("/ptimer/run_with_delta_0 policy=%s",
> +                              policy_name),
> +        g_memdup(&policy, 1), check_run_with_delta_0, g_free);
> +    g_free(tmp);
> +
> +    g_test_add_data_func_full(
> +        tmp = g_strdup_printf("/ptimer/periodic_with_load_0 policy=%s",
> +                              policy_name),
> +        g_memdup(&policy, 1), check_periodic_with_load_0, g_free);
> +    g_free(tmp);
> +
> +    g_test_add_data_func_full(
> +        tmp = g_strdup_printf("/ptimer/oneshot_with_load_0 policy=%s",
> +                              policy_name),
> +        g_memdup(&policy, 1), check_oneshot_with_load_0, g_free);
> +    g_free(tmp);
>  }
>  
>  static void add_all_ptimer_policies_comb_tests(void)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v2 04/30] timer: use an inline function for free

[Paolo Bonzini]

On 21/02/2017 15:14, Marc-André Lureau wrote:
> Similarly to allocation, do it from an inline function. This allows
> tests to only use the headers for allocation/free of timer.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> ---
>  include/qemu/timer.h | 5 ++++-
>  qemu-timer.c         | 5 -----
>  2 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> index 9abed51ae8..26e628584c 100644
> --- a/include/qemu/timer.h
> +++ b/include/qemu/timer.h
> @@ -610,7 +610,10 @@ void timer_deinit(QEMUTimer *ts);
>   *
>   * Free a timer (it must not be on the active list)
>   */
> -void timer_free(QEMUTimer *ts);
> +static inline void timer_free(QEMUTimer *ts)
> +{
> +    g_free(ts);
> +}
>  
>  /**
>   * timer_del:
> diff --git a/qemu-timer.c b/qemu-timer.c
> index ff620ecff7..6cf70b96f6 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -355,11 +355,6 @@ void timer_deinit(QEMUTimer *ts)
>      ts->timer_list = NULL;
>  }
>  
> -void timer_free(QEMUTimer *ts)
> -{
> -    g_free(ts);
> -}
> -
>  static void timer_del_locked(QEMUTimerList *timer_list, QEMUTimer *ts)
>  {
>      QEMUTimer **pt, *t;
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v2 23/30] bus: do not unref hotplug handler

[Marc-André Lureau]

Hi

On Wed, Feb 22, 2017 at 3:39 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

>
>
> On 21/02/2017 15:14, Marc-André Lureau wrote:
> > Apparently, none of the bus owner give a reference to the hotplug
> > handler property, do not unref it on bus release.
> >
> > Furthermore, a bus is allowed to be its own hotplug handler, which can
> > be seen in qbus_set_bus_hotplug_handler() function. However, in this
> > case, the reference can't be given to the property, or this will create
> > a cyclic dependency and the bus will never be free.
> >
> > Each bus owner should manage the lifecycle of the hotplug handler.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Almost all qbus_set_hotplug_handler callers are using it to set the
> parent device (that is, the "adapter" or "bridge", whatever you want to
> call it) as the hotplug handler.
>
> The exception is piix4_update_bus_hotplug.  This one is the only case
> where OBJ_PROP_LINK_UNREF_ON_RELEASE would be right.  Luckily, there
> _is_ a reference to that device somewhere else to keep it alive, namely
> in /machine's acpi-device prop.  So this case is a bit hacky (not your
> fault) but works as well.  In addition the PIIX4_PM device is not
> hot(un)pluggable.
>

I understand you agree with my change, correct?

>
> Can you please add a comment to piix4_update_bus_hotplug explaining that
> the PIIX4PMState cannot outlive the PCIBus, because /machine keeps it
> alive?
>

 Do you mean PCIBus cannot outlive PIIX4PMState? (ie the PIIX4PMState will
always be there since it's linked from /machine)

thanks

>
> >
> > diff --git a/hw/core/bus.c b/hw/core/bus.c
> > index cf383fc1af..4651f24486 100644
> > --- a/hw/core/bus.c
> > +++ b/hw/core/bus.c
> > @@ -197,7 +197,7 @@ static void qbus_initfn(Object *obj)
> >                               TYPE_HOTPLUG_HANDLER,
> >                               (Object **)&bus->hotplug_handler,
> >                               object_property_allow_set_link,
> > -                             OBJ_PROP_LINK_UNREF_ON_RELEASE,
> > +                             0,
> >                               NULL);
> >      object_property_add_bool(obj, "realized",
> >                               bus_get_realized, bus_set_realized, NULL);
> > --
> > 2.11.0.295.gd7dffce1c.dirty
> >
> >
> >
>
> --
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 23/30] bus: do not unref hotplug handler

[Paolo Bonzini]

On 22/02/2017 14:03, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Feb 22, 2017 at 3:39 PM Paolo Bonzini <pbonzini@redhat.com
> <mailto:pbonzini@redhat.com>> wrote:
> 
> 
> 
>     On 21/02/2017 15:14, Marc-André Lureau wrote:
>     > Apparently, none of the bus owner give a reference to the hotplug
>     > handler property, do not unref it on bus release.
>     >
>     > Furthermore, a bus is allowed to be its own hotplug handler, which can
>     > be seen in qbus_set_bus_hotplug_handler() function. However, in this
>     > case, the reference can't be given to the property, or this will
>     create
>     > a cyclic dependency and the bus will never be free.
>     >
>     > Each bus owner should manage the lifecycle of the hotplug handler.
>     >
>     > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com
>     <mailto:marcandre.lureau@redhat.com>>
> 
>     Almost all qbus_set_hotplug_handler callers are using it to set the
>     parent device (that is, the "adapter" or "bridge", whatever you want to
>     call it) as the hotplug handler.
> 
>     The exception is piix4_update_bus_hotplug.  This one is the only case
>     where OBJ_PROP_LINK_UNREF_ON_RELEASE would be right.  Luckily, there
>     _is_ a reference to that device somewhere else to keep it alive, namely
>     in /machine's acpi-device prop.  So this case is a bit hacky (not your
>     fault) but works as well.  In addition the PIIX4_PM device is not
>     hot(un)pluggable.
> 
> I understand you agree with my change, correct?

Yes.

>     Can you please add a comment to piix4_update_bus_hotplug explaining that
>     the PIIX4PMState cannot outlive the PCIBus, because /machine keeps
>     it alive?
> 
> 
>  Do you mean PCIBus cannot outlive PIIX4PMState? (ie the PIIX4PMState
> will always be there since it's linked from /machine)

Yes (or just because it's not unpluggable).

Paolo

Re: [Qemu-devel] [PATCH v2 00/30] Various memory leak fixes

[Marc-André Lureau]

Hi

On Tue, Feb 21, 2017 at 6:27 PM Marc-André Lureau <
marcandre.lureau@redhat.com> wrote:

> After this series removing a few memory leaks, make check with ASAN
> enabled gives a "clean" run with x86_64-softmmu target (there are
> warnings related to coroutine makecontext/swapcontext).
>
> This should help spot memory related regressions when introducing one.
>
> Note: some of these patches were sent and reviewed previously but not
> yet applied, I rebased them and included them here for completeness.
>

The series is missing reviews of the following patches: 8, 9, 11, 12, 17,
19, 20, 28, 29

Any volunteer? Thanks!


>
> v2:
> - add r-b tags
> - fix gtest compatibility with old glib
> - add "migration: fix id leak regression" patch
>
> Igor Mammedov (1):
>   pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice
>
> Marc-André Lureau (29):
>   qtest: fix a memory leak
>   tests: fix qmp response leak
>   tests: fix leaks in test-io-channel-command
>   timer: use an inline function for free
>   glib-compat: add g_test_add_data_func_full fallback
>   tests: fix ptimer leaks
>   tests: fix endianness-test leaks
>   tests: fix q35-test leaks
>   tests: fix vhost-user-test leaks
>   tests: fix ide-test leaks
>   tests: fix hd-geo-test leaks
>   tests: fix bios-tables-test leak
>   tests: fix ipmi-kcs-test leak
>   tests: fix ipmi-bt-test leak
>   tests: fix eepro100-test leak
>   tests: fix tco-test leaks
>   tests: fix e1000-test leak
>   tests: fix i440fx-test leaks
>   tests: fix e1000e leaks
>   tests: fix virtio-scsi-test leak
>   tests: fix virtio-9p-test leaks
>   bus: do not unref hotplug handler
>   usb: replace handle_destroy with unrealize
>   usb: release the created buses
>   tests: allows to run single test in usb-hcd-ehci-test
>   tests: fix usb-test leaks
>   tests: add specialized device_find function
>   tests: fix virtio-blk-test leaks
>   migration: fix id leak regression
>
>  include/glib-compat.h           |  21 +++++++
>  include/hw/ptimer.h             |   1 +
>  include/hw/usb.h                |   5 --
>  include/qemu/timer.h            |   5 +-
>  tests/libqos/usb.h              |   1 +
>  tests/libqos/virtio-pci.h       |   6 +-
>  hw/acpi/pcihp.c                 |  11 ----
>  hw/core/bus.c                   |   2 +-
>  hw/core/ptimer.c                |   8 +++
>  hw/i386/acpi-build.c            |   4 +-
>  hw/usb/bus.c                    |   9 +--
>  hw/usb/dev-audio.c              |   4 +-
>  hw/usb/dev-bluetooth.c          |   4 +-
>  hw/usb/dev-hid.c                |   4 +-
>  hw/usb/dev-hub.c                |   4 +-
>  hw/usb/dev-network.c            |   4 +-
>  hw/usb/dev-smartcard-reader.c   |   4 +-
>  hw/usb/dev-storage.c            |  16 ++++++
>  hw/usb/dev-uas.c                |   6 +-
>  hw/usb/dev-wacom.c              |   4 +-
>  hw/usb/host-libusb.c            |   4 +-
>  hw/usb/redirect.c               |   4 +-
>  migration/savevm.c              |   1 +
>  qemu-timer.c                    |   5 --
>  qtest.c                         |   1 +
>  tests/bios-tables-test.c        |   2 +-
>  tests/e1000-test.c              |   1 +
>  tests/e1000e-test.c             |   6 +-
>  tests/eepro100-test.c           |   1 +
>  tests/endianness-test.c         |   3 +
>  tests/hd-geo-test.c             |  47 ++++++++++------
>  tests/i440fx-test.c             |   5 ++
>  tests/ide-test.c                |  12 ++++
>  tests/ipmi-bt-test.c            |   1 +
>  tests/ipmi-kcs-test.c           |   1 +
>  tests/libqos/usb.c              |   6 ++
>  tests/libqos/virtio-pci.c       |  38 +++++++++++--
>  tests/libqtest.c                |  10 ----
>  tests/postcopy-test.c           |   2 +-
>  tests/ptimer-test-stubs.c       |   5 ++
>  tests/ptimer-test.c             | 122
> ++++++++++++++++++++++++----------------
>  tests/pvpanic-test.c            |   1 +
>  tests/q35-test.c                |   3 +
>  tests/tco-test.c                |  35 +++++++-----
>  tests/test-filter-mirror.c      |   2 +-
>  tests/test-filter-redirector.c  |   4 +-
>  tests/test-io-channel-command.c |   6 +-
>  tests/usb-hcd-ehci-test.c       |  19 +++++--
>  tests/usb-hcd-uhci-test.c       |   1 +
>  tests/vhost-user-test.c         |  11 ++--
>  tests/virtio-9p-test.c          |   2 +-
>  tests/virtio-blk-test.c         |  29 +++++-----
>  tests/virtio-scsi-test.c        |   2 +-
>  53 files changed, 338 insertions(+), 177 deletions(-)
>
> --
> 2.11.0.295.gd7dffce1c.dirty
>
>
> --
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 11/30] tests: fix hd-geo-test leaks

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Spotted by ASAN.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/hd-geo-test.c | 47 +++++++++++++++++++++++++++++++----------------
>  1 file changed, 31 insertions(+), 16 deletions(-)
>
> diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
> index 6176e81ab2..88f8d76d32 100644
> --- a/tests/hd-geo-test.c
> +++ b/tests/hd-geo-test.c
> @@ -19,6 +19,8 @@
>  #include "qemu-common.h"
>  #include "libqtest.h"
>  
> +#define ARGV_SIZE 256
> +
>  static char *create_test_img(int secs)
>  {
>      char *template = strdup("/tmp/qtest.XXXXXX");
> @@ -66,7 +68,7 @@ static const CHST hd_chst[backend_last][mbr_last] = {
>      },
>  };
>  
> -static const char *img_file_name[backend_last];
> +static char *img_file_name[backend_last];
>  
>  static const CHST *cur_ide[4];
>  
> @@ -234,28 +236,34 @@ static int setup_ide(int argc, char *argv[], int argv_sz,
>   */
>  static void test_ide_none(void)
>  {
> -    char *argv[256];
> +    char **argv = g_new0(char *, ARGV_SIZE);
> +    char *tmp;
>  
> -    setup_common(argv, ARRAY_SIZE(argv));
> -    qtest_start(g_strjoinv(" ", argv));
> +    setup_common(argv, ARGV_SIZE);
> +    qtest_start(tmp = g_strjoinv(" ", argv));

While I'm totally fine with things like while ((tmp = ...)) when the
result is more concise, I don't like this.  Please do

       tmp = g_strjoinv(" ", argv);
       qtest_start(tmp);

Also, please rename tmp to extra_args, argstr, or maybe args.

> +    g_strfreev(argv);
> +    g_free(tmp);
>      test_cmos();
>      qtest_end();
>  }

I think we should have a qtest_startv() that takes an argv[] and doesn't
use the shell to split arguments.  Outside this patch's scope.

[More of the same snipped...]

With the stylistic changes outlined above:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 12/30] tests: fix bios-tables-test leak

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> The inside array should be free too.
> Spotted by ASAN.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/bios-tables-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 54048050c0..d54018da73 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -144,7 +144,7 @@ static void free_test_data(test_data *data)
>          g_free(temp->asl_file);
>      }
>  
> -    g_array_free(data->tables, false);
> +    g_array_free(data->tables, true);
>  }
>  
>  static uint8_t acpi_checksum(const uint8_t *data, int len)

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 19/30] tests: fix i440fx-test leaks

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Spotted by ASAN.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/i440fx-test.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c
> index da2d5a53f0..e9d05c87d1 100644
> --- a/tests/i440fx-test.c
> +++ b/tests/i440fx-test.c
> @@ -134,6 +134,8 @@ static void test_i440fx_defaults(gconstpointer opaque)
>      /* 3.2.26 */
>      g_assert_cmpint(qpci_config_readb(dev, 0x93), ==, 0x00); /* TRC */
>  
> +    g_free(dev);
> +    qpci_free_pc(bus);
>      qtest_end();
>  }
>  
> @@ -270,6 +272,9 @@ static void test_i440fx_pam(gconstpointer opaque)
>          /* Verify the area is not our new mask */
>          g_assert(!verify_area(pam_area[i].start, pam_area[i].end, 0x82));
>      }
> +
> +    g_free(dev);
> +    qpci_free_pc(bus);
>      qtest_end();
>  }

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 20/30] tests: fix e1000e leaks

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Spotted by ASAN.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/e1000e-test.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/tests/e1000e-test.c b/tests/e1000e-test.c
> index 8c42ca919f..c612dc64ec 100644
> --- a/tests/e1000e-test.c
> +++ b/tests/e1000e-test.c
> @@ -99,7 +99,10 @@ static QPCIBus *test_bus;
>  
>  static void e1000e_pci_foreach_callback(QPCIDevice *dev, int devfn, void *data)
>  {
> -    *(QPCIDevice **) data = dev;
> +    QPCIDevice **res = data;
> +
> +    g_assert_null(*res);
> +    *res = dev;
>  }
>  
>  static QPCIDevice *e1000e_device_find(QPCIBus *bus)
   {
       static const int e1000e_vendor_id = 0x8086;
       static const int e1000e_dev_id = 0x10D3;

       QPCIDevice *e1000e_dev = NULL;

       qpci_device_foreach(bus, e1000e_vendor_id, e1000e_dev_id,
           e1000e_pci_foreach_callback, &e1000e_dev);

This hunk adds an assertion.  I guess it checks that we're finding no
more than one e1000e device.  The connection to the stated purpose of
this patch isn't obvious, but I think I got it: each hit allocates, but
you have only one g_free(), below.  Okay, but explaining this briefly
the commit message wouldn't hurt.

> @@ -403,6 +406,7 @@ static void data_test_clear(e1000e_device *d)
>      e1000e_device_clear(test_bus, d);
>      close(test_sockets[0]);
>      pc_alloc_uninit(test_alloc);
> +    g_free(d->pci_dev);
>      qpci_free_pc(test_bus);
>      qtest_end();
>  }

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 30/30] migration: fix id leak regression

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
> This leak was introduced in commit
> 581f08bac22bdd5e081ae07f68071a0fc3c5c2c7.
> 
> (it stands out quickly with ASAN once the rest of the leaks are also
> removed from make check with this series)
> 
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Queued just this one.

> ---
>  migration/savevm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 5ecd264134..87c7a00832 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -688,6 +688,7 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
>  
>                  return -1;
>              }
> +            g_free(id);
>  
>              se->compat = g_new0(CompatEntry, 1);
>              pstrcpy(se->compat->idstr, sizeof(se->compat->idstr), vmsd->name);
> -- 
> 2.11.0.295.gd7dffce1c.dirty
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 08/30] tests: fix q35-test leaks

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]

On 02/21/2017 08:14 AM, Marc-André Lureau wrote:
> Spotted by ASAN.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/q35-test.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/q35-test.c b/tests/q35-test.c
> index 763fe3d6ae..e010d781e7 100644
> --- a/tests/q35-test.c
> +++ b/tests/q35-test.c
> @@ -71,6 +71,9 @@ static void test_smram_lock(void)
>      g_assert(smram_test_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN) == false);
>      smram_set_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN, true);
>      g_assert(smram_test_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN) == true);
> +
> +    qpci_free_pc(pcibus);
> +    g_free(pcidev);

I might have cleaned pcidev first (reverse order cleanup, since it was
initialized after pcibus), but I don't see any use-after-free problems
in your choice of order.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 09/30] tests: fix vhost-user-test leaks

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 492 bytes --]

On 02/21/2017 08:14 AM, Marc-André Lureau wrote:
> Spotted by ASAN.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/libqos/virtio-pci.h |  2 ++
>  tests/libqos/virtio-pci.c |  6 ++++++
>  tests/vhost-user-test.c   | 11 +++++++----
>  3 files changed, 15 insertions(+), 4 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 17/30] tests: fix tco-test leaks

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 427 bytes --]

On 02/21/2017 08:14 AM, Marc-André Lureau wrote:
> Spotted by ASAN.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/tco-test.c | 35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 28/30] tests: add specialized device_find function

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]

On 02/21/2017 08:14 AM, Marc-André Lureau wrote:
> Allows to specify which slot to look for the device.

"Allow[s] to ${verb}" is not idiomatic; it's missing a subject.  But
"Allows $subject to" (as in "allows someone to" or "allows me to") is
wordy, compared to just saying "Allows ${verb}ing".  I'd suggest:

Allow specifying which slot to favor when looking for the device.

> 
> This will be used in the following patch to avoid leaking when multiple
> devices exists and we want to lookup the hotplug one.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/libqos/virtio-pci.h |  4 ++--
>  tests/libqos/virtio-pci.c | 31 ++++++++++++++++++++++++++-----
>  2 files changed, 28 insertions(+), 7 deletions(-)
> 

> @@ -55,10 +57,11 @@ static void qvirtio_pci_foreach_callback(
>      QVirtioPCIForeachData *d = data;
>      QVirtioPCIDevice *vpcidev = qpcidevice_to_qvirtiodevice(dev);
>  
> -    if (vpcidev->vdev.device_type == d->device_type) {
> +    if (vpcidev->vdev.device_type == d->device_type &&
> +        (!d->has_slot || vpcidev->pdev->devfn == d->slot << 3)) {
>          d->func(&vpcidev->vdev, d->user_data);
>      } else {
> -        g_free(vpcidev);
> +        qvirtio_pci_device_free(vpcidev);

Is this an unmentioned leak plug?  Either it should be mentioned, or
squashed into the leak fix of 29.

With those cleanups,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 29/30] tests: fix virtio-blk-test leaks

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 739 bytes --]

On 02/21/2017 08:14 AM, Marc-André Lureau wrote:
> Use qvirtio_pci_device_find_slot() to avoid leaking the non-hp
> device. Add assert() to avoid further leaks in the future.
> 
> Use qvirtio_pci_device_free() to correctly free QVirtioPCIDevice.

Yep, the hunk I mentioned in 28/30 probably belongs here, alongside the
other instances of this fix.

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/libqos/virtio-pci.c |  1 +
>  tests/virtio-blk-test.c   | 14 +++++++-------
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 00/30] Various memory leak fixes

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

On 02/27/2017 05:04 AM, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Feb 21, 2017 at 6:27 PM Marc-André Lureau <
> marcandre.lureau@redhat.com> wrote:
> 
>> After this series removing a few memory leaks, make check with ASAN
>> enabled gives a "clean" run with x86_64-softmmu target (there are
>> warnings related to coroutine makecontext/swapcontext).
>>
>> This should help spot memory related regressions when introducing one.
>>
>> Note: some of these patches were sent and reviewed previously but not
>> yet applied, I rebased them and included them here for completeness.
>>
> 
> The series is missing reviews of the following patches: 8, 9, 11, 12, 17,
> 19, 20, 28, 29
> 
> Any volunteer? Thanks!

I think we've covered it all now; if not, ping me for another review.

As these are leak fixes, I think they are appropriate even if your pull
request missed soft freeze.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 28/30] tests: add specialized device_find function

[Marc-André Lureau]

On Wed, Mar 1, 2017 at 3:22 AM Eric Blake <eblake@redhat.com> wrote:

> On 02/21/2017 08:14 AM, Marc-André Lureau wrote:
> > Allows to specify which slot to look for the device.
>
> "Allow[s] to ${verb}" is not idiomatic; it's missing a subject.  But
> "Allows $subject to" (as in "allows someone to" or "allows me to") is
> wordy, compared to just saying "Allows ${verb}ing".  I'd suggest:
>
> Allow specifying which slot to favor when looking for the device.
>
> >
> > This will be used in the following patch to avoid leaking when multiple
> > devices exists and we want to lookup the hotplug one.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  tests/libqos/virtio-pci.h |  4 ++--
> >  tests/libqos/virtio-pci.c | 31 ++++++++++++++++++++++++++-----
> >  2 files changed, 28 insertions(+), 7 deletions(-)
> >
>
> > @@ -55,10 +57,11 @@ static void qvirtio_pci_foreach_callback(
> >      QVirtioPCIForeachData *d = data;
> >      QVirtioPCIDevice *vpcidev = qpcidevice_to_qvirtiodevice(dev);
> >
> > -    if (vpcidev->vdev.device_type == d->device_type) {
> > +    if (vpcidev->vdev.device_type == d->device_type &&
> > +        (!d->has_slot || vpcidev->pdev->devfn == d->slot << 3)) {
> >          d->func(&vpcidev->vdev, d->user_data);
> >      } else {
> > -        g_free(vpcidev);
> > +        qvirtio_pci_device_free(vpcidev);
>
> Is this an unmentioned leak plug?  Either it should be mentioned, or
> squashed into the leak fix of 29.
>

I'll move it there, thanks

>
> With those cleanups,
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266 <+1%20919-301-3266>
> Libvirt virtualization library http://libvirt.org
>
> --
Marc-André Lureau