[Qemu-devel] [PULL 00/26] Leak patches

[Qemu-devel] [PULL 00/26] Leak patches

[Marc-André Lureau]

The following changes since commit 1fd66154fdf8305e6668a96046a22b863b4d7320:

  Merge remote-tracking branch 'remotes/sstabellini/tags/xen-20160905' into staging (2016-09-06 11:43:18 +0100)

are available in the git repository at:

  git@github.com:elmarco/qemu.git tags/leak-pull-request

for you to fetch changes up to 58d104a7f2698ffff3ee5976f96ebcff111be63b:

  tests: fix postcopy-test leaks (2016-09-06 16:20:27 +0400)

----------------------------------------------------------------

----------------------------------------------------------------

Marc-André Lureau (26):
  tests: fix test-qga leaks
  glib-compat: add g_(s)list_free_full()
  qga: free the whole blacklist
  qga: free remaining leaking state
  tests: fix test-cutils leaks
  tests: fix test-vmstate leaks
  tests: fix test-iov leaks
  tests: fix check-qom-interface leaks
  tests: fix check-qom-proplist leaks
  tests: fix small leak in test-io-channel-command
  tests: fix leak in test-string-input-visitor
  portio: keep references on portio
  pc: simplify passing qemu_irq
  pc: don't leak a20_line
  machine: use class base init generated name
  acpi-build: fix array leak
  tests: fix qom-test leaks
  pc: free i8259
  pc: keep gsi reference
  sd: free timer
  ipmi: free extern timer
  bus: simplify name handling
  tests: add qtest_add_data_func_full
  tests: pc-cpu-test leaks fixes
  tests: fix rsp leak in postcopy-test
  tests: fix postcopy-test leaks

 hw/audio/gus.c                    |  9 ++++++---
 hw/audio/sb16.c                   |  4 +++-
 hw/block/fdc.c                    |  4 +++-
 hw/char/parallel.c                |  3 ++-
 hw/core/bus.c                     | 21 ++++++---------------
 hw/core/machine.c                 |  1 +
 hw/display/vga-isa.c              |  8 ++++++--
 hw/dma/i8257.c                    |  6 ++++--
 hw/i386/acpi-build.c              |  4 ++--
 hw/i386/pc.c                      |  9 +++++----
 hw/i386/pc_piix.c                 | 17 ++++++++---------
 hw/i386/pc_q35.c                  | 15 ++++++++-------
 hw/ide/core.c                     |  6 ++++--
 hw/input/pckbd.c                  |  4 ++--
 hw/ipmi/ipmi_bmc_extern.c         |  9 +++++++++
 hw/isa/isa-bus.c                  | 14 +++++---------
 hw/sd/sd.c                        |  9 +++++++++
 qga/guest-agent-command-state.c   |  6 ++++++
 qga/main.c                        | 13 +++++++------
 tests/check-qom-interface.c       |  1 +
 tests/check-qom-proplist.c        | 16 ++++++++++++++++
 tests/libqtest.c                  | 15 +++++++++++++++
 tests/pc-cpu-test.c               | 24 +++++++++++++++++++-----
 tests/postcopy-test.c             |  7 +++++--
 tests/qom-test.c                  |  5 +++--
 tests/test-cutils.c               | 24 ++++++++++++++++--------
 tests/test-io-channel-command.c   | 20 +++++++++++++-------
 tests/test-iov.c                  |  7 +++++++
 tests/test-qga.c                  |  8 +++++++-
 tests/test-string-input-visitor.c |  1 +
 tests/test-vmstate.c              |  8 ++++++--
 tests/libqtest.h                  | 17 +++++++++++++++++
 include/glib-compat.h             | 24 ++++++++++++++++++++++++
 include/hw/boards.h               |  2 +-
 include/hw/i386/pc.h              |  4 ++--
 include/hw/ide/internal.h         |  2 ++
 include/hw/isa/i8257.h            |  2 ++
 include/hw/isa/isa.h              |  5 ++++-
 include/hw/qdev-core.h            |  2 +-
 qga/guest-agent-core.h            |  1 +
 40 files changed, 259 insertions(+), 98 deletions(-)

-- 
2.10.0

[Qemu-devel] [PULL 01/26] tests: fix test-qga leaks

[Marc-André Lureau]

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

diff --git a/tests/test-qga.c b/tests/test-qga.c
index dac8fb8..21f44f8 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -398,6 +398,7 @@ static void test_qga_file_ops(gconstpointer fix)
     /* check content */
     path = g_build_filename(fixture->test_dir, "foo", NULL);
     f = fopen(path, "r");
+    g_free(path);
     g_assert_nonnull(f);
     count = fread(tmp, 1, sizeof(tmp), f);
     g_assert_cmpint(count, ==, sizeof(helloworld));
@@ -700,7 +701,9 @@ static void test_qga_config(gconstpointer data)
     cwd = g_get_current_dir();
     cmd = g_strdup_printf("%s%cqemu-ga -D",
                           cwd, G_DIR_SEPARATOR);
+    g_free(cwd);
     g_shell_parse_argv(cmd, NULL, &argv, &error);
+    g_free(cmd);
     g_assert_no_error(error);
 
     env[0] = g_strdup_printf("QGA_CONF=tests%cdata%ctest-qga-config",
@@ -708,6 +711,8 @@ static void test_qga_config(gconstpointer data)
     env[1] = NULL;
     g_spawn_sync(NULL, argv, env, 0,
                  NULL, NULL, &out, &err, &status, &error);
+    g_strfreev(argv);
+
     g_assert_no_error(error);
     g_assert_cmpstr(err, ==, "");
     g_assert_cmpint(status, ==, 0);
-- 
2.10.0

[Qemu-devel] [PULL 02/26] glib-compat: add g_(s)list_free_full()

[Marc-André Lureau]

Those functions are only available since glib 2.28.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 include/glib-compat.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/glib-compat.h b/include/glib-compat.h
index 8d5a7f3..8093163 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -280,4 +280,28 @@ static inline void g_hash_table_add(GHashTable *hash_table, gpointer key)
     } while (0)
 #endif
 
+#if !GLIB_CHECK_VERSION(2, 28, 0)
+static inline void g_list_free_full(GList *list, GDestroyNotify free_func)
+{
+    GList *l;
+
+    for (l = list; l; l = l->next) {
+        free_func(l->data);
+    }
+
+    g_list_free(list);
+}
+
+static inline void g_slist_free_full(GSList *list, GDestroyNotify free_func)
+{
+    GSList *l;
+
+    for (l = list; l; l = l->next) {
+        free_func(l->data);
+    }
+
+    g_slist_free(list);
+}
+#endif
+
 #endif
-- 
2.10.0

[Qemu-devel] [PULL 03/26] qga: free the whole blacklist

[Marc-André Lureau]

Free the config blacklist list, not just the elements. Do it so in the
more appropriate function config_free().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qga/main.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 4c3b2c7..bb48214 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -1175,6 +1175,7 @@ static void config_free(GAConfig *config)
 #ifdef CONFIG_FSFREEZE
     g_free(config->fsfreeze_hook);
 #endif
+    g_list_free_full(config->blacklist, g_free);
     g_free(config);
 }
 
@@ -1310,11 +1311,6 @@ static int run_agent(GAState *s, GAConfig *config)
     return EXIT_SUCCESS;
 }
 
-static void free_blacklist_entry(gpointer entry, gpointer unused)
-{
-    g_free(entry);
-}
-
 int main(int argc, char **argv)
 {
     int ret = EXIT_SUCCESS;
@@ -1379,7 +1375,6 @@ end:
     if (s->channel) {
         ga_channel_free(s->channel);
     }
-    g_list_foreach(config->blacklist, free_blacklist_entry, NULL);
     g_free(s->pstate_filepath);
     g_free(s->state_filepath_isfrozen);
 
-- 
2.10.0

[Qemu-devel] [PULL 04/26] qga: free remaining leaking state

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qga/guest-agent-command-state.c | 6 ++++++
 qga/main.c                      | 6 ++++++
 qga/guest-agent-core.h          | 1 +
 3 files changed, 13 insertions(+)

diff --git a/qga/guest-agent-command-state.c b/qga/guest-agent-command-state.c
index 4de229c..e609d32 100644
--- a/qga/guest-agent-command-state.c
+++ b/qga/guest-agent-command-state.c
@@ -71,3 +71,9 @@ GACommandState *ga_command_state_new(void)
     cs->groups = NULL;
     return cs;
 }
+
+void ga_command_state_free(GACommandState *cs)
+{
+    g_slist_free_full(cs->groups, g_free);
+    g_free(cs);
+}
diff --git a/qga/main.c b/qga/main.c
index bb48214..0b9d04e 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -1371,6 +1371,8 @@ int main(int argc, char **argv)
 end:
     if (s->command_state) {
         ga_command_state_cleanup_all(s->command_state);
+        ga_command_state_free(s->command_state);
+        json_message_parser_destroy(&s->parser);
     }
     if (s->channel) {
         ga_channel_free(s->channel);
@@ -1383,6 +1385,10 @@ end:
     }
 
     config_free(config);
+    if (s->main_loop) {
+        g_main_loop_unref(s->main_loop);
+    }
+    g_free(s);
 
     return ret;
 }
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
index 0a49516..63e9d39 100644
--- a/qga/guest-agent-core.h
+++ b/qga/guest-agent-core.h
@@ -28,6 +28,7 @@ void ga_command_state_add(GACommandState *cs,
 void ga_command_state_init_all(GACommandState *cs);
 void ga_command_state_cleanup_all(GACommandState *cs);
 GACommandState *ga_command_state_new(void);
+void ga_command_state_free(GACommandState *cs);
 bool ga_logging_enabled(GAState *s);
 void ga_disable_logging(GAState *s);
 void ga_enable_logging(GAState *s);
-- 
2.10.0

[Qemu-devel] [PULL 05/26] tests: fix test-cutils leaks

[Marc-André Lureau]

Spotted thanks to ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/test-cutils.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/tests/test-cutils.c b/tests/test-cutils.c
index 64e3e95..20b0f59 100644
--- a/tests/test-cutils.c
+++ b/tests/test-cutils.c
@@ -378,7 +378,7 @@ static void test_qemu_strtol_hex(void)
 
 static void test_qemu_strtol_max(void)
 {
-    const char *str = g_strdup_printf("%ld", LONG_MAX);
+    char *str = g_strdup_printf("%ld", LONG_MAX);
     char f = 'X';
     const char *endptr = &f;
     long res = 999;
@@ -389,6 +389,7 @@ static void test_qemu_strtol_max(void)
     g_assert_cmpint(err, ==, 0);
     g_assert_cmpint(res, ==, LONG_MAX);
     g_assert(endptr == str + strlen(str));
+    g_free(str);
 }
 
 static void test_qemu_strtol_overflow(void)
@@ -497,7 +498,7 @@ static void test_qemu_strtol_full_trailing(void)
 
 static void test_qemu_strtol_full_max(void)
 {
-    const char *str = g_strdup_printf("%ld", LONG_MAX);
+    char *str = g_strdup_printf("%ld", LONG_MAX);
     long res;
     int err;
 
@@ -505,6 +506,7 @@ static void test_qemu_strtol_full_max(void)
 
     g_assert_cmpint(err, ==, 0);
     g_assert_cmpint(res, ==, LONG_MAX);
+    g_free(str);
 }
 
 static void test_qemu_strtoul_correct(void)
@@ -662,7 +664,7 @@ static void test_qemu_strtoul_hex(void)
 
 static void test_qemu_strtoul_max(void)
 {
-    const char *str = g_strdup_printf("%lu", ULONG_MAX);
+    char *str = g_strdup_printf("%lu", ULONG_MAX);
     char f = 'X';
     const char *endptr = &f;
     unsigned long res = 999;
@@ -673,6 +675,7 @@ static void test_qemu_strtoul_max(void)
     g_assert_cmpint(err, ==, 0);
     g_assert_cmpint(res, ==, ULONG_MAX);
     g_assert(endptr == str + strlen(str));
+    g_free(str);
 }
 
 static void test_qemu_strtoul_overflow(void)
@@ -776,7 +779,7 @@ static void test_qemu_strtoul_full_trailing(void)
 
 static void test_qemu_strtoul_full_max(void)
 {
-    const char *str = g_strdup_printf("%lu", ULONG_MAX);
+    char *str = g_strdup_printf("%lu", ULONG_MAX);
     unsigned long res = 999;
     int err;
 
@@ -784,6 +787,7 @@ static void test_qemu_strtoul_full_max(void)
 
     g_assert_cmpint(err, ==, 0);
     g_assert_cmpint(res, ==, ULONG_MAX);
+    g_free(str);
 }
 
 static void test_qemu_strtoll_correct(void)
@@ -941,7 +945,7 @@ static void test_qemu_strtoll_hex(void)
 
 static void test_qemu_strtoll_max(void)
 {
-    const char *str = g_strdup_printf("%lld", LLONG_MAX);
+    char *str = g_strdup_printf("%lld", LLONG_MAX);
     char f = 'X';
     const char *endptr = &f;
     int64_t res = 999;
@@ -952,6 +956,7 @@ static void test_qemu_strtoll_max(void)
     g_assert_cmpint(err, ==, 0);
     g_assert_cmpint(res, ==, LLONG_MAX);
     g_assert(endptr == str + strlen(str));
+    g_free(str);
 }
 
 static void test_qemu_strtoll_overflow(void)
@@ -1058,7 +1063,7 @@ static void test_qemu_strtoll_full_trailing(void)
 static void test_qemu_strtoll_full_max(void)
 {
 
-    const char *str = g_strdup_printf("%lld", LLONG_MAX);
+    char *str = g_strdup_printf("%lld", LLONG_MAX);
     int64_t res;
     int err;
 
@@ -1066,6 +1071,7 @@ static void test_qemu_strtoll_full_max(void)
 
     g_assert_cmpint(err, ==, 0);
     g_assert_cmpint(res, ==, LLONG_MAX);
+    g_free(str);
 }
 
 static void test_qemu_strtoull_correct(void)
@@ -1223,7 +1229,7 @@ static void test_qemu_strtoull_hex(void)
 
 static void test_qemu_strtoull_max(void)
 {
-    const char *str = g_strdup_printf("%llu", ULLONG_MAX);
+    char *str = g_strdup_printf("%llu", ULLONG_MAX);
     char f = 'X';
     const char *endptr = &f;
     uint64_t res = 999;
@@ -1234,6 +1240,7 @@ static void test_qemu_strtoull_max(void)
     g_assert_cmpint(err, ==, 0);
     g_assert_cmpint(res, ==, ULLONG_MAX);
     g_assert(endptr == str + strlen(str));
+    g_free(str);
 }
 
 static void test_qemu_strtoull_overflow(void)
@@ -1339,7 +1346,7 @@ static void test_qemu_strtoull_full_trailing(void)
 
 static void test_qemu_strtoull_full_max(void)
 {
-    const char *str = g_strdup_printf("%lld", ULLONG_MAX);
+    char *str = g_strdup_printf("%lld", ULLONG_MAX);
     uint64_t res = 999;
     int err;
 
@@ -1347,6 +1354,7 @@ static void test_qemu_strtoull_full_max(void)
 
     g_assert_cmpint(err, ==, 0);
     g_assert_cmpint(res, ==, ULLONG_MAX);
+    g_free(str);
 }
 
 static void test_qemu_strtosz_simple(void)
-- 
2.10.0

[Qemu-devel] [PULL 06/26] tests: fix test-vmstate leaks

[Marc-André Lureau]

Spotted thanks to ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/test-vmstate.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index 41fd841..d8da26f 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -50,16 +50,20 @@ static QEMUFile *open_test_file(bool write)
 {
     int fd = dup(temp_fd);
     QIOChannel *ioc;
+    QEMUFile *f;
+
     lseek(fd, 0, SEEK_SET);
     if (write) {
         g_assert_cmpint(ftruncate(fd, 0), ==, 0);
     }
     ioc = QIO_CHANNEL(qio_channel_file_new_fd(fd));
     if (write) {
-        return qemu_fopen_channel_output(ioc);
+        f = qemu_fopen_channel_output(ioc);
     } else {
-        return qemu_fopen_channel_input(ioc);
+        f = qemu_fopen_channel_input(ioc);
     }
+    object_unref(OBJECT(ioc));
+    return f;
 }
 
 #define SUCCESS(val) \
-- 
2.10.0

[Qemu-devel] [PULL 07/26] tests: fix test-iov leaks

[Marc-André Lureau]

Spotted thanks to ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/test-iov.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tests/test-iov.c b/tests/test-iov.c
index 46ae25e..a22d71f 100644
--- a/tests/test-iov.c
+++ b/tests/test-iov.c
@@ -208,6 +208,9 @@ static void test_io(void)
                } while(k < j);
            }
        }
+       iov_free(iov, niov);
+       g_free(buf);
+       g_free(siov);
        exit(0);
 
     } else {
@@ -246,6 +249,10 @@ static void test_io(void)
                test_iov_bytes(iov, niov, i, j - i);
            }
         }
+
+       iov_free(iov, niov);
+       g_free(buf);
+       g_free(siov);
      }
 #endif
 }
-- 
2.10.0

[Qemu-devel] [PULL 08/26] tests: fix check-qom-interface leaks

[Marc-André Lureau]

Found thanks to ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/check-qom-interface.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/check-qom-interface.c b/tests/check-qom-interface.c
index 719ddcf..f87c9aa 100644
--- a/tests/check-qom-interface.c
+++ b/tests/check-qom-interface.c
@@ -76,6 +76,7 @@ static void test_interface_impl(const char *type)
 
     g_assert(iobj);
     g_assert(ioc->test == PATTERN);
+    object_unref(obj);
 }
 
 static void interface_direct_test(void)
-- 
2.10.0

[Qemu-devel] [PULL 09/26] tests: fix check-qom-proplist leaks

[Marc-André Lureau]

Found thanks to ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/check-qom-proplist.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 42defe7..a16cefc 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -230,6 +230,13 @@ struct DummyBackendClass {
 };
 
 
+static void dummy_dev_finalize(Object *obj)
+{
+    DummyDev *dev = DUMMY_DEV(obj);
+
+    object_unref(OBJECT(dev->bus));
+}
+
 static void dummy_dev_init(Object *obj)
 {
     DummyDev *dev = DUMMY_DEV(obj);
@@ -257,6 +264,13 @@ static void dummy_dev_class_init(ObjectClass *klass, void *opaque)
 }
 
 
+static void dummy_bus_finalize(Object *obj)
+{
+    DummyBus *bus = DUMMY_BUS(obj);
+
+    object_unref(OBJECT(bus->backend));
+}
+
 static void dummy_bus_init(Object *obj)
 {
 }
@@ -283,6 +297,7 @@ static const TypeInfo dummy_dev_info = {
     .parent        = TYPE_OBJECT,
     .instance_size = sizeof(DummyDev),
     .instance_init = dummy_dev_init,
+    .instance_finalize = dummy_dev_finalize,
     .class_size = sizeof(DummyDevClass),
     .class_init = dummy_dev_class_init,
 };
@@ -292,6 +307,7 @@ static const TypeInfo dummy_bus_info = {
     .parent        = TYPE_OBJECT,
     .instance_size = sizeof(DummyBus),
     .instance_init = dummy_bus_init,
+    .instance_finalize = dummy_bus_finalize,
     .class_size = sizeof(DummyBusClass),
     .class_init = dummy_bus_class_init,
 };
-- 
2.10.0

[Qemu-devel] [PULL 10/26] tests: fix small leak in test-io-channel-command

[Marc-André Lureau]

srcfifo && dstfifo must be freed in error case, however unlink() may
delete a file from a different context. Instead, use mkdtemp()/rmdir()
for the temporary files.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/test-io-channel-command.c | 20 +++++++++++++-------
 tests/test-qga.c                |  3 ++-
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/tests/test-io-channel-command.c b/tests/test-io-channel-command.c
index 1d1f461..f99118e 100644
--- a/tests/test-io-channel-command.c
+++ b/tests/test-io-channel-command.c
@@ -18,6 +18,7 @@
  *
  */
 
+#include <glib/gstdio.h>
 #include "qemu/osdep.h"
 #include "io/channel-command.h"
 #include "io-channel-helpers.h"
@@ -26,11 +27,14 @@
 #ifndef WIN32
 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);
+    char *tmpdir = g_strdup("/tmp/test-io-channel.XXXXXX");
+    g_assert_nonnull(mkdtemp(tmpdir));
+
+    char *fifo = g_strdup_printf("%s/command.fifo", tmpdir);
+    char *srcfifo = g_strdup_printf("PIPE:%s,wronly", fifo);
+    char *dstfifo = g_strdup_printf("PIPE:%s,rdonly", fifo);
     const char *srcargv[] = {
         "/bin/socat", "-", srcfifo, NULL,
     };
@@ -38,11 +42,10 @@ static void test_io_channel_command_fifo(bool async)
         "/bin/socat", dstfifo, "-", NULL,
     };
 
-    unlink(TEST_FIFO);
     if (access("/bin/socat", X_OK) < 0) {
-        return; /* Pretend success if socat is not present */
+        goto end; /* Pretend success if socat is not present */
     }
-    if (mkfifo(TEST_FIFO, 0600) < 0) {
+    if (mkfifo(fifo, 0600) < 0) {
         abort();
     }
     src = QIO_CHANNEL(qio_channel_command_new_spawn(srcargv,
@@ -59,9 +62,12 @@ static void test_io_channel_command_fifo(bool async)
     object_unref(OBJECT(src));
     object_unref(OBJECT(dst));
 
+end:
+    g_free(fifo);
     g_free(srcfifo);
     g_free(dstfifo);
-    unlink(TEST_FIFO);
+    g_rmdir(tmpdir);
+    g_free(tmpdir);
 }
 
 
diff --git a/tests/test-qga.c b/tests/test-qga.c
index 21f44f8..0d1acef 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -55,7 +55,8 @@ fixture_setup(TestFixture *fixture, gconstpointer data)
     fixture->loop = g_main_loop_new(NULL, FALSE);
 
     fixture->test_dir = g_strdup("/tmp/qgatest.XXXXXX");
-    g_assert_nonnull(mkdtemp(fixture->test_dir));
+    path = mkdtemp(fixture->test_dir);
+    g_assert_nonnull(path);
 
     path = g_build_filename(fixture->test_dir, "sock", NULL);
     cwd = g_get_current_dir();
-- 
2.10.0

[Qemu-devel] [PULL 11/26] tests: fix leak in test-string-input-visitor

[Marc-André Lureau]

Free the list returned by visit_type_intList().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/test-string-input-visitor.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index d837ebe..a679fbc 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -228,6 +228,7 @@ static void test_visitor_in_fuzz(TestInputVisitorData *data,
 
         v = visitor_input_test_init(data, buf);
         visit_type_intList(v, NULL, &ilres, NULL);
+        qapi_free_intList(ilres);
         visitor_input_teardown(data, NULL);
 
         v = visitor_input_test_init(data, buf);
-- 
2.10.0

[Qemu-devel] [PULL 12/26] portio: keep references on portio

[Marc-André Lureau]

The isa_register_portio_list() function allocates ioports
data/state. Let's keep the reference to this data on some owner.  This
isn't enough to fix leaks, but at least, ASAN stops complaining of
direct leaks. Further cleanup would require calling
portio_list_del/destroy().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/audio/gus.c            |  9 ++++++---
 hw/audio/sb16.c           |  4 +++-
 hw/block/fdc.c            |  4 +++-
 hw/char/parallel.c        |  3 ++-
 hw/display/vga-isa.c      |  8 ++++++--
 hw/dma/i8257.c            |  6 ++++--
 hw/ide/core.c             |  6 ++++--
 hw/isa/isa-bus.c          | 14 +++++---------
 include/hw/ide/internal.h |  2 ++
 include/hw/isa/i8257.h    |  2 ++
 include/hw/isa/isa.h      |  5 ++++-
 11 files changed, 41 insertions(+), 22 deletions(-)

diff --git a/hw/audio/gus.c b/hw/audio/gus.c
index 6c02646..3d08a65 100644
--- a/hw/audio/gus.c
+++ b/hw/audio/gus.c
@@ -60,6 +60,8 @@ typedef struct GUSState {
     int64_t last_ticks;
     qemu_irq pic;
     IsaDma *isa_dma;
+    PortioList portio_list1;
+    PortioList portio_list2;
 } GUSState;
 
 static uint32_t gus_readb(void *opaque, uint32_t nport)
@@ -265,9 +267,10 @@ static void gus_realizefn (DeviceState *dev, Error **errp)
     s->samples = AUD_get_buffer_size_out (s->voice) >> s->shift;
     s->mixbuf = g_malloc0 (s->samples << s->shift);
 
-    isa_register_portio_list (d, s->port, gus_portio_list1, s, "gus");
-    isa_register_portio_list (d, (s->port + 0x100) & 0xf00,
-                              gus_portio_list2, s, "gus");
+    isa_register_portio_list(d, &s->portio_list1, s->port,
+                             gus_portio_list1, s, "gus");
+    isa_register_portio_list(d, &s->portio_list2, (s->port + 0x100) & 0xf00,
+                             gus_portio_list2, s, "gus");
 
     s->isa_dma = isa_get_dma(isa_bus_from_device(d), s->emu.gusdma);
     k = ISADMA_GET_CLASS(s->isa_dma);
diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c
index 3a4a57a..6b4427f 100644
--- a/hw/audio/sb16.c
+++ b/hw/audio/sb16.c
@@ -106,6 +106,7 @@ typedef struct SB16State {
     /* mixer state */
     int mixer_nreg;
     uint8_t mixer_regs[256];
+    PortioList portio_list;
 } SB16State;
 
 static void SB_audio_callback (void *opaque, int free);
@@ -1378,7 +1379,8 @@ static void sb16_realizefn (DeviceState *dev, Error **errp)
         dolog ("warning: Could not create auxiliary timer\n");
     }
 
-    isa_register_portio_list (isadev, s->port, sb16_ioport_list, s, "sb16");
+    isa_register_portio_list(isadev, &s->portio_list, s->port,
+                             sb16_ioport_list, s, "sb16");
 
     s->isa_hdma = isa_get_dma(isa_bus_from_device(isadev), s->hdma);
     k = ISADMA_GET_CLASS(s->isa_hdma);
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index f73af7d..b79873a 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -692,6 +692,7 @@ struct FDCtrl {
     /* Timers state */
     uint8_t timer0;
     uint8_t timer1;
+    PortioList portio_list;
 };
 
 static FloppyDriveType get_fallback_drive_type(FDrive *drv)
@@ -2495,7 +2496,8 @@ static void isabus_fdc_realize(DeviceState *dev, Error **errp)
     FDCtrl *fdctrl = &isa->state;
     Error *err = NULL;
 
-    isa_register_portio_list(isadev, isa->iobase, fdc_portio_list, fdctrl,
+    isa_register_portio_list(isadev, &fdctrl->portio_list,
+                             isa->iobase, fdc_portio_list, fdctrl,
                              "fdc");
 
     isa_init_irq(isadev, &fdctrl->irq, isa->irq);
diff --git a/hw/char/parallel.c b/hw/char/parallel.c
index 11c78fe..fa08566 100644
--- a/hw/char/parallel.c
+++ b/hw/char/parallel.c
@@ -80,6 +80,7 @@ typedef struct ParallelState {
     uint32_t last_read_offset; /* For debugging */
     /* Memory-mapped interface */
     int it_shift;
+    PortioList portio_list;
 } ParallelState;
 
 #define TYPE_ISA_PARALLEL "isa-parallel"
@@ -532,7 +533,7 @@ static void parallel_isa_realizefn(DeviceState *dev, Error **errp)
         s->status = dummy;
     }
 
-    isa_register_portio_list(isadev, base,
+    isa_register_portio_list(isadev, &s->portio_list, base,
                              (s->hw_driver
                               ? &isa_parallel_portio_hw_list[0]
                               : &isa_parallel_portio_sw_list[0]),
diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
index f5aff1c..1af9556 100644
--- a/hw/display/vga-isa.c
+++ b/hw/display/vga-isa.c
@@ -39,6 +39,8 @@ typedef struct ISAVGAState {
     ISADevice parent_obj;
 
     struct VGACommonState state;
+    PortioList portio_vga;
+    PortioList portio_vbe;
 } ISAVGAState;
 
 static void vga_isa_reset(DeviceState *dev)
@@ -60,9 +62,11 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
     vga_common_init(s, OBJECT(dev), true);
     s->legacy_address_space = isa_address_space(isadev);
     vga_io_memory = vga_init_io(s, OBJECT(dev), &vga_ports, &vbe_ports);
-    isa_register_portio_list(isadev, 0x3b0, vga_ports, s, "vga");
+    isa_register_portio_list(isadev, &d->portio_vga,
+                             0x3b0, vga_ports, s, "vga");
     if (vbe_ports) {
-        isa_register_portio_list(isadev, 0x1ce, vbe_ports, s, "vbe");
+        isa_register_portio_list(isadev, &d->portio_vbe,
+                                 0x1ce, vbe_ports, s, "vbe");
     }
     memory_region_add_subregion_overlap(isa_address_space(isadev),
                                         0x000a0000,
diff --git a/hw/dma/i8257.c b/hw/dma/i8257.c
index f345c54..bffbdea 100644
--- a/hw/dma/i8257.c
+++ b/hw/dma/i8257.c
@@ -553,10 +553,12 @@ static void i8257_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion(isa_address_space_io(isa),
                                 d->base, &d->channel_io);
 
-    isa_register_portio_list(isa, d->page_base, page_portio_list, d,
+    isa_register_portio_list(isa, &d->portio_page,
+                             d->page_base, page_portio_list, d,
                              "dma-page");
     if (d->pageh_base >= 0) {
-        isa_register_portio_list(isa, d->pageh_base, pageh_portio_list, d,
+        isa_register_portio_list(isa, &d->portio_pageh,
+                                 d->pageh_base, pageh_portio_list, d,
                                  "dma-pageh");
     }
 
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 45b6df1..b0e42a6 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2619,10 +2619,12 @@ void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
 {
     /* ??? Assume only ISA and PCI configurations, and that the PCI-ISA
        bridge has been setup properly to always register with ISA.  */
-    isa_register_portio_list(dev, iobase, ide_portio_list, bus, "ide");
+    isa_register_portio_list(dev, &bus->portio_list,
+                             iobase, ide_portio_list, bus, "ide");
 
     if (iobase2) {
-        isa_register_portio_list(dev, iobase2, ide_portio2_list, bus, "ide");
+        isa_register_portio_list(dev, &bus->portio2_list,
+                                 iobase2, ide_portio2_list, bus, "ide");
     }
 }
 
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index ce74db2..9d07b11 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -131,24 +131,20 @@ void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start)
     isa_init_ioport(dev, start);
 }
 
-void isa_register_portio_list(ISADevice *dev, uint16_t start,
+void isa_register_portio_list(ISADevice *dev,
+                              PortioList *piolist, uint16_t start,
                               const MemoryRegionPortio *pio_start,
                               void *opaque, const char *name)
 {
-    PortioList piolist;
+    assert(piolist && !piolist->owner);
 
     /* START is how we should treat DEV, regardless of the actual
        contents of the portio array.  This is how the old code
        actually handled e.g. the FDC device.  */
     isa_init_ioport(dev, start);
 
-    /* FIXME: the device should store created PortioList in its state.  Note
-       that DEV can be NULL here and that single device can register several
-       portio lists.  Current implementation is leaking memory allocated
-       in portio_list_init.  The leak is not critical because it happens only
-       at initialization time.  */
-    portio_list_init(&piolist, OBJECT(dev), pio_start, opaque, name);
-    portio_list_add(&piolist, isabus->address_space_io, start);
+    portio_list_init(piolist, OBJECT(dev), pio_start, opaque, name);
+    portio_list_add(piolist, isabus->address_space_io, start);
 }
 
 static void isa_device_init(Object *obj)
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 7824bc3..a6dd2c3 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -480,6 +480,8 @@ struct IDEBus {
     uint8_t retry_unit;
     int64_t retry_sector_num;
     uint32_t retry_nsector;
+    PortioList portio_list;
+    PortioList portio2_list;
 };
 
 #define TYPE_IDE_DEVICE "ide-device"
diff --git a/include/hw/isa/i8257.h b/include/hw/isa/i8257.h
index aa211c0..88a2766 100644
--- a/include/hw/isa/i8257.h
+++ b/include/hw/isa/i8257.h
@@ -36,6 +36,8 @@ typedef struct I8257State {
     QEMUBH *dma_bh;
     bool dma_bh_scheduled;
     int running;
+    PortioList portio_page;
+    PortioList portio_pageh;
 } I8257State;
 
 #endif
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index 7693ac5..c2fdd70 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -134,12 +134,15 @@ void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start);
  * device and use the legacy portio routines.
  *
  * @dev: the ISADevice against which these are registered; may be NULL.
+ * @piolist: the PortioList associated with the io ports
  * @start: the base I/O port against which the portio->offset is applied.
  * @portio: the ports, sorted by offset.
  * @opaque: passed into the portio callbacks.
  * @name: passed into memory_region_init_io.
  */
-void isa_register_portio_list(ISADevice *dev, uint16_t start,
+void isa_register_portio_list(ISADevice *dev,
+                              PortioList *piolist,
+                              uint16_t start,
                               const MemoryRegionPortio *portio,
                               void *opaque, const char *name);
 
-- 
2.10.0

[Qemu-devel] [PULL 13/26] pc: simplify passing qemu_irq

[Marc-André Lureau]

qemu_irq is already a pointer, no need to have an extra pointer level.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hw/i386/pc.c         | 8 ++++----
 hw/input/pckbd.c     | 4 ++--
 include/hw/i386/pc.h | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 022dd1b..15966ce 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -530,9 +530,9 @@ static uint64_t port92_read(void *opaque, hwaddr addr,
     return ret;
 }
 
-static void port92_init(ISADevice *dev, qemu_irq *a20_out)
+static void port92_init(ISADevice *dev, qemu_irq a20_out)
 {
-    qdev_connect_gpio_out_named(DEVICE(dev), PORT92_A20_LINE, 0, *a20_out);
+    qdev_connect_gpio_out_named(DEVICE(dev), PORT92_A20_LINE, 0, a20_out);
 }
 
 static const VMStateDescription vmstate_port92_isa = {
@@ -1594,7 +1594,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
 
     a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 2);
     i8042 = isa_create_simple(isa_bus, "i8042");
-    i8042_setup_a20_line(i8042, &a20_line[0]);
+    i8042_setup_a20_line(i8042, a20_line[0]);
     if (!no_vmport) {
         vmport_init(isa_bus);
         vmmouse = isa_try_create(isa_bus, "vmmouse");
@@ -1607,7 +1607,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
         qdev_init_nofail(dev);
     }
     port92 = isa_create_simple(isa_bus, "port92");
-    port92_init(port92, &a20_line[1]);
+    port92_init(port92, a20_line[1]);
 
     DMA_init(isa_bus, 0);
 
diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index dc57e2c..d414288 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -499,9 +499,9 @@ void i8042_isa_mouse_fake_event(void *opaque)
     ps2_mouse_fake_event(s->mouse);
 }
 
-void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out)
+void i8042_setup_a20_line(ISADevice *dev, qemu_irq a20_out)
 {
-    qdev_connect_gpio_out_named(DEVICE(dev), I8042_A20_LINE, 0, *a20_out);
+    qdev_connect_gpio_out_named(DEVICE(dev), I8042_A20_LINE, 0, a20_out);
 }
 
 static const VMStateDescription vmstate_kbd_isa = {
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 74c175c..330c1f2 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -220,7 +220,7 @@ void i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
                    MemoryRegion *region, ram_addr_t size,
                    hwaddr mask);
 void i8042_isa_mouse_fake_event(void *opaque);
-void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out);
+void i8042_setup_a20_line(ISADevice *dev, qemu_irq a20_out);
 
 /* pc.c */
 extern int fd_bootchk;
-- 
2.10.0

[Qemu-devel] [PULL 14/26] pc: don't leak a20_line

[Marc-André Lureau]

The irqs array is no longer being used

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hw/i386/pc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 15966ce..e31f70f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1608,6 +1608,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
     }
     port92 = isa_create_simple(isa_bus, "port92");
     port92_init(port92, a20_line[1]);
+    g_free(a20_line);
 
     DMA_init(isa_bus, 0);
 
-- 
2.10.0

[Qemu-devel] [PULL 15/26] machine: use class base init generated name

[Marc-André Lureau]

machine_class_base_init() member name is allocated by
machine_class_base_init(), but not freed by
machine_class_finalize().  Simply freeing there doesn't work,
because DEFINE_PC_MACHINE() overwrites it with a literal string.

Fix DEFINE_PC_MACHINE() not to overwrite it, and add the missing
free to machine_class_finalize().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 hw/core/machine.c    | 1 +
 include/hw/boards.h  | 2 +-
 include/hw/i386/pc.h | 1 -
 3 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index e5a456f..00fbe3e 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -561,6 +561,7 @@ static void machine_class_finalize(ObjectClass *klass, void *data)
     if (mc->compat_props) {
         g_array_free(mc->compat_props, true);
     }
+    g_free(mc->name);
 }
 
 void machine_register_compat_props(MachineState *machine)
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 3e69eca..e46a744 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -93,7 +93,7 @@ struct MachineClass {
     /*< public >*/
 
     const char *family; /* NULL iff @name identifies a standalone machtype */
-    const char *name;
+    char *name;
     const char *alias;
     const char *desc;
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 330c1f2..422fac7 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -903,7 +903,6 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
     { \
         MachineClass *mc = MACHINE_CLASS(oc); \
         optsfn(mc); \
-        mc->name = namestr; \
         mc->init = initfn; \
     } \
     static const TypeInfo pc_machine_type_##suffix = { \
-- 
2.10.0

[Qemu-devel] [PULL 16/26] acpi-build: fix array leak

[Marc-André Lureau]

The free_ranges array is used as a temporary pointer array, the segment
should still be freed, however, it shouldn't free the elements themself.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Tested-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/i386/acpi-build.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index a26a4bb..433feba 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -789,7 +789,7 @@ static gint crs_range_compare(gconstpointer a, gconstpointer b)
 static void crs_replace_with_free_ranges(GPtrArray *ranges,
                                          uint64_t start, uint64_t end)
 {
-    GPtrArray *free_ranges = g_ptr_array_new_with_free_func(crs_range_free);
+    GPtrArray *free_ranges = g_ptr_array_new();
     uint64_t free_base = start;
     int i;
 
@@ -813,7 +813,7 @@ static void crs_replace_with_free_ranges(GPtrArray *ranges,
         g_ptr_array_add(ranges, g_ptr_array_index(free_ranges, i));
     }
 
-    g_ptr_array_free(free_ranges, false);
+    g_ptr_array_free(free_ranges, true);
 }
 
 /*
-- 
2.10.0

[Qemu-devel] [PULL 17/26] tests: fix qom-test leaks

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/qom-test.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/qom-test.c b/tests/qom-test.c
index 23493a2..d48f890 100644
--- a/tests/qom-test.c
+++ b/tests/qom-test.c
@@ -115,7 +115,7 @@ static void add_machine_test_cases(void)
     const QListEntry *p;
     QObject *qobj;
     QString *qstr;
-    const char *mname, *path;
+    const char *mname;
 
     qtest_start("-machine none");
     response = qmp("{ 'execute': 'query-machines' }");
@@ -132,8 +132,9 @@ static void add_machine_test_cases(void)
         g_assert(qstr);
         mname = qstring_get_str(qstr);
         if (!is_blacklisted(arch, mname)) {
-            path = g_strdup_printf("qom/%s", mname);
+            char *path = g_strdup_printf("qom/%s", mname);
             qtest_add_data_func(path, g_strdup(mname), test_machine);
+            g_free(path);
         }
     }
 
-- 
2.10.0

[Qemu-devel] [PULL 18/26] pc: free i8259

[Marc-André Lureau]

Simiarly to 2ba154cf4eb8636cdd3aa90f392ca9e77206ca39

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/i386/pc_q35.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index c0b9961..c5e8367 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -213,6 +213,8 @@ static void pc_q35_init(MachineState *machine)
     for (i = 0; i < ISA_NUM_IRQS; i++) {
         gsi_state->i8259_irq[i] = i8259[i];
     }
+    g_free(i8259);
+
     if (pcmc->pci_enabled) {
         ioapic_init_gsi(gsi_state, "q35");
     }
-- 
2.10.0

[Qemu-devel] [PULL 19/26] pc: keep gsi reference

[Marc-André Lureau]

Further cleanup would need to call qemu_free_irq() at the appropriate
time, but for now this silences ASAN about direct leaks.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 hw/i386/pc_piix.c    | 17 ++++++++---------
 hw/i386/pc_q35.c     | 13 ++++++-------
 include/hw/i386/pc.h |  1 +
 3 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a07dc81..2af8888 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -74,7 +74,6 @@ static void pc_init1(MachineState *machine,
     ISABus *isa_bus;
     PCII440FXState *i440fx_state;
     int piix3_devfn = -1;
-    qemu_irq *gsi;
     qemu_irq *i8259;
     qemu_irq smi_irq;
     GSIState *gsi_state;
@@ -185,16 +184,16 @@ static void pc_init1(MachineState *machine,
     gsi_state = g_malloc0(sizeof(*gsi_state));
     if (kvm_ioapic_in_kernel()) {
         kvm_pc_setup_irq_routing(pcmc->pci_enabled);
-        gsi = qemu_allocate_irqs(kvm_pc_gsi_handler, gsi_state,
-                                 GSI_NUM_PINS);
+        pcms->gsi = qemu_allocate_irqs(kvm_pc_gsi_handler, gsi_state,
+                                       GSI_NUM_PINS);
     } else {
-        gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
+        pcms->gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
     }
 
     if (pcmc->pci_enabled) {
         pci_bus = i440fx_init(host_type,
                               pci_type,
-                              &i440fx_state, &piix3_devfn, &isa_bus, gsi,
+                              &i440fx_state, &piix3_devfn, &isa_bus, pcms->gsi,
                               system_memory, system_io, machine->ram_size,
                               pcms->below_4g_mem_size,
                               pcms->above_4g_mem_size,
@@ -207,7 +206,7 @@ static void pc_init1(MachineState *machine,
                               &error_abort);
         no_hpet = 1;
     }
-    isa_bus_irqs(isa_bus, gsi);
+    isa_bus_irqs(isa_bus, pcms->gsi);
 
     if (kvm_pic_in_kernel()) {
         i8259 = kvm_i8259_init(isa_bus);
@@ -225,7 +224,7 @@ static void pc_init1(MachineState *machine,
         ioapic_init_gsi(gsi_state, "i440fx");
     }
 
-    pc_register_ferr_irq(gsi[13]);
+    pc_register_ferr_irq(pcms->gsi[13]);
 
     pc_vga_init(isa_bus, pcmc->pci_enabled ? pci_bus : NULL);
 
@@ -235,7 +234,7 @@ static void pc_init1(MachineState *machine,
     }
 
     /* init basic PC hardware */
-    pc_basic_device_init(isa_bus, gsi, &rtc_state, true,
+    pc_basic_device_init(isa_bus, pcms->gsi, &rtc_state, true,
                          (pcms->vmport != ON_OFF_AUTO_ON), 0x4);
 
     pc_nic_init(isa_bus, pci_bus);
@@ -279,7 +278,7 @@ static void pc_init1(MachineState *machine,
         smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
         /* TODO: Populate SPD eeprom data.  */
         smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
-                              gsi[9], smi_irq,
+                              pcms->gsi[9], smi_irq,
                               pc_machine_is_smm_enabled(pcms),
                               &piix4_pm);
         smbus_eeprom_init(smbus, 8, NULL, 0);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index c5e8367..3cbcbb0 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -69,7 +69,6 @@ static void pc_q35_init(MachineState *machine)
     MemoryRegion *ram_memory;
     GSIState *gsi_state;
     ISABus *isa_bus;
-    qemu_irq *gsi;
     qemu_irq *i8259;
     int i;
     ICH9LPCState *ich9_lpc;
@@ -153,10 +152,10 @@ static void pc_q35_init(MachineState *machine)
     gsi_state = g_malloc0(sizeof(*gsi_state));
     if (kvm_ioapic_in_kernel()) {
         kvm_pc_setup_irq_routing(pcmc->pci_enabled);
-        gsi = qemu_allocate_irqs(kvm_pc_gsi_handler, gsi_state,
-                                 GSI_NUM_PINS);
+        pcms->gsi = qemu_allocate_irqs(kvm_pc_gsi_handler, gsi_state,
+                                       GSI_NUM_PINS);
     } else {
-        gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
+        pcms->gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
     }
 
     /* create pci host bus */
@@ -195,7 +194,7 @@ static void pc_q35_init(MachineState *machine)
     ich9_lpc = ICH9_LPC_DEVICE(lpc);
     lpc_dev = DEVICE(lpc);
     for (i = 0; i < GSI_NUM_PINS; i++) {
-        qdev_connect_gpio_out_named(lpc_dev, ICH9_GPIO_GSI, i, gsi[i]);
+        qdev_connect_gpio_out_named(lpc_dev, ICH9_GPIO_GSI, i, pcms->gsi[i]);
     }
     pci_bus_irqs(host_bus, ich9_lpc_set_irq, ich9_lpc_map_irq, ich9_lpc,
                  ICH9_LPC_NB_PIRQS);
@@ -219,7 +218,7 @@ static void pc_q35_init(MachineState *machine)
         ioapic_init_gsi(gsi_state, "q35");
     }
 
-    pc_register_ferr_irq(gsi[13]);
+    pc_register_ferr_irq(pcms->gsi[13]);
 
     assert(pcms->vmport != ON_OFF_AUTO__MAX);
     if (pcms->vmport == ON_OFF_AUTO_AUTO) {
@@ -227,7 +226,7 @@ static void pc_q35_init(MachineState *machine)
     }
 
     /* init basic PC hardware */
-    pc_basic_device_init(isa_bus, gsi, &rtc_state, !mc->no_floppy,
+    pc_basic_device_init(isa_bus, pcms->gsi, &rtc_state, !mc->no_floppy,
                          (pcms->vmport != ON_OFF_AUTO_ON), 0xff0104);
 
     /* connect pm stuff to lpc */
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 422fac7..c4316c3 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -53,6 +53,7 @@ struct PCMachineState {
     ISADevice *rtc;
     PCIBus *bus;
     FWCfgState *fw_cfg;
+    qemu_irq *gsi;
 
     /* Configuration options: */
     uint64_t max_ram_below_4g;
-- 
2.10.0

[Qemu-devel] [PULL 20/26] sd: free timer

[Marc-André Lureau]

Free the timer allocated in instance_init.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
---
 hw/sd/sd.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 87c6dc1..8e88e83 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1876,6 +1876,14 @@ static void sd_instance_init(Object *obj)
     sd->ocr_power_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sd_ocr_powerup, sd);
 }
 
+static void sd_instance_finalize(Object *obj)
+{
+    SDState *sd = SD_CARD(obj);
+
+    timer_del(sd->ocr_power_timer);
+    timer_free(sd->ocr_power_timer);
+}
+
 static void sd_realize(DeviceState *dev, Error **errp)
 {
     SDState *sd = SD_CARD(dev);
@@ -1927,6 +1935,7 @@ static const TypeInfo sd_info = {
     .class_size = sizeof(SDCardClass),
     .class_init = sd_class_init,
     .instance_init = sd_instance_init,
+    .instance_finalize = sd_instance_finalize,
 };
 
 static void sd_register_types(void)
-- 
2.10.0

[Qemu-devel] [PULL 21/26] ipmi: free extern timer

[Marc-André Lureau]

Free the timer allocated during instance init.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Corey Minyard <cminyard@mvista.com>
---
 hw/ipmi/ipmi_bmc_extern.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
index 157879e..5b73983 100644
--- a/hw/ipmi/ipmi_bmc_extern.c
+++ b/hw/ipmi/ipmi_bmc_extern.c
@@ -487,6 +487,14 @@ static void ipmi_bmc_extern_init(Object *obj)
     vmstate_register(NULL, 0, &vmstate_ipmi_bmc_extern, ibe);
 }
 
+static void ipmi_bmc_extern_finalize(Object *obj)
+{
+    IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj);
+
+    timer_del(ibe->extern_timer);
+    timer_free(ibe->extern_timer);
+}
+
 static Property ipmi_bmc_extern_properties[] = {
     DEFINE_PROP_CHR("chardev", IPMIBmcExtern, chr),
     DEFINE_PROP_END_OF_LIST(),
@@ -508,6 +516,7 @@ static const TypeInfo ipmi_bmc_extern_type = {
     .parent        = TYPE_IPMI_BMC,
     .instance_size = sizeof(IPMIBmcExtern),
     .instance_init = ipmi_bmc_extern_init,
+    .instance_finalize = ipmi_bmc_extern_finalize,
     .class_init    = ipmi_bmc_extern_class_init,
  };
 
-- 
2.10.0

[Qemu-devel] [PULL 22/26] bus: simplify name handling

[Marc-André Lureau]

Simplify a bit the code by using g_strdup_printf() and store it in a
non-const value so casting is no longer needed, and ownership is
clearer.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hw/core/bus.c          | 21 ++++++---------------
 include/hw/qdev-core.h |  2 +-
 2 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/hw/core/bus.c b/hw/core/bus.c
index 3e3f8ac..cf383fc 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -78,8 +78,7 @@ static void qbus_realize(BusState *bus, DeviceState *parent, const char *name)
 {
     const char *typename = object_get_typename(OBJECT(bus));
     BusClass *bc;
-    char *buf;
-    int i, len, bus_id;
+    int i, bus_id;
 
     bus->parent = parent;
 
@@ -88,23 +87,15 @@ static void qbus_realize(BusState *bus, DeviceState *parent, const char *name)
     } else if (bus->parent && bus->parent->id) {
         /* parent device has id -> use it plus parent-bus-id for bus name */
         bus_id = bus->parent->num_child_bus;
-
-        len = strlen(bus->parent->id) + 16;
-        buf = g_malloc(len);
-        snprintf(buf, len, "%s.%d", bus->parent->id, bus_id);
-        bus->name = buf;
+        bus->name = g_strdup_printf("%s.%d", bus->parent->id, bus_id);
     } else {
         /* no id -> use lowercase bus type plus global bus-id for bus name */
         bc = BUS_GET_CLASS(bus);
         bus_id = bc->automatic_ids++;
-
-        len = strlen(typename) + 16;
-        buf = g_malloc(len);
-        len = snprintf(buf, len, "%s.%d", typename, bus_id);
-        for (i = 0; i < len; i++) {
-            buf[i] = qemu_tolower(buf[i]);
+        bus->name = g_strdup_printf("%s.%d", typename, bus_id);
+        for (i = 0; bus->name[i]; i++) {
+            bus->name[i] = qemu_tolower(bus->name[i]);
         }
-        bus->name = buf;
     }
 
     if (bus->parent) {
@@ -229,7 +220,7 @@ static void qbus_finalize(Object *obj)
 {
     BusState *bus = BUS(obj);
 
-    g_free((char *)bus->name);
+    g_free(bus->name);
 }
 
 static const TypeInfo bus_info = {
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 4b4b33b..2c97347 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -224,7 +224,7 @@ typedef struct BusChild {
 struct BusState {
     Object obj;
     DeviceState *parent;
-    const char *name;
+    char *name;
     HotplugHandler *hotplug_handler;
     int max_index;
     bool realized;
-- 
2.10.0

[Qemu-devel] [PULL 23/26] tests: add qtest_add_data_func_full

[Marc-André Lureau]

Allows one to specify a destroy function for the test data.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 tests/libqtest.c | 15 +++++++++++++++
 tests/libqtest.h | 17 +++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index eb00f13..9e2d0cd 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -758,6 +758,21 @@ void qtest_add_func(const char *str, void (*fn)(void))
     g_free(path);
 }
 
+void qtest_add_data_func_full(const char *str, void *data,
+                              void (*fn)(const void *),
+                              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);
+#else
+    /* 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);
+#endif
+    g_free(path);
+}
+
 void qtest_add_data_func(const char *str, const void *data,
                          void (*fn)(const void *))
 {
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 37f37ad..d2b4853 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -425,6 +425,23 @@ void qtest_add_func(const char *str, void (*fn)(void));
 void qtest_add_data_func(const char *str, const void *data,
                          void (*fn)(const void *));
 
+/**
+ * qtest_add_data_func_full:
+ * @str: Test case path.
+ * @data: Test case data
+ * @fn: Test case function
+ * @data_free_func: GDestroyNotify for data
+ *
+ * Add a GTester testcase with the given name, data and function.
+ * The path is prefixed with the architecture under test, as
+ * returned by qtest_get_arch().
+ *
+ * @data is passed to @data_free_func() on test completion.
+ */
+void qtest_add_data_func_full(const char *str, void *data,
+                              void (*fn)(const void *),
+                              GDestroyNotify data_free_func);
+
 /**
  * qtest_add:
  * @testpath: Test case path
-- 
2.10.0

[Qemu-devel] [PULL 24/26] tests: pc-cpu-test leaks fixes

[Marc-André Lureau]

The path is allocated and should be freed.

The qmp response should be unref, but then 'machine' must be duplicated.

Use a destroy function for the PCTestData.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/pc-cpu-test.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/tests/pc-cpu-test.c b/tests/pc-cpu-test.c
index 4428cea..c3a2633 100644
--- a/tests/pc-cpu-test.c
+++ b/tests/pc-cpu-test.c
@@ -14,7 +14,7 @@
 #include "qapi/qmp/types.h"
 
 struct PCTestData {
-    const char *machine;
+    char *machine;
     const char *cpu_model;
     unsigned sockets;
     unsigned cores;
@@ -71,6 +71,14 @@ static void test_pc_without_cpu_add(gconstpointer data)
     g_free(args);
 }
 
+static void test_data_free(gpointer data)
+{
+    PCTestData *pc = data;
+
+    g_free(pc->machine);
+    g_free(pc);
+}
+
 static void add_pc_test_cases(void)
 {
     QDict *response, *minfo;
@@ -78,7 +86,8 @@ static void add_pc_test_cases(void)
     const QListEntry *p;
     QObject *qobj;
     QString *qstr;
-    const char *mname, *path;
+    const char *mname;
+    char *path;
     PCTestData *data;
 
     qtest_start("-machine none");
@@ -99,7 +108,7 @@ static void add_pc_test_cases(void)
             continue;
         }
         data = g_malloc(sizeof(PCTestData));
-        data->machine = mname;
+        data->machine = g_strdup(mname);
         data->cpu_model = "Haswell"; /* 1.3+ theoretically */
         data->sockets = 1;
         data->cores = 3;
@@ -119,14 +128,19 @@ static void add_pc_test_cases(void)
             path = g_strdup_printf("cpu/%s/init/%ux%ux%u&maxcpus=%u",
                                    mname, data->sockets, data->cores,
                                    data->threads, data->maxcpus);
-            qtest_add_data_func(path, data, test_pc_without_cpu_add);
+            qtest_add_data_func_full(path, data, test_pc_without_cpu_add,
+                                     test_data_free);
+            g_free(path);
         } else {
             path = g_strdup_printf("cpu/%s/add/%ux%ux%u&maxcpus=%u",
                                    mname, data->sockets, data->cores,
                                    data->threads, data->maxcpus);
-            qtest_add_data_func(path, data, test_pc_with_cpu_add);
+            qtest_add_data_func_full(path, data, test_pc_with_cpu_add,
+                                     test_data_free);
+            g_free(path);
         }
     }
+    QDECREF(response);
     qtest_end();
 }
 
-- 
2.10.0

[Qemu-devel] [PULL 25/26] tests: fix rsp leak in postcopy-test

[Marc-André Lureau]

In all cases, even when the dict doesn't contain 'ram', the qmp response
must be unref.

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

diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
index 229e9e9..bf4e579 100644
--- a/tests/postcopy-test.c
+++ b/tests/postcopy-test.c
@@ -260,8 +260,8 @@ static uint64_t get_migration_pass(void)
     } else {
         rsp_ram = qdict_get_qdict(rsp_return, "ram");
         result = qdict_get_try_int(rsp_ram, "dirty-sync-count", 0);
-        QDECREF(rsp);
     }
+    QDECREF(rsp);
     return result;
 }
 
-- 
2.10.0

[Qemu-devel] [PULL 26/26] tests: fix postcopy-test leaks

[Marc-André Lureau]

A few strings are allocated and never freed.

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

diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
index bf4e579..41ed1a9 100644
--- a/tests/postcopy-test.c
+++ b/tests/postcopy-test.c
@@ -176,6 +176,7 @@ static void wait_for_serial(const char *side)
     int started = (strcmp(side, "src_serial") == 0 &&
                    strcmp(arch, "ppc64") == 0) ? 0 : 1;
 
+    g_free(serialpath);
     do {
         int readvalue = fgetc(serialfile);
 
@@ -203,7 +204,6 @@ static void wait_for_serial(const char *side)
         case 'B':
             /* It's alive! */
             fclose(serialfile);
-            g_free(serialpath);
             return;
 
         case EOF:
@@ -350,6 +350,7 @@ static void cleanup(const char *filename)
     char *path = g_strdup_printf("%s/%s", tmpfs, filename);
 
     unlink(path);
+    g_free(path);
 }
 
 static void test_migrate(void)
@@ -394,6 +395,8 @@ static void test_migrate(void)
         g_assert_not_reached();
     }
 
+    g_free(bootpath);
+
     from = qtest_start(cmd_src);
     g_free(cmd_src);
 
-- 
2.10.0

Re: [Qemu-devel] [PULL 10/26] tests: fix small leak in test-io-channel-command

[Daniel P. Berrange]

On Tue, Sep 06, 2016 at 04:26:23PM +0400, Marc-André Lureau wrote:
> srcfifo && dstfifo must be freed in error case, however unlink() may
> delete a file from a different context. Instead, use mkdtemp()/rmdir()
> for the temporary files.

This isn't making much sense to me. What different context are
you referring to ?  The fifo filename is unique to this particular
unit test and AFAIK, we don't support running the same unit test
multiple times concurrently, so the existing unlink looks safe to
me.

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/test-io-channel-command.c | 20 +++++++++++++-------
>  tests/test-qga.c                |  3 ++-
>  2 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/test-io-channel-command.c b/tests/test-io-channel-command.c
> index 1d1f461..f99118e 100644
> --- a/tests/test-io-channel-command.c
> +++ b/tests/test-io-channel-command.c
> @@ -18,6 +18,7 @@
>   *
>   */
>  
> +#include <glib/gstdio.h>
>
>  #include "qemu/osdep.h"
>  #include "io/channel-command.h"
>  #include "io-channel-helpers.h"
> @@ -26,11 +27,14 @@
>  #ifndef WIN32
>  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);
> +    char *tmpdir = g_strdup("/tmp/test-io-channel.XXXXXX");
> +    g_assert_nonnull(mkdtemp(tmpdir));
> +
> +    char *fifo = g_strdup_printf("%s/command.fifo", tmpdir);
> +    char *srcfifo = g_strdup_printf("PIPE:%s,wronly", fifo);
> +    char *dstfifo = g_strdup_printf("PIPE:%s,rdonly", fifo);
>      const char *srcargv[] = {
>          "/bin/socat", "-", srcfifo, NULL,
>      };
> @@ -38,11 +42,10 @@ static void test_io_channel_command_fifo(bool async)
>          "/bin/socat", dstfifo, "-", NULL,
>      };
>  
> -    unlink(TEST_FIFO);
>      if (access("/bin/socat", X_OK) < 0) {
> -        return; /* Pretend success if socat is not present */
> +        goto end; /* Pretend success if socat is not present */
>      }
> -    if (mkfifo(TEST_FIFO, 0600) < 0) {
> +    if (mkfifo(fifo, 0600) < 0) {
>          abort();
>      }
>      src = QIO_CHANNEL(qio_channel_command_new_spawn(srcargv,
> @@ -59,9 +62,12 @@ static void test_io_channel_command_fifo(bool async)
>      object_unref(OBJECT(src));
>      object_unref(OBJECT(dst));
>  
> +end:
> +    g_free(fifo);
>      g_free(srcfifo);
>      g_free(dstfifo);
> -    unlink(TEST_FIFO);
> +    g_rmdir(tmpdir);
> +    g_free(tmpdir);
>  }
>  
>  
> diff --git a/tests/test-qga.c b/tests/test-qga.c
> index 21f44f8..0d1acef 100644
> --- a/tests/test-qga.c
> +++ b/tests/test-qga.c
> @@ -55,7 +55,8 @@ fixture_setup(TestFixture *fixture, gconstpointer data)
>      fixture->loop = g_main_loop_new(NULL, FALSE);
>  
>      fixture->test_dir = g_strdup("/tmp/qgatest.XXXXXX");
> -    g_assert_nonnull(mkdtemp(fixture->test_dir));
> +    path = mkdtemp(fixture->test_dir);
> +    g_assert_nonnull(path);
>  
>      path = g_build_filename(fixture->test_dir, "sock", NULL);
>      cwd = g_get_current_dir();
> -- 
> 2.10.0
> 
> 

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PULL 10/26] tests: fix small leak in test-io-channel-command

[Marc-André Lureau]

Hi

----- Original Message -----
> On Tue, Sep 06, 2016 at 04:26:23PM +0400, Marc-André Lureau wrote:
> > srcfifo && dstfifo must be freed in error case, however unlink() may
> > delete a file from a different context. Instead, use mkdtemp()/rmdir()
> > for the temporary files.
> 
> This isn't making much sense to me. What different context are
> you referring to ?  The fifo filename is unique to this particular
> unit test and AFAIK, we don't support running the same unit test
> multiple times concurrently, so the existing unlink looks safe to
> me.

This was changed to address Eric's comment: https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg04554.html

Is it a problem to use temporary dir instead so tests could actually run concurrently?

Anything wrong with this patch?

> 
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > ---
> >  tests/test-io-channel-command.c | 20 +++++++++++++-------
> >  tests/test-qga.c                |  3 ++-
> >  2 files changed, 15 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tests/test-io-channel-command.c
> > b/tests/test-io-channel-command.c
> > index 1d1f461..f99118e 100644
> > --- a/tests/test-io-channel-command.c
> > +++ b/tests/test-io-channel-command.c
> > @@ -18,6 +18,7 @@
> >   *
> >   */
> >  
> > +#include <glib/gstdio.h>
> >
> >  #include "qemu/osdep.h"
> >  #include "io/channel-command.h"
> >  #include "io-channel-helpers.h"
> > @@ -26,11 +27,14 @@
> >  #ifndef WIN32
> >  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);
> > +    char *tmpdir = g_strdup("/tmp/test-io-channel.XXXXXX");
> > +    g_assert_nonnull(mkdtemp(tmpdir));
> > +
> > +    char *fifo = g_strdup_printf("%s/command.fifo", tmpdir);
> > +    char *srcfifo = g_strdup_printf("PIPE:%s,wronly", fifo);
> > +    char *dstfifo = g_strdup_printf("PIPE:%s,rdonly", fifo);
> >      const char *srcargv[] = {
> >          "/bin/socat", "-", srcfifo, NULL,
> >      };
> > @@ -38,11 +42,10 @@ static void test_io_channel_command_fifo(bool async)
> >          "/bin/socat", dstfifo, "-", NULL,
> >      };
> >  
> > -    unlink(TEST_FIFO);
> >      if (access("/bin/socat", X_OK) < 0) {
> > -        return; /* Pretend success if socat is not present */
> > +        goto end; /* Pretend success if socat is not present */
> >      }
> > -    if (mkfifo(TEST_FIFO, 0600) < 0) {
> > +    if (mkfifo(fifo, 0600) < 0) {
> >          abort();
> >      }
> >      src = QIO_CHANNEL(qio_channel_command_new_spawn(srcargv,
> > @@ -59,9 +62,12 @@ static void test_io_channel_command_fifo(bool async)
> >      object_unref(OBJECT(src));
> >      object_unref(OBJECT(dst));
> >  
> > +end:
> > +    g_free(fifo);
> >      g_free(srcfifo);
> >      g_free(dstfifo);
> > -    unlink(TEST_FIFO);
> > +    g_rmdir(tmpdir);
> > +    g_free(tmpdir);
> >  }
> >  
> >  
> > diff --git a/tests/test-qga.c b/tests/test-qga.c
> > index 21f44f8..0d1acef 100644
> > --- a/tests/test-qga.c
> > +++ b/tests/test-qga.c
> > @@ -55,7 +55,8 @@ fixture_setup(TestFixture *fixture, gconstpointer data)
> >      fixture->loop = g_main_loop_new(NULL, FALSE);
> >  
> >      fixture->test_dir = g_strdup("/tmp/qgatest.XXXXXX");
> > -    g_assert_nonnull(mkdtemp(fixture->test_dir));
> > +    path = mkdtemp(fixture->test_dir);
> > +    g_assert_nonnull(path);
> >  
> >      path = g_build_filename(fixture->test_dir, "sock", NULL);
> >      cwd = g_get_current_dir();
> > --
> > 2.10.0
> > 
> > 
> 
> Regards,
> Daniel
> --
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
> 

Re: [Qemu-devel] [PULL 10/26] tests: fix small leak in test-io-channel-command

[Daniel P. Berrange]

On Tue, Sep 06, 2016 at 08:53:33AM -0400, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > On Tue, Sep 06, 2016 at 04:26:23PM +0400, Marc-André Lureau wrote:
> > > srcfifo && dstfifo must be freed in error case, however unlink() may
> > > delete a file from a different context. Instead, use mkdtemp()/rmdir()
> > > for the temporary files.
> > 
> > This isn't making much sense to me. What different context are
> > you referring to ?  The fifo filename is unique to this particular
> > unit test and AFAIK, we don't support running the same unit test
> > multiple times concurrently, so the existing unlink looks safe to
> > me.
> 
> This was changed to address Eric's comment: https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg04554.html

That could have been trivially fixed by reordering the cleanup code

eg

    unlink(TEST_FIFO);

  end:
    g_free(srcfifo);
    g_free(dstfifo);
 }

> 
> Is it a problem to use temporary dir instead so tests could actually
> run concurrently?

I've always written tests on the assumption that distinct unit tests
can run concurrently, but you'll never have the same unit test run
concurrently. IMHO running the same unit test concurrently is a
non-goal.

> Anything wrong with this patch?

It needlessly adds extra code complexity to solve a non-goal IMHO.

> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > Reviewed-by: Eric Blake <eblake@redhat.com>
> > > ---
> > >  tests/test-io-channel-command.c | 20 +++++++++++++-------
> > >  tests/test-qga.c                |  3 ++-
> > >  2 files changed, 15 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/tests/test-io-channel-command.c
> > > b/tests/test-io-channel-command.c
> > > index 1d1f461..f99118e 100644
> > > --- a/tests/test-io-channel-command.c
> > > +++ b/tests/test-io-channel-command.c
> > > @@ -18,6 +18,7 @@
> > >   *
> > >   */
> > >  
> > > +#include <glib/gstdio.h>
> > >
> > >  #include "qemu/osdep.h"
> > >  #include "io/channel-command.h"
> > >  #include "io-channel-helpers.h"
> > > @@ -26,11 +27,14 @@
> > >  #ifndef WIN32
> > >  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);
> > > +    char *tmpdir = g_strdup("/tmp/test-io-channel.XXXXXX");
> > > +    g_assert_nonnull(mkdtemp(tmpdir));
> > > +
> > > +    char *fifo = g_strdup_printf("%s/command.fifo", tmpdir);
> > > +    char *srcfifo = g_strdup_printf("PIPE:%s,wronly", fifo);
> > > +    char *dstfifo = g_strdup_printf("PIPE:%s,rdonly", fifo);
> > >      const char *srcargv[] = {
> > >          "/bin/socat", "-", srcfifo, NULL,
> > >      };
> > > @@ -38,11 +42,10 @@ static void test_io_channel_command_fifo(bool async)
> > >          "/bin/socat", dstfifo, "-", NULL,
> > >      };
> > >  
> > > -    unlink(TEST_FIFO);
> > >      if (access("/bin/socat", X_OK) < 0) {
> > > -        return; /* Pretend success if socat is not present */
> > > +        goto end; /* Pretend success if socat is not present */
> > >      }
> > > -    if (mkfifo(TEST_FIFO, 0600) < 0) {
> > > +    if (mkfifo(fifo, 0600) < 0) {
> > >          abort();
> > >      }
> > >      src = QIO_CHANNEL(qio_channel_command_new_spawn(srcargv,
> > > @@ -59,9 +62,12 @@ static void test_io_channel_command_fifo(bool async)
> > >      object_unref(OBJECT(src));
> > >      object_unref(OBJECT(dst));
> > >  
> > > +end:
> > > +    g_free(fifo);
> > >      g_free(srcfifo);
> > >      g_free(dstfifo);
> > > -    unlink(TEST_FIFO);
> > > +    g_rmdir(tmpdir);
> > > +    g_free(tmpdir);
> > >  }
> > >  
> > >  
> > > diff --git a/tests/test-qga.c b/tests/test-qga.c
> > > index 21f44f8..0d1acef 100644
> > > --- a/tests/test-qga.c
> > > +++ b/tests/test-qga.c
> > > @@ -55,7 +55,8 @@ fixture_setup(TestFixture *fixture, gconstpointer data)
> > >      fixture->loop = g_main_loop_new(NULL, FALSE);
> > >  
> > >      fixture->test_dir = g_strdup("/tmp/qgatest.XXXXXX");
> > > -    g_assert_nonnull(mkdtemp(fixture->test_dir));
> > > +    path = mkdtemp(fixture->test_dir);
> > > +    g_assert_nonnull(path);
> > >  
> > >      path = g_build_filename(fixture->test_dir, "sock", NULL);
> > >      cwd = g_get_current_dir();


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PULL 10/26] tests: fix small leak in test-io-channel-command

[Marc-André Lureau]

Hi

----- Original Message -----
> Hi
> 
> ----- Original Message -----
> > On Tue, Sep 06, 2016 at 04:26:23PM +0400, Marc-André Lureau wrote:
> > > srcfifo && dstfifo must be freed in error case, however unlink() may
> > > delete a file from a different context. Instead, use mkdtemp()/rmdir()
> > > for the temporary files.
> > 
> > This isn't making much sense to me. What different context are
> > you referring to ?  The fifo filename is unique to this particular
> > unit test and AFAIK, we don't support running the same unit test
> > multiple times concurrently, so the existing unlink looks safe to
> > me.
> 
> This was changed to address Eric's comment:
> https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg04554.html
> 
> Is it a problem to use temporary dir instead so tests could actually run
> concurrently?
> 
> Anything wrong with this patch?

Actually, the patch leaks /tmp/test-io-channel.XXXXXX dirs now, oops.

At least, there is a missing unlink for the fifo.
Anything else?

> 
> > 
> > > 
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > Reviewed-by: Eric Blake <eblake@redhat.com>
> > > ---
> > >  tests/test-io-channel-command.c | 20 +++++++++++++-------
> > >  tests/test-qga.c                |  3 ++-
> > >  2 files changed, 15 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/tests/test-io-channel-command.c
> > > b/tests/test-io-channel-command.c
> > > index 1d1f461..f99118e 100644
> > > --- a/tests/test-io-channel-command.c
> > > +++ b/tests/test-io-channel-command.c
> > > @@ -18,6 +18,7 @@
> > >   *
> > >   */
> > >  
> > > +#include <glib/gstdio.h>
> > >
> > >  #include "qemu/osdep.h"
> > >  #include "io/channel-command.h"
> > >  #include "io-channel-helpers.h"
> > > @@ -26,11 +27,14 @@
> > >  #ifndef WIN32
> > >  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);
> > > +    char *tmpdir = g_strdup("/tmp/test-io-channel.XXXXXX");
> > > +    g_assert_nonnull(mkdtemp(tmpdir));
> > > +
> > > +    char *fifo = g_strdup_printf("%s/command.fifo", tmpdir);
> > > +    char *srcfifo = g_strdup_printf("PIPE:%s,wronly", fifo);
> > > +    char *dstfifo = g_strdup_printf("PIPE:%s,rdonly", fifo);
> > >      const char *srcargv[] = {
> > >          "/bin/socat", "-", srcfifo, NULL,
> > >      };
> > > @@ -38,11 +42,10 @@ static void test_io_channel_command_fifo(bool async)
> > >          "/bin/socat", dstfifo, "-", NULL,
> > >      };
> > >  
> > > -    unlink(TEST_FIFO);
> > >      if (access("/bin/socat", X_OK) < 0) {
> > > -        return; /* Pretend success if socat is not present */
> > > +        goto end; /* Pretend success if socat is not present */
> > >      }
> > > -    if (mkfifo(TEST_FIFO, 0600) < 0) {
> > > +    if (mkfifo(fifo, 0600) < 0) {
> > >          abort();
> > >      }
> > >      src = QIO_CHANNEL(qio_channel_command_new_spawn(srcargv,
> > > @@ -59,9 +62,12 @@ static void test_io_channel_command_fifo(bool async)
> > >      object_unref(OBJECT(src));
> > >      object_unref(OBJECT(dst));
> > >  
> > > +end:
> > > +    g_free(fifo);
> > >      g_free(srcfifo);
> > >      g_free(dstfifo);
> > > -    unlink(TEST_FIFO);
> > > +    g_rmdir(tmpdir);
> > > +    g_free(tmpdir);
> > >  }
> > >  
> > >  
> > > diff --git a/tests/test-qga.c b/tests/test-qga.c
> > > index 21f44f8..0d1acef 100644
> > > --- a/tests/test-qga.c
> > > +++ b/tests/test-qga.c
> > > @@ -55,7 +55,8 @@ fixture_setup(TestFixture *fixture, gconstpointer data)
> > >      fixture->loop = g_main_loop_new(NULL, FALSE);
> > >  
> > >      fixture->test_dir = g_strdup("/tmp/qgatest.XXXXXX");
> > > -    g_assert_nonnull(mkdtemp(fixture->test_dir));
> > > +    path = mkdtemp(fixture->test_dir);
> > > +    g_assert_nonnull(path);
> > >  
> > >      path = g_build_filename(fixture->test_dir, "sock", NULL);
> > >      cwd = g_get_current_dir();
> > > --
> > > 2.10.0
> > > 
> > > 
> > 
> > Regards,
> > Daniel
> > --
> > |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/
> > |:|
> > |: http://libvirt.org              -o-             http://virt-manager.org
> > |:|
> > |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/
> > |:|
> > |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc
> > |:|
> > 
> 

Re: [Qemu-devel] [PULL 00/26] Leak patches

[Peter Maydell]

On 6 September 2016 at 13:26, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
> The following changes since commit 1fd66154fdf8305e6668a96046a22b863b4d7320:
>
>   Merge remote-tracking branch 'remotes/sstabellini/tags/xen-20160905' into staging (2016-09-06 11:43:18 +0100)
>
> are available in the git repository at:
>
>   git@github.com:elmarco/qemu.git tags/leak-pull-request
>
> for you to fetch changes up to 58d104a7f2698ffff3ee5976f96ebcff111be63b:
>
>   tests: fix postcopy-test leaks (2016-09-06 16:20:27 +0400)
>
> ----------------------------------------------------------------

I'm afraid this doesn't build with our minimum glib version:

/Users/pm215/src/qemu-for-merges/tests/libqtest.c:771:42: error: expected ')'
                      (GTestFixtureFunc) fn, (GTestFixtureFunc) data_free_func);
                                         ^
/Users/pm215/src/qemu-for-merges/tests/libqtest.c:770:22: note: to
match this '('
    g_test_add_vtable(path, 0, data, NULL,
                     ^
1 error generated.

The GTestFixtureFunc typedef was only introduced in glib 2.28, and our
minimum is 2.22.

Also, g_test_add_vtable() in glib 2.22 has this prototype:

void    g_test_add_vtable               (const char     *testpath,
                                         gsize           data_size,
                                         gconstpointer   test_data,
                                         void          (*data_setup)    (void),
                                         void          (*data_test)     (void),
                                         void          (*data_teardown) (void));

but GTestFixtureFunc is typedefed in newer glib as
void (*GTestFixtureFunc) (gpointer fixture, gconstpointer user_data);

so it looks like this function has changed signature somewhere
between glib versions, which makes me a bit nervous about using it.

thanks
-- PMM

Re: [Qemu-devel] [PULL 00/26] Leak patches

[Marc-André Lureau]

Hi

----- Original Message -----
> On 6 September 2016 at 13:26, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> > The following changes since commit
> > 1fd66154fdf8305e6668a96046a22b863b4d7320:
> >
> >   Merge remote-tracking branch 'remotes/sstabellini/tags/xen-20160905' into
> >   staging (2016-09-06 11:43:18 +0100)
> >
> > are available in the git repository at:
> >
> >   git@github.com:elmarco/qemu.git tags/leak-pull-request
> >
> > for you to fetch changes up to 58d104a7f2698ffff3ee5976f96ebcff111be63b:
> >
> >   tests: fix postcopy-test leaks (2016-09-06 16:20:27 +0400)
> >
> > ----------------------------------------------------------------
> 
> I'm afraid this doesn't build with our minimum glib version:
> 
> /Users/pm215/src/qemu-for-merges/tests/libqtest.c:771:42: error: expected ')'
>                       (GTestFixtureFunc) fn, (GTestFixtureFunc)
>                       data_free_func);
>                                          ^
> /Users/pm215/src/qemu-for-merges/tests/libqtest.c:770:22: note: to
> match this '('
>     g_test_add_vtable(path, 0, data, NULL,
>                      ^
> 1 error generated.
> 
> The GTestFixtureFunc typedef was only introduced in glib 2.28, and our
> minimum is 2.22.

Argh,..

> 
> Also, g_test_add_vtable() in glib 2.22 has this prototype:
> 
> void    g_test_add_vtable               (const char     *testpath,
>                                          gsize           data_size,
>                                          gconstpointer   test_data,
>                                          void          (*data_setup)
>                                          (void),
>                                          void          (*data_test)
>                                          (void),
>                                          void          (*data_teardown)
>                                          (void));
> 
> but GTestFixtureFunc is typedefed in newer glib as
> void (*GTestFixtureFunc) (gpointer fixture, gconstpointer user_data);
> 
> so it looks like this function has changed signature somewhere
> between glib versions, which makes me a bit nervous about using it.

Perhaps we should get back to the simpler version, only using g_test_add_data_func_full() with 2.34:
https://patchwork.kernel.org/patch/9251373/

I can update the patch that way with a comment about expected leaks < 2.34.

there is no discussions about bumping the glib requirements btw?

Re: [Qemu-devel] [PULL 00/26] Leak patches

[Peter Maydell]

On 6 September 2016 at 16:33, Marc-André Lureau <mlureau@redhat.com> wrote:
> there is no discussions about bumping the glib requirements btw?

They're set so we can continue to support a SUSE LTS distro
version.

thanks
-- PMM

Re: [Qemu-devel] [PULL 00/26] Leak patches

[Marc-André Lureau]

Hi

----- Original Message -----
> Hi
> 
> ----- Original Message -----
> > On 6 September 2016 at 13:26, Marc-André Lureau
> > <marcandre.lureau@redhat.com> wrote:
> > > The following changes since commit
> > > 1fd66154fdf8305e6668a96046a22b863b4d7320:
> > >
> > >   Merge remote-tracking branch 'remotes/sstabellini/tags/xen-20160905'
> > >   into
> > >   staging (2016-09-06 11:43:18 +0100)
> > >
> > > are available in the git repository at:
> > >
> > >   git@github.com:elmarco/qemu.git tags/leak-pull-request
> > >
> > > for you to fetch changes up to 58d104a7f2698ffff3ee5976f96ebcff111be63b:
> > >
> > >   tests: fix postcopy-test leaks (2016-09-06 16:20:27 +0400)
> > >
> > > ----------------------------------------------------------------
> > 
> > I'm afraid this doesn't build with our minimum glib version:
> > 
> > /Users/pm215/src/qemu-for-merges/tests/libqtest.c:771:42: error: expected
> > ')'
> >                       (GTestFixtureFunc) fn, (GTestFixtureFunc)
> >                       data_free_func);
> >                                          ^
> > /Users/pm215/src/qemu-for-merges/tests/libqtest.c:770:22: note: to
> > match this '('
> >     g_test_add_vtable(path, 0, data, NULL,
> >                      ^
> > 1 error generated.
> > 
> > The GTestFixtureFunc typedef was only introduced in glib 2.28, and our
> > minimum is 2.22.
> 
> Argh,..
> 
> > 
> > Also, g_test_add_vtable() in glib 2.22 has this prototype:
> > 
> > void    g_test_add_vtable               (const char     *testpath,
> >                                          gsize           data_size,
> >                                          gconstpointer   test_data,
> >                                          void          (*data_setup)
> >                                          (void),
> >                                          void          (*data_test)
> >                                          (void),
> >                                          void          (*data_teardown)
> >                                          (void));
> > 
> > but GTestFixtureFunc is typedefed in newer glib as
> > void (*GTestFixtureFunc) (gpointer fixture, gconstpointer user_data);
> > 
> > so it looks like this function has changed signature somewhere
> > between glib versions, which makes me a bit nervous about using it.
> 
> Perhaps we should get back to the simpler version, only using
> g_test_add_data_func_full() with 2.34:
> https://patchwork.kernel.org/patch/9251373/
> 
> I can update the patch that way with a comment about expected leaks < 2.34.
> 

Eric, since you suggested some compat code for older versions, would you be fine with the above plan? I can send a new patch for review.

Re: [Qemu-devel] [PULL 00/26] Leak patches

[Eric Blake]

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

On 09/07/2016 01:57 PM, Marc-André Lureau wrote:

>>> I'm afraid this doesn't build with our minimum glib version:
>>>
>>> /Users/pm215/src/qemu-for-merges/tests/libqtest.c:771:42: error: expected
>>> ')'
>>>                       (GTestFixtureFunc) fn, (GTestFixtureFunc)
>>>                       data_free_func);

>>> The GTestFixtureFunc typedef was only introduced in glib 2.28, and our
>>> minimum is 2.22.
>>
>> Argh,..
>>
>>>
>>> Also, g_test_add_vtable() in glib 2.22 has this prototype:
>>>

>>>                                          void          (*data_test)
>>>                                          (void),
>>>                                          void          (*data_teardown)
>>>                                          (void));
>>>
>>> but GTestFixtureFunc is typedefed in newer glib as
>>> void (*GTestFixtureFunc) (gpointer fixture, gconstpointer user_data);

So when glib prototyped it, they changed from (void) to (gpointer,
gconstpointer) as the signature.  Which means there's no real portable
way to cast, other than to create a conditional #define typedef to the
appropriate variant for the glib being targeted.

>>>
>>> so it looks like this function has changed signature somewhere
>>> between glib versions, which makes me a bit nervous about using it.

Do you use either 'fixture' or 'user_data' in the updated signature?  If
not, and the older signature of (void) matches how we are using things,
then the casting just exists to shut up the compiler (and we are relying
on the ABI, rather than C, to happen to do the right thing).  If we ARE
using either argument, then you'd have to refactor things to pass the
arguments through a global rather than as a function parameter.

>>
>> Perhaps we should get back to the simpler version, only using
>> g_test_add_data_func_full() with 2.34:
>> https://patchwork.kernel.org/patch/9251373/
>>
>> I can update the patch that way with a comment about expected leaks < 2.34.

Living with leaks on older glib is starting to sound the most palatable;
but it needs more comments than what was listed in that patchwork entry.

>>
> 
> Eric, since you suggested some compat code for older versions, would you be fine with the above plan? I can send a new patch for review.

I'm fine with the #if GLIB_CHECK_VERSION(2, 34, 0), but only if there is
corresponding comments in both the code and the commit message that
documents that we have exhausted all other reasonable possibilities of
catering to older glib, and that it doesn't matter if the test leaks.

-- 
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 --]