[Qemu-devel] [PATCH for-2.7 v3 00/36] Various memory leak fixes

[Qemu-devel] [PATCH for-2.7 v3 00/36] Various memory leak fixes

[marcandre.lureau]

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

Hi,

Thanks to AddressSanitizer (ASAN), I found a number of direct leaks
worth fixing. Note that there are probably many indirect leaks left (I
am adding some here), I haven't investigated much yet.

There are still a number of direct leaks remaining, in particular in
the tests, but my libc doesn't give me good backtraces.

In order to easily switch to asan-enabled build, I make use of make
CFLAGS argument, which is why the first patch is also there.

v3:
- add glib<2.34 fallback for g_test_add_data_func_full() using
  g_test_add_vtable() with an explicit comment
- add and use a tcp_chr_free_connection(), from tcp_chr_disconnect()
  (suggested by Paolo)
- remove bad comment from "free MuxDriver" patch (Eric)
- replace GFunc casts with a qemu_g_func_free() inline (suggested by Eric)
- replace CFLAGS override patch with a toplevel CFLAGS filter
  (suggested by Paolo)
- drop a patch already merged upstream
- update reviewed-by tags (28/36)

v2:
- split "pci-bus: do not allocate and leak bsel" as a seperate patch,
  since the direct leak looks like a bug
- hook virtio_input_finalize to the abstract base class
- use mkdtemp in test-io-channel-command
- fix pc-cpu-test commit message
- move the sglist destruction inside of ncq_err, call dma_buf_commit
  in ide_dma_cb, as suggested by John Snow
- fix build with glib 2.22
- add the given reviewed-by tags (22/37, 15 left)

Marc-André Lureau (36):
  build-sys: fix building with make CFLAGS=.. argument
  tests: fix test-qga leaks
  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
  qdist: fix entries memory leak
  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
  numa: do not leak NumaOptions
  pc: simplify passing qemu_irq
  pc: don't leak a20_line
  machine: use class base init generated name
  acpi-build: fix array leak
  char: free the tcp connection data when closing
  char: free MuxDriver when closing
  tests: fix qom-test leaks
  pc: free i8259
  pc: keep gsi reference
  ahci: free irqs array
  sd: free timer
  qjson: free str
  virtio-input: free config list
  ipmi: free extern timer
  usb: free USBDevice.strings
  usb: free leaking path
  bus: simplify name handling
  tests: add qtest_add_data_func_full
  tests: pc-cpu-test leaks fixes
  tests: fix rsp leak in postcopy-test
  ahci: fix sglist leak on retry
  tests: fix postcopy-test leaks

 Makefile                          |  3 ++-
 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/ahci.c                     |  4 ++--
 hw/ide/core.c                     |  7 +++++--
 hw/input/pckbd.c                  |  4 ++--
 hw/input/virtio-input.c           | 11 +++++++++++
 hw/ipmi/ipmi_bmc_extern.c         |  9 +++++++++
 hw/isa/isa-bus.c                  | 14 +++++---------
 hw/sd/sd.c                        |  9 +++++++++
 hw/usb/bus.c                      |  7 +++++++
 hw/usb/desc.c                     |  1 +
 include/glib-compat.h             |  9 +++++++++
 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 +-
 migration/qjson.c                 |  1 +
 numa.c                            | 15 ++++++++-------
 pc-bios/optionrom/Makefile        |  2 --
 qemu-char.c                       | 34 ++++++++++++++++++++++++++++------
 qga/guest-agent-command-state.c   |  7 +++++++
 qga/guest-agent-core.h            |  1 +
 qga/main.c                        | 14 ++++++++------
 tests/check-qom-interface.c       |  1 +
 tests/check-qom-proplist.c        | 16 ++++++++++++++++
 tests/libqtest.c                  | 15 +++++++++++++++
 tests/libqtest.h                  |  7 ++++++-
 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 ++++++--
 util/qdist.c                      |  3 ++-
 50 files changed, 298 insertions(+), 118 deletions(-)

-- 
2.9.0

[Qemu-devel] [PATCH for-2.7 v3 01/36] build-sys: fix building with make CFLAGS=.. argument

[marcandre.lureau]

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

When calling make with a CFLAGS=.. argument, the -g/-O filter is not
applied, which may result with build failure with ASAN for example. It
could be solved with an 'override' directive on CFLAGS, but that would
actually prevent setting different CFLAGS manually.

Instead, filter the CFLAGS argument from the top-level Makefile (so
you could still call make with a different CFLAGS argument on a
rom/Makefile manually)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 Makefile                   | 3 ++-
 pc-bios/optionrom/Makefile | 2 --
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 0d7647f..50b4b3a 100644
--- a/Makefile
+++ b/Makefile
@@ -225,8 +225,9 @@ dtc/%:
 $(SUBDIR_RULES): libqemuutil.a libqemustub.a $(common-obj-y) $(qom-obj-y) $(crypto-aes-obj-$(CONFIG_USER_ONLY))
 
 ROMSUBDIR_RULES=$(patsubst %,romsubdir-%, $(ROMS))
+# Only keep -O and -g cflags
 romsubdir-%:
-	$(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C pc-bios/$* V="$(V)" TARGET_DIR="$*/",)
+	$(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C pc-bios/$* V="$(V)" TARGET_DIR="$*/" CFLAGS="$(filter -O% -g%,$(CFLAGS))",)
 
 ALL_SUBDIRS=$(TARGET_DIRS) $(patsubst %,pc-bios/%, $(ROMS))
 
diff --git a/pc-bios/optionrom/Makefile b/pc-bios/optionrom/Makefile
index 24e175e..6bab490 100644
--- a/pc-bios/optionrom/Makefile
+++ b/pc-bios/optionrom/Makefile
@@ -24,8 +24,6 @@ QEMU_CFLAGS += $(call cc-option, $(QEMU_CFLAGS), -no-integrated-as)
 QEMU_CFLAGS += -m32 -include $(SRC_PATH)/pc-bios/optionrom/code16gcc.h
 endif
 
-# Drop gcov and glib flags
-CFLAGS := $(filter -O% -g%, $(CFLAGS))
 QEMU_INCLUDES += -I$(SRC_PATH)
 
 Wa = -Wa,
-- 
2.9.0

[Qemu-devel] [PATCH for-2.7 v3 02/36] tests: fix test-qga leaks

[marcandre.lureau]

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

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.9.0

[Qemu-devel] [PATCH for-2.7 v3 03/36] qga: free the whole blacklist

[marcandre.lureau]

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

Free the list, not just the elements.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/glib-compat.h | 9 +++++++++
 qga/main.c            | 8 ++------
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/include/glib-compat.h b/include/glib-compat.h
index 01aa7b3..6d643b2 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -260,4 +260,13 @@ static inline void g_hash_table_add(GHashTable *hash_table, gpointer key)
     } while (0)
 #endif
 
+/*
+ * A GFunc function helper freeing the first argument (not part of glib)
+ */
+static inline void qemu_g_func_free(gpointer data,
+                                    gpointer user_data)
+{
+    g_free(data);
+}
+
 #endif
diff --git a/qga/main.c b/qga/main.c
index 4c3b2c7..868508b 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -1175,6 +1175,8 @@ static void config_free(GAConfig *config)
 #ifdef CONFIG_FSFREEZE
     g_free(config->fsfreeze_hook);
 #endif
+    g_list_foreach(config->blacklist, qemu_g_func_free, NULL);
+    g_list_free(config->blacklist);
     g_free(config);
 }
 
@@ -1310,11 +1312,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 +1376,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.9.0

[Qemu-devel] [PATCH for-2.7 v3 04/36] qga: free remaining leaking state

[marcandre.lureau]

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

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

diff --git a/qga/guest-agent-command-state.c b/qga/guest-agent-command-state.c
index 4de229c..31c6368 100644
--- a/qga/guest-agent-command-state.c
+++ b/qga/guest-agent-command-state.c
@@ -71,3 +71,10 @@ GACommandState *ga_command_state_new(void)
     cs->groups = NULL;
     return cs;
 }
+
+void ga_command_state_free(GACommandState *cs)
+{
+    g_slist_foreach(cs->groups, qemu_g_func_free, NULL);
+    g_slist_free(cs->groups);
+    g_free(cs);
+}
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);
diff --git a/qga/main.c b/qga/main.c
index 868508b..0038702 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -1372,6 +1372,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);
@@ -1384,6 +1386,10 @@ end:
     }
 
     config_free(config);
+    if (s->main_loop) {
+        g_main_loop_unref(s->main_loop);
+    }
+    g_free(s);
 
     return ret;
 }
-- 
2.9.0

[Qemu-devel] [PATCH for-2.7 v3 05/36] tests: fix test-cutils leaks

[marcandre.lureau]

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

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.9.0

[Qemu-devel] [PATCH for-2.7 v3 06/36] tests: fix test-vmstate leaks

[marcandre.lureau]

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

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.9.0

[Qemu-devel] [PATCH for-2.7 v3 07/36] tests: fix test-iov leaks

[marcandre.lureau]

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

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.9.0

[Qemu-devel] [PATCH for-2.7 v3 08/36] qdist: fix entries memory leak

[marcandre.lureau]

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

qdist_init() allocates entries, make sure we don't leak it.

Spotted thanks to ASAN.

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

diff --git a/util/qdist.c b/util/qdist.c
index 56f5738..e94cf46 100644
--- a/util/qdist.c
+++ b/util/qdist.c
@@ -188,7 +188,8 @@ void qdist_bin__internal(struct qdist *to, const struct qdist *from, size_t n)
             }
         }
         /* they're equally spaced, so copy the dist and bail out */
-        to->entries = g_new(struct qdist_entry, from->n);
+        to->entries = g_realloc_n(to->entries, from->n,
+                                  sizeof(struct qdist_entry));
         to->n = from->n;
         memcpy(to->entries, from->entries, sizeof(*to->entries) * to->n);
         return;
-- 
2.9.0

[Qemu-devel] [PATCH for-2.7 v3 09/36] tests: fix check-qom-interface leaks

[marcandre.lureau]

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

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.9.0

[Qemu-devel] [PATCH for-2.7 v3 10/36] tests: fix check-qom-proplist leaks

[marcandre.lureau]

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

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.9.0

[Qemu-devel] [PATCH for-2.7 v3 11/36] tests: fix small leak in test-io-channel-command

[marcandre.lureau]

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

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.9.0

[Qemu-devel] [PATCH for-2.7 v3 12/36] tests: fix leak in test-string-input-visitor

[marcandre.lureau]

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

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.9.0

[Qemu-devel] [PATCH for-2.7 v3 13/36] portio: keep references on portio

[marcandre.lureau]

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

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 d117b7c..f9c8162 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2618,10 +2618,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.9.0

[Qemu-devel] [PATCH for-2.7 v3 14/36] numa: do not leak NumaOptions

[marcandre.lureau]

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

In all cases, call qapi_free_NumaOptions(), by using a common ending
block.

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

diff --git a/numa.c b/numa.c
index cbae430..3be3b26 100644
--- a/numa.c
+++ b/numa.c
@@ -223,14 +223,14 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
     }
 
     if (err) {
-        goto error;
+        goto end;
     }
 
     switch (object->type) {
     case NUMA_OPTIONS_KIND_NODE:
         numa_node_parse(object->u.node.data, opts, &err);
         if (err) {
-            goto error;
+            goto end;
         }
         nb_numa_nodes++;
         break;
@@ -238,13 +238,14 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
         abort();
     }
 
-    return 0;
-
-error:
-    error_report_err(err);
+end:
     qapi_free_NumaOptions(object);
+    if (err) {
+        error_report_err(err);
+        return -1;
+    }
 
-    return -1;
+    return 0;
 }
 
 static char *enumerate_cpus(unsigned long *cpus, int max_cpus)
-- 
2.9.0

[Qemu-devel] [PATCH for-2.7 v3 15/36] pc: simplify passing qemu_irq

[marcandre.lureau]

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

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 47593b7..6b138d6 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 c87c5c1..eb1d414 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.9.0

[Qemu-devel] [PATCH for-2.7 v3 16/36] pc: don't leak a20_line

[marcandre.lureau]

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

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 6b138d6..fd4a050 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.9.0

[Qemu-devel] [PATCH for-2.7 v3 17/36] machine: use class base init generated name

[marcandre.lureau]

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

Remove machine class name initialization from DEFINE_PC_MACHINE, rely on
class base init name generation instead. Get rid of some leaks that way.

Signed-off-by: Marc-André Lureau <marcandre.lureau@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 eb1d414..afd025a 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.9.0

[Qemu-devel] [PATCH for-2.7 v3 18/36] acpi-build: fix array leak

[marcandre.lureau]

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

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.9.0

[Qemu-devel] [PATCH for-2.7 v3 19/36] char: free the tcp connection data when closing

[marcandre.lureau]

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

Make sure the connection data got freed when closing the chardev, to
avoid leaks.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qemu-char.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index a50b8fb..625abcf 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2843,7 +2843,7 @@ static GSource *tcp_chr_add_watch(CharDriverState *chr, GIOCondition cond)
     return qio_channel_create_watch(s->ioc, cond);
 }
 
-static void tcp_chr_disconnect(CharDriverState *chr)
+static void tcp_chr_free_connection(CharDriverState *chr)
 {
     TCPCharDriver *s = chr->opaque;
 
@@ -2851,11 +2851,6 @@ static void tcp_chr_disconnect(CharDriverState *chr)
         return;
     }
 
-    s->connected = 0;
-    if (s->listen_ioc) {
-        s->listen_tag = qio_channel_add_watch(
-            QIO_CHANNEL(s->listen_ioc), G_IO_IN, tcp_chr_accept, chr, NULL);
-    }
     tcp_set_msgfds(chr, NULL, 0);
     remove_fd_in_watch(chr);
     object_unref(OBJECT(s->sioc));
@@ -2863,6 +2858,24 @@ static void tcp_chr_disconnect(CharDriverState *chr)
     object_unref(OBJECT(s->ioc));
     s->ioc = NULL;
     g_free(chr->filename);
+    chr->filename = NULL;
+    s->connected = 0;
+}
+
+static void tcp_chr_disconnect(CharDriverState *chr)
+{
+    TCPCharDriver *s = chr->opaque;
+
+    if (!s->connected) {
+        return;
+    }
+
+    tcp_chr_free_connection(chr);
+
+    if (s->listen_ioc) {
+        s->listen_tag = qio_channel_add_watch(
+            QIO_CHANNEL(s->listen_ioc), G_IO_IN, tcp_chr_accept, chr, NULL);
+    }
     chr->filename = SocketAddress_to_str("disconnected:", s->addr,
                                          s->is_listen, s->is_telnet);
     qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
@@ -3179,6 +3192,7 @@ static void tcp_chr_close(CharDriverState *chr)
     TCPCharDriver *s = chr->opaque;
     int i;
 
+    tcp_chr_free_connection(chr);
     if (s->reconnect_timer) {
         g_source_remove(s->reconnect_timer);
         s->reconnect_timer = 0;
-- 
2.9.0

[Qemu-devel] [PATCH for-2.7 v3 20/36] char: free MuxDriver when closing

[marcandre.lureau]

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

Similarly to other chr_close callbacks, free char type specific data.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qemu-char.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/qemu-char.c b/qemu-char.c
index 625abcf..25c4f67 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -786,6 +786,13 @@ static GSource *mux_chr_add_watch(CharDriverState *s, GIOCondition cond)
     return d->drv->chr_add_watch(d->drv, cond);
 }
 
+static void mux_chr_close(struct CharDriverState *chr)
+{
+    MuxDriver *d = chr->opaque;
+
+    g_free(d);
+}
+
 static CharDriverState *qemu_chr_open_mux(const char *id,
                                           ChardevBackend *backend,
                                           ChardevReturn *ret, Error **errp)
@@ -810,6 +817,7 @@ static CharDriverState *qemu_chr_open_mux(const char *id,
     chr->opaque = d;
     d->drv = drv;
     d->focus = -1;
+    chr->chr_close = mux_chr_close;
     chr->chr_write = mux_chr_write;
     chr->chr_update_read_handler = mux_chr_update_read_handler;
     chr->chr_accept_input = mux_chr_accept_input;
-- 
2.9.0

[Qemu-devel] [PATCH for-2.7 v3 21/36] tests: fix qom-test leaks

[marcandre.lureau]

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

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.9.0

[Qemu-devel] [PATCH for-2.7 v3 22/36] pc: free i8259

[marcandre.lureau]

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

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.9.0

[Qemu-devel] [PATCH for-2.7 v3 23/36] pc: keep gsi reference

[marcandre.lureau]

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

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>
---
 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 afd025a..e97ccad 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.9.0

[Qemu-devel] [PATCH for-2.7 v3 24/36] ahci: free irqs array

[marcandre.lureau]

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

Each irq is referenced by the IDEBus in ide_init2(), thus we can free
the no longer used array.

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

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index bcb9ff9..6defeed 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1478,6 +1478,7 @@ void ahci_realize(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports)
         ad->port.dma->ops = &ahci_dma_ops;
         ide_register_restart_cb(&ad->port);
     }
+    g_free(irqs);
 }
 
 void ahci_uninit(AHCIState *s)
-- 
2.9.0

[Qemu-devel] [PATCH for-2.7 v3 25/36] sd: free timer

[marcandre.lureau]

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

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.9.0

[Qemu-devel] [PATCH for-2.7 v3 26/36] qjson: free str

[marcandre.lureau]

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

Release the qstring allocated in qjson_new().

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

diff --git a/migration/qjson.c b/migration/qjson.c
index 5cae55a..f345904 100644
--- a/migration/qjson.c
+++ b/migration/qjson.c
@@ -109,5 +109,6 @@ void qjson_finish(QJSON *json)
 
 void qjson_destroy(QJSON *json)
 {
+    QDECREF(json->str);
     g_free(json);
 }
-- 
2.9.0

[Qemu-devel] [PATCH for-2.7 v3 27/36] virtio-input: free config list

[marcandre.lureau]

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

Clear the list when finalizing. The list is created during realize with
virtio_input_idstr_config() and later by further calls to
virtio_input_init_config() and virtio_input_add_config().

This leak can be reproduced with device-introspect-test -p
/x86_64/device/introspect/concrete.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/input/virtio-input.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/input/virtio-input.c b/hw/input/virtio-input.c
index a87fd68..ccdf730 100644
--- a/hw/input/virtio-input.c
+++ b/hw/input/virtio-input.c
@@ -270,6 +270,16 @@ static void virtio_input_device_realize(DeviceState *dev, Error **errp)
     vinput->sts = virtio_add_queue(vdev, 64, virtio_input_handle_sts);
 }
 
+static void virtio_input_finalize(Object *obj)
+{
+    VirtIOInput *vinput = VIRTIO_INPUT(obj);
+    VirtIOInputConfig *cfg, *next;
+
+    QTAILQ_FOREACH_SAFE(cfg, &vinput->cfg_list, node, next) {
+        QTAILQ_REMOVE(&vinput->cfg_list, cfg, node);
+        g_free(cfg);
+    }
+}
 static void virtio_input_device_unrealize(DeviceState *dev, Error **errp)
 {
     VirtIOInputClass *vic = VIRTIO_INPUT_GET_CLASS(dev);
@@ -318,6 +328,7 @@ static const TypeInfo virtio_input_info = {
     .class_size    = sizeof(VirtIOInputClass),
     .class_init    = virtio_input_class_init,
     .abstract      = true,
+    .instance_finalize = virtio_input_finalize,
 };
 
 /* ----------------------------------------------------------------- */
-- 
2.9.0

[Qemu-devel] [PATCH for-2.7 v3 28/36] ipmi: free extern timer

[marcandre.lureau]

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

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.9.0

[Qemu-devel] [PATCH for-2.7 v3 29/36] usb: free USBDevice.strings

[marcandre.lureau]

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

The list is created during instance init and further populated with
usb_desc_set_string(). Clear it when unrealizing the device.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/bus.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index c28ccb8..25913ad 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -279,6 +279,13 @@ static void usb_qdev_realize(DeviceState *qdev, Error **errp)
 static void usb_qdev_unrealize(DeviceState *qdev, Error **errp)
 {
     USBDevice *dev = USB_DEVICE(qdev);
+    USBDescString *s, *next;
+
+    QLIST_FOREACH_SAFE(s, &dev->strings, next, next) {
+        QLIST_REMOVE(s, next);
+        g_free(s->str);
+        g_free(s);
+    }
 
     if (dev->attached) {
         usb_device_detach(dev);
-- 
2.9.0

[Qemu-devel] [PATCH for-2.7 v3 30/36] usb: free leaking path

[marcandre.lureau]

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

qdev_get_dev_path() returns an allocated string, free it when no longer
needed.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/desc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/usb/desc.c b/hw/usb/desc.c
index adb026e..5e0e1d1 100644
--- a/hw/usb/desc.c
+++ b/hw/usb/desc.c
@@ -574,6 +574,7 @@ void usb_desc_create_serial(USBDevice *dev)
     }
     dst += snprintf(serial+dst, sizeof(serial)-dst, "-%s", dev->port->path);
     usb_desc_set_string(dev, index, serial);
+    g_free(path);
 }
 
 const char *usb_desc_get_string(USBDevice *dev, uint8_t index)
-- 
2.9.0

[Qemu-devel] [PATCH for-2.7 v3 31/36] bus: simplify name handling

[marcandre.lureau]

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

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.9.0

[Qemu-devel] [PATCH for-2.7 v3 32/36] tests: add qtest_add_data_func_full

[marcandre.lureau]

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

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/libqtest.c | 15 +++++++++++++++
 tests/libqtest.h |  7 ++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

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..e4c9c39 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -413,15 +413,20 @@ const char *qtest_get_arch(void);
 void qtest_add_func(const char *str, void (*fn)(void));
 
 /**
- * qtest_add_data_func:
+ * 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().
  */
+void qtest_add_data_func_full(const char *str, void *data,
+                              void (*fn)(const void *),
+                              GDestroyNotify data_free_func);
+
 void qtest_add_data_func(const char *str, const void *data,
                          void (*fn)(const void *));
 
-- 
2.9.0

[Qemu-devel] [PATCH for-2.7 v3 33/36] tests: pc-cpu-test leaks fixes

[marcandre.lureau]

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

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.9.0

[Qemu-devel] [PATCH for-2.7 v3 34/36] tests: fix rsp leak in postcopy-test

[marcandre.lureau]

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

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.9.0

[Qemu-devel] [PATCH for-2.7 v3 35/36] ahci: fix sglist leak on retry

[marcandre.lureau]

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

ahci-test /x86_64/ahci/io/dma/lba28/retry triggers the following leak:

Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x7fc4b2a25e20 in malloc (/lib64/libasan.so.3+0xc6e20)
    #1 0x7fc4993bce58 in g_malloc (/lib64/libglib-2.0.so.0+0x4ee58)
    #2 0x556a187d4b34 in ahci_populate_sglist hw/ide/ahci.c:896
    #3 0x556a187d8237 in ahci_dma_prepare_buf hw/ide/ahci.c:1367
    #4 0x556a187b5a1a in ide_dma_cb hw/ide/core.c:844
    #5 0x556a187d7eec in ahci_start_dma hw/ide/ahci.c:1333
    #6 0x556a187b650b in ide_start_dma hw/ide/core.c:921
    #7 0x556a187b61e6 in ide_sector_start_dma hw/ide/core.c:911
    #8 0x556a187b9e26 in cmd_write_dma hw/ide/core.c:1486
    #9 0x556a187bd519 in ide_exec_cmd hw/ide/core.c:2027
    #10 0x556a187d71c5 in handle_reg_h2d_fis hw/ide/ahci.c:1204
    #11 0x556a187d7681 in handle_cmd hw/ide/ahci.c:1254
    #12 0x556a187d168a in check_cmd hw/ide/ahci.c:510
    #13 0x556a187d0afc in ahci_port_write hw/ide/ahci.c:314
    #14 0x556a187d105d in ahci_mem_write hw/ide/ahci.c:435
    #15 0x556a1831d959 in memory_region_write_accessor /home/elmarco/src/qemu/memory.c:525
    #16 0x556a1831dc35 in access_with_adjusted_size /home/elmarco/src/qemu/memory.c:591
    #17 0x556a18323ce3 in memory_region_dispatch_write /home/elmarco/src/qemu/memory.c:1262
    #18 0x556a1828cf67 in address_space_write_continue /home/elmarco/src/qemu/exec.c:2578
    #19 0x556a1828d20b in address_space_write /home/elmarco/src/qemu/exec.c:2635
    #20 0x556a1828d92b in address_space_rw /home/elmarco/src/qemu/exec.c:2737
    #21 0x556a1828daf7 in cpu_physical_memory_rw /home/elmarco/src/qemu/exec.c:2746
    #22 0x556a183068d3 in cpu_physical_memory_write /home/elmarco/src/qemu/include/exec/cpu-common.h:72
    #23 0x556a18308194 in qtest_process_command /home/elmarco/src/qemu/qtest.c:382
    #24 0x556a18309999 in qtest_process_inbuf /home/elmarco/src/qemu/qtest.c:573
    #25 0x556a18309a4a in qtest_read /home/elmarco/src/qemu/qtest.c:585
    #26 0x556a18598b85 in qemu_chr_be_write_impl /home/elmarco/src/qemu/qemu-char.c:387
    #27 0x556a18598c52 in qemu_chr_be_write /home/elmarco/src/qemu/qemu-char.c:399
    #28 0x556a185a2afa in tcp_chr_read /home/elmarco/src/qemu/qemu-char.c:2902
    #29 0x556a18cbaf52 in qio_channel_fd_source_dispatch io/channel-watch.c:84

Follow John Snow recommendation:
  Everywhere else ncq_err is used, it is accompanied by a list cleanup
  except for ncq_cb, which is the case you are fixing here.

  Move the sglist destruction inside of ncq_err and then delete it from
  the other two locations to keep it tidy.

  Call dma_buf_commit in ide_dma_cb after the early return. Though, this
  is also a little wonky because this routine does more than clear the
  list, but it is at the moment the centralized "we're done with the
  sglist" function and none of the other side effects that occur in
  dma_buf_commit will interfere with the reset that occurs from
  ide_restart_bh, I think

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/ide/ahci.c | 3 +--
 hw/ide/core.c | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 6defeed..f3438ad 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -919,6 +919,7 @@ static void ncq_err(NCQTransferState *ncq_tfs)
     ide_state->error = ABRT_ERR;
     ide_state->status = READY_STAT | ERR_STAT;
     ncq_tfs->drive->port_regs.scr_err |= (1 << ncq_tfs->tag);
+    qemu_sglist_destroy(&ncq_tfs->sglist);
     ncq_tfs->used = 0;
 }
 
@@ -1025,7 +1026,6 @@ static void execute_ncq_command(NCQTransferState *ncq_tfs)
     default:
         DPRINTF(port, "error: unsupported NCQ command (0x%02x) received\n",
                 ncq_tfs->cmd);
-        qemu_sglist_destroy(&ncq_tfs->sglist);
         ncq_err(ncq_tfs);
     }
 }
@@ -1092,7 +1092,6 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
         error_report("ahci: PRDT length for NCQ command (0x%zx) "
                      "is smaller than the requested size (0x%zx)",
                      ncq_tfs->sglist.size, size);
-        qemu_sglist_destroy(&ncq_tfs->sglist);
         ncq_err(ncq_tfs);
         ahci_trigger_irq(ad->hba, ad, PORT_IRQ_OVERFLOW);
         return;
diff --git a/hw/ide/core.c b/hw/ide/core.c
index f9c8162..82d7f2a 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -822,6 +822,7 @@ static void ide_dma_cb(void *opaque, int ret)
         return;
     }
     if (ret < 0) {
+        dma_buf_commit(s, 0);
         if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s->dma_cmd))) {
             s->bus->dma->aiocb = NULL;
             return;
-- 
2.9.0

[Qemu-devel] [PATCH for-2.7 v3 36/36] tests: fix postcopy-test leaks

[marcandre.lureau]

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

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.9.0

Re: [Qemu-devel] [PATCH for-2.7 v3 19/36] char: free the tcp connection data when closing

[Paolo Bonzini]

On 03/08/2016 16:55, marcandre.lureau@redhat.com wrote:
> @@ -2851,11 +2851,6 @@ static void tcp_chr_disconnect(CharDriverState *chr)
>          return;
>      }
>  
> -    s->connected = 0;
> -    if (s->listen_ioc) {
> -        s->listen_tag = qio_channel_add_watch(
> -            QIO_CHANNEL(s->listen_ioc), G_IO_IN, tcp_chr_accept, chr, NULL);
> -    }
>      tcp_set_msgfds(chr, NULL, 0);
>      remove_fd_in_watch(chr);
>      object_unref(OBJECT(s->sioc));
> @@ -2863,6 +2858,24 @@ static void tcp_chr_disconnect(CharDriverState *chr)
>      object_unref(OBJECT(s->ioc));
>      s->ioc = NULL;
>      g_free(chr->filename);
> +    chr->filename = NULL;
> +    s->connected = 0;
> +}
> +
> +static void tcp_chr_disconnect(CharDriverState *chr)
> +{
> +    TCPCharDriver *s = chr->opaque;
> +
> +    if (!s->connected) {
> +        return;
> +    }
> +
> +    tcp_chr_free_connection(chr);
> +
> +    if (s->listen_ioc) {
> +        s->listen_tag = qio_channel_add_watch(
> +            QIO_CHANNEL(s->listen_ioc), G_IO_IN, tcp_chr_accept, chr, NULL);
> +    }

I think

    if (s->read_msgfds_num) {
        for (i = 0; i < s->read_msgfds_num; i++) {
            close(s->read_msgfds[i]);
        }
        g_free(s->read_msgfds);
    }


should be moved from tcp_chr_close to tcp_chr_free_connection.  The
following parts of tcp_chr_close instead are now duplicate, and can be
removed:

    remove_fd_in_watch(chr);
    if (s->ioc) {
        object_unref(OBJECT(s->ioc));
    }
    if (s->write_msgfds_num) {
        g_free(s->write_msgfds);
    }

are now duplicate and can be removed.

Finally, since you're at it, here in tcp_set_msgfds:

    /* clear old pending fd array */
    g_free(s->write_msgfds);
    s->write_msgfds = NULL;

it's best to add a s->write_msgfds_num = 0.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH for-2.7 v3 19/36] char: free the tcp connection data when closing

[Marc-André Lureau]

Hi

----- Original Message -----
> 
> 
> On 03/08/2016 16:55, marcandre.lureau@redhat.com wrote:
> > @@ -2851,11 +2851,6 @@ static void tcp_chr_disconnect(CharDriverState *chr)
> >          return;
> >      }
> >  
> > -    s->connected = 0;
> > -    if (s->listen_ioc) {
> > -        s->listen_tag = qio_channel_add_watch(
> > -            QIO_CHANNEL(s->listen_ioc), G_IO_IN, tcp_chr_accept, chr,
> > NULL);
> > -    }
> >      tcp_set_msgfds(chr, NULL, 0);
> >      remove_fd_in_watch(chr);
> >      object_unref(OBJECT(s->sioc));
> > @@ -2863,6 +2858,24 @@ static void tcp_chr_disconnect(CharDriverState *chr)
> >      object_unref(OBJECT(s->ioc));
> >      s->ioc = NULL;
> >      g_free(chr->filename);
> > +    chr->filename = NULL;
> > +    s->connected = 0;
> > +}
> > +
> > +static void tcp_chr_disconnect(CharDriverState *chr)
> > +{
> > +    TCPCharDriver *s = chr->opaque;
> > +
> > +    if (!s->connected) {
> > +        return;
> > +    }
> > +
> > +    tcp_chr_free_connection(chr);
> > +
> > +    if (s->listen_ioc) {
> > +        s->listen_tag = qio_channel_add_watch(
> > +            QIO_CHANNEL(s->listen_ioc), G_IO_IN, tcp_chr_accept, chr,
> > NULL);
> > +    }
> 
> I think
> 
>     if (s->read_msgfds_num) {
>         for (i = 0; i < s->read_msgfds_num; i++) {
>             close(s->read_msgfds[i]);
>         }
>         g_free(s->read_msgfds);
>     }
> 
> 
> should be moved from tcp_chr_close to tcp_chr_free_connection.  The
> following parts of tcp_chr_close instead are now duplicate, and can be
> removed:
> 
>     remove_fd_in_watch(chr);
>     if (s->ioc) {
>         object_unref(OBJECT(s->ioc));
>     }
>     if (s->write_msgfds_num) {
>         g_free(s->write_msgfds);
>     }
> 
> are now duplicate and can be removed.

correct

> 
> Finally, since you're at it, here in tcp_set_msgfds:
> 
>     /* clear old pending fd array */
>     g_free(s->write_msgfds);
>     s->write_msgfds = NULL;
> 
> it's best to add a s->write_msgfds_num = 0.

That makes sense, for the error case.

done

Re: [Qemu-devel] [PATCH for-2.7 v3 01/36] build-sys: fix building with make CFLAGS=.. argument

[Paolo Bonzini]

On 03/08/2016 16:55, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> When calling make with a CFLAGS=.. argument, the -g/-O filter is not
> applied, which may result with build failure with ASAN for example. It
> could be solved with an 'override' directive on CFLAGS, but that would
> actually prevent setting different CFLAGS manually.
> 
> Instead, filter the CFLAGS argument from the top-level Makefile (so
> you could still call make with a different CFLAGS argument on a
> rom/Makefile manually)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  Makefile                   | 3 ++-
>  pc-bios/optionrom/Makefile | 2 --
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 0d7647f..50b4b3a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -225,8 +225,9 @@ dtc/%:
>  $(SUBDIR_RULES): libqemuutil.a libqemustub.a $(common-obj-y) $(qom-obj-y) $(crypto-aes-obj-$(CONFIG_USER_ONLY))
>  
>  ROMSUBDIR_RULES=$(patsubst %,romsubdir-%, $(ROMS))
> +# Only keep -O and -g cflags
>  romsubdir-%:
> -	$(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C pc-bios/$* V="$(V)" TARGET_DIR="$*/",)
> +	$(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C pc-bios/$* V="$(V)" TARGET_DIR="$*/" CFLAGS="$(filter -O% -g%,$(CFLAGS))",)
>  
>  ALL_SUBDIRS=$(TARGET_DIRS) $(patsubst %,pc-bios/%, $(ROMS))
>  
> diff --git a/pc-bios/optionrom/Makefile b/pc-bios/optionrom/Makefile
> index 24e175e..6bab490 100644
> --- a/pc-bios/optionrom/Makefile
> +++ b/pc-bios/optionrom/Makefile
> @@ -24,8 +24,6 @@ QEMU_CFLAGS += $(call cc-option, $(QEMU_CFLAGS), -no-integrated-as)
>  QEMU_CFLAGS += -m32 -include $(SRC_PATH)/pc-bios/optionrom/code16gcc.h
>  endif
>  
> -# Drop gcov and glib flags
> -CFLAGS := $(filter -O% -g%, $(CFLAGS))
>  QEMU_INCLUDES += -I$(SRC_PATH)
>  
>  Wa = -Wa,
> 

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

Re: [Qemu-devel] [PATCH for-2.7 v3 03/36] qga: free the whole blacklist

[Markus Armbruster]

Copying the QGA maintainer.

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Free the list, not just the elements.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/glib-compat.h | 9 +++++++++
>  qga/main.c            | 8 ++------
>  2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/include/glib-compat.h b/include/glib-compat.h
> index 01aa7b3..6d643b2 100644
> --- a/include/glib-compat.h
> +++ b/include/glib-compat.h
> @@ -260,4 +260,13 @@ static inline void g_hash_table_add(GHashTable *hash_table, gpointer key)
>      } while (0)
>  #endif
>  
> +/*
> + * A GFunc function helper freeing the first argument (not part of glib)

Well, it's not part of GLib, because non-obsolete GLib has no use for
it!  You'd simply pass g_free directly to a _free_full() function
instead of passing a silly wrapper to a _foreach() function.

The commit does a bit more than just plug a leak, it also provides a new
helper for general use.  Mention in the commit message?

I wonder how many more of these silly g_free() wrappers we have.  A
quick grep reports hits in string-input-visitor.c and
qobject/json-streamer.c.  If you add one to a header, you get to hunt
them down :)

> + */
> +static inline void qemu_g_func_free(gpointer data,
> +                                    gpointer user_data)
> +{
> +    g_free(data);
> +}
> +
>  #endif
> diff --git a/qga/main.c b/qga/main.c
> index 4c3b2c7..868508b 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -1175,6 +1175,8 @@ static void config_free(GAConfig *config)
>  #ifdef CONFIG_FSFREEZE
>      g_free(config->fsfreeze_hook);
>  #endif
> +    g_list_foreach(config->blacklist, qemu_g_func_free, NULL);
> +    g_list_free(config->blacklist);

Modern GLib code doesn't need silly wrappers:

    g_list_free_full(config->blacklist, g_free);

Unfortunately, this requires 2.28, and we're stull stuck at 2.22.
Please explain this in the commit message.

Even better, provide a replacement in glib-compat.h #if
!GLIB_CHECK_VERSION(2, 28, 0).  Will facilitate ditching the work-around
when we upgrade to 2.28.

>      g_free(config);
>  }
>  
> @@ -1310,11 +1312,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 +1376,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);

If you at least explain why not g_list_free_full() in the commit
message, you can add
Reviewed-by: Markus Armbruster <armbru@redhat.com>

But I'd like to encourage you to explore providing a replacement for
g_list_free_full().

Re: [Qemu-devel] [PATCH for-2.7 v3 04/36] qga: free remaining leaking state

[Markus Armbruster]

Copying the QGA maintainer.

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qga/guest-agent-command-state.c | 7 +++++++
>  qga/guest-agent-core.h          | 1 +
>  qga/main.c                      | 6 ++++++
>  3 files changed, 14 insertions(+)
>
> diff --git a/qga/guest-agent-command-state.c b/qga/guest-agent-command-state.c
> index 4de229c..31c6368 100644
> --- a/qga/guest-agent-command-state.c
> +++ b/qga/guest-agent-command-state.c
> @@ -71,3 +71,10 @@ GACommandState *ga_command_state_new(void)
>      cs->groups = NULL;
>      return cs;
>  }
> +
> +void ga_command_state_free(GACommandState *cs)
> +{
> +    g_slist_foreach(cs->groups, qemu_g_func_free, NULL);
> +    g_slist_free(cs->groups);

Remarks on g_list_free_full() in PATCH 03 apply to g_slist_free_full()
here.

> +    g_free(cs);
> +}
> 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);
> diff --git a/qga/main.c b/qga/main.c
> index 868508b..0038702 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -1372,6 +1372,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);
> @@ -1384,6 +1386,10 @@ end:
>      }
>  
>      config_free(config);
> +    if (s->main_loop) {
> +        g_main_loop_unref(s->main_loop);
> +    }
> +    g_free(s);
>  
>      return ret;
>  }

Not bothering to free memory immediately before terminating the process
is not a leak.  The commit message shouldn't claim it is.

Personally, I wouldn't bother with freeing memory there.  Applies to
PATCH 03, too.  But it looks like the maintainer likes having it done.
If that's true, then doing it completely is probably better.

Leaving actual review to the maintainer.

Re: [Qemu-devel] [PATCH for-2.7 v3 03/36] qga: free the whole blacklist

[Marc-André Lureau]

Hi

----- Original Message -----
> Copying the QGA maintainer.
> 
> marcandre.lureau@redhat.com writes:
> 
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Free the list, not just the elements.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  include/glib-compat.h | 9 +++++++++
> >  qga/main.c            | 8 ++------
> >  2 files changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/glib-compat.h b/include/glib-compat.h
> > index 01aa7b3..6d643b2 100644
> > --- a/include/glib-compat.h
> > +++ b/include/glib-compat.h
> > @@ -260,4 +260,13 @@ static inline void g_hash_table_add(GHashTable
> > *hash_table, gpointer key)
> >      } while (0)
> >  #endif
> >  
> > +/*
> > + * A GFunc function helper freeing the first argument (not part of glib)
> 
> Well, it's not part of GLib, because non-obsolete GLib has no use for
> it!  You'd simply pass g_free directly to a _free_full() function
> instead of passing a silly wrapper to a _foreach() function.
> 
> The commit does a bit more than just plug a leak, it also provides a new
> helper for general use.  Mention in the commit message?
> 
> I wonder how many more of these silly g_free() wrappers we have.  A
> quick grep reports hits in string-input-visitor.c and
> qobject/json-streamer.c.  If you add one to a header, you get to hunt
> them down :)
> 
> > + */
> > +static inline void qemu_g_func_free(gpointer data,
> > +                                    gpointer user_data)
> > +{
> > +    g_free(data);
> > +}
> > +
> >  #endif
> > diff --git a/qga/main.c b/qga/main.c
> > index 4c3b2c7..868508b 100644
> > --- a/qga/main.c
> > +++ b/qga/main.c
> > @@ -1175,6 +1175,8 @@ static void config_free(GAConfig *config)
> >  #ifdef CONFIG_FSFREEZE
> >      g_free(config->fsfreeze_hook);
> >  #endif
> > +    g_list_foreach(config->blacklist, qemu_g_func_free, NULL);
> > +    g_list_free(config->blacklist);
> 
> Modern GLib code doesn't need silly wrappers:
> 
>     g_list_free_full(config->blacklist, g_free);
> 
> Unfortunately, this requires 2.28, and we're stull stuck at 2.22.
> Please explain this in the commit message.
> 
> Even better, provide a replacement in glib-compat.h #if
> !GLIB_CHECK_VERSION(2, 28, 0).  Will facilitate ditching the work-around
> when we upgrade to 2.28.

ok

> 
> >      g_free(config);
> >  }
> >  
> > @@ -1310,11 +1312,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 +1376,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);
> 
> If you at least explain why not g_list_free_full() in the commit
> message, you can add
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
> But I'd like to encourage you to explore providing a replacement for
> g_list_free_full().

Such replacement would make use of a (GFunc) cast, glib implementation is calling g_list_foreach (list, (GFunc) free_func, NULL), but Eric didn't want to have such cast in qemu code. He suggested to have the static inline helper in https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06561.html

Re: [Qemu-devel] [PATCH for-2.7 v3 17/36] machine: use class base init generated name

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Remove machine class name initialization from DEFINE_PC_MACHINE, rely on
> class base init name generation instead. Get rid of some leaks that way.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@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 eb1d414..afd025a 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 = { \

I guess there are is at least one assignment to mc->name not visible in
this patch that assigns an allocated string, which leaks before the
patch.  The commit message seems to provide a clue "class base init name
generation".  I could probably find it with some effort, but patches
that take that much work to understand make me grumpy.  Please provide
another clue :)

Re: [Qemu-devel] [PATCH for-2.7 v3 17/36] machine: use class base init generated name

[Marc-André Lureau]

Hi

On Thu, Aug 4, 2016 at 5:58 PM Markus Armbruster <armbru@redhat.com> wrote:

> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Remove machine class name initialization from DEFINE_PC_MACHINE, rely on
> > class base init name generation instead. Get rid of some leaks that way.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@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 eb1d414..afd025a 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 = { \
>
> I guess there are is at least one assignment to mc->name not visible in
> this patch that assigns an allocated string, which leaks before the
> patch.  The commit message seems to provide a clue "class base init name
> generation".  I could probably find it with some effort, but patches
> that take that much work to understand make me grumpy.  Please provide
> another clue :)
>

Sorry, thanks for reminding me to write better commit messages.

git grep 'mc->name ='
hw/core/machine.c:        mc->name = g_strndup(cname,

Is that better:

Remove machine class name initialization from DEFINE_PC_MACHINE, rely on
name generation from machine_class_base_init() instead, and free the
corresponding allocation in machine_class_finalize().
-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH for-2.7 v3 03/36] qga: free the whole blacklist

[Markus Armbruster]

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

> Hi
>
> ----- Original Message -----
>> Copying the QGA maintainer.
>> 
>> marcandre.lureau@redhat.com writes:
>> 
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > Free the list, not just the elements.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  include/glib-compat.h | 9 +++++++++
>> >  qga/main.c            | 8 ++------
>> >  2 files changed, 11 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/include/glib-compat.h b/include/glib-compat.h
>> > index 01aa7b3..6d643b2 100644
>> > --- a/include/glib-compat.h
>> > +++ b/include/glib-compat.h
>> > @@ -260,4 +260,13 @@ static inline void g_hash_table_add(GHashTable
>> > *hash_table, gpointer key)
>> >      } while (0)
>> >  #endif
>> >  
>> > +/*
>> > + * A GFunc function helper freeing the first argument (not part of glib)
>> 
>> Well, it's not part of GLib, because non-obsolete GLib has no use for
>> it!  You'd simply pass g_free directly to a _free_full() function
>> instead of passing a silly wrapper to a _foreach() function.
>> 
>> The commit does a bit more than just plug a leak, it also provides a new
>> helper for general use.  Mention in the commit message?
>> 
>> I wonder how many more of these silly g_free() wrappers we have.  A
>> quick grep reports hits in string-input-visitor.c and
>> qobject/json-streamer.c.  If you add one to a header, you get to hunt
>> them down :)
>> 
>> > + */
>> > +static inline void qemu_g_func_free(gpointer data,
>> > +                                    gpointer user_data)
>> > +{
>> > +    g_free(data);
>> > +}
>> > +
>> >  #endif
>> > diff --git a/qga/main.c b/qga/main.c
>> > index 4c3b2c7..868508b 100644
>> > --- a/qga/main.c
>> > +++ b/qga/main.c
>> > @@ -1175,6 +1175,8 @@ static void config_free(GAConfig *config)
>> >  #ifdef CONFIG_FSFREEZE
>> >      g_free(config->fsfreeze_hook);
>> >  #endif
>> > +    g_list_foreach(config->blacklist, qemu_g_func_free, NULL);
>> > +    g_list_free(config->blacklist);
>> 
>> Modern GLib code doesn't need silly wrappers:
>> 
>>     g_list_free_full(config->blacklist, g_free);
>> 
>> Unfortunately, this requires 2.28, and we're stull stuck at 2.22.
>> Please explain this in the commit message.
>> 
>> Even better, provide a replacement in glib-compat.h #if
>> !GLIB_CHECK_VERSION(2, 28, 0).  Will facilitate ditching the work-around
>> when we upgrade to 2.28.
>
> ok
>
>> 
>> >      g_free(config);
>> >  }
>> >  
>> > @@ -1310,11 +1312,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 +1376,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);
>> 
>> If you at least explain why not g_list_free_full() in the commit
>> message, you can add
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> 
>> But I'd like to encourage you to explore providing a replacement for
>> g_list_free_full().
>
> Such replacement would make use of a (GFunc) cast, glib implementation is calling g_list_foreach (list, (GFunc) free_func, NULL), but Eric didn't want to have such cast in qemu code. He suggested to have the static inline helper in https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06561.html

Pointlessly dirty:

    g_list_foreach(list, (GFunc)g_free, NULL)

Trade dirty for cumbersome:

    void free_wrapper(gpointer data, gpointer unused)
    {
        g_free(data)
    }

    g_list_foreach(list, free_wrapper, NULL);

But this is C.  In C, simple things are best done the simple way.  Even
when that means we don't get to show off how amazingly well we've been
educated on higher order functions:

    for (node = list; node; node = next) {
        next = node->next;
        g_free(node);
    }

Simple, stupid, and not dirty.

Quote: "Note that it is considered perfectly acceptable to access
list->next directly."

Re: [Qemu-devel] [PATCH for-2.7 v3 23/36] pc: keep gsi reference

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> 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>

I looked into cleaning up the qemu_irq allocation mess long ago, but
gave up after the work-in-progress had ballooned to a dozen commits or
so.

> ---
>  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 afd025a..e97ccad 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;

It's a start, I guess.

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

Re: [Qemu-devel] [PATCH for-2.7 v3 35/36] ahci: fix sglist leak on retry

[Markus Armbruster]

John, can you review this one?

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> ahci-test /x86_64/ahci/io/dma/lba28/retry triggers the following leak:
>
> Direct leak of 16 byte(s) in 1 object(s) allocated from:
>     #0 0x7fc4b2a25e20 in malloc (/lib64/libasan.so.3+0xc6e20)
>     #1 0x7fc4993bce58 in g_malloc (/lib64/libglib-2.0.so.0+0x4ee58)
>     #2 0x556a187d4b34 in ahci_populate_sglist hw/ide/ahci.c:896
>     #3 0x556a187d8237 in ahci_dma_prepare_buf hw/ide/ahci.c:1367
>     #4 0x556a187b5a1a in ide_dma_cb hw/ide/core.c:844
>     #5 0x556a187d7eec in ahci_start_dma hw/ide/ahci.c:1333
>     #6 0x556a187b650b in ide_start_dma hw/ide/core.c:921
>     #7 0x556a187b61e6 in ide_sector_start_dma hw/ide/core.c:911
>     #8 0x556a187b9e26 in cmd_write_dma hw/ide/core.c:1486
>     #9 0x556a187bd519 in ide_exec_cmd hw/ide/core.c:2027
>     #10 0x556a187d71c5 in handle_reg_h2d_fis hw/ide/ahci.c:1204
>     #11 0x556a187d7681 in handle_cmd hw/ide/ahci.c:1254
>     #12 0x556a187d168a in check_cmd hw/ide/ahci.c:510
>     #13 0x556a187d0afc in ahci_port_write hw/ide/ahci.c:314
>     #14 0x556a187d105d in ahci_mem_write hw/ide/ahci.c:435
>     #15 0x556a1831d959 in memory_region_write_accessor /home/elmarco/src/qemu/memory.c:525
>     #16 0x556a1831dc35 in access_with_adjusted_size /home/elmarco/src/qemu/memory.c:591
>     #17 0x556a18323ce3 in memory_region_dispatch_write /home/elmarco/src/qemu/memory.c:1262
>     #18 0x556a1828cf67 in address_space_write_continue /home/elmarco/src/qemu/exec.c:2578
>     #19 0x556a1828d20b in address_space_write /home/elmarco/src/qemu/exec.c:2635
>     #20 0x556a1828d92b in address_space_rw /home/elmarco/src/qemu/exec.c:2737
>     #21 0x556a1828daf7 in cpu_physical_memory_rw /home/elmarco/src/qemu/exec.c:2746
>     #22 0x556a183068d3 in cpu_physical_memory_write /home/elmarco/src/qemu/include/exec/cpu-common.h:72
>     #23 0x556a18308194 in qtest_process_command /home/elmarco/src/qemu/qtest.c:382
>     #24 0x556a18309999 in qtest_process_inbuf /home/elmarco/src/qemu/qtest.c:573
>     #25 0x556a18309a4a in qtest_read /home/elmarco/src/qemu/qtest.c:585
>     #26 0x556a18598b85 in qemu_chr_be_write_impl /home/elmarco/src/qemu/qemu-char.c:387
>     #27 0x556a18598c52 in qemu_chr_be_write /home/elmarco/src/qemu/qemu-char.c:399
>     #28 0x556a185a2afa in tcp_chr_read /home/elmarco/src/qemu/qemu-char.c:2902
>     #29 0x556a18cbaf52 in qio_channel_fd_source_dispatch io/channel-watch.c:84
>
> Follow John Snow recommendation:
>   Everywhere else ncq_err is used, it is accompanied by a list cleanup
>   except for ncq_cb, which is the case you are fixing here.
>
>   Move the sglist destruction inside of ncq_err and then delete it from
>   the other two locations to keep it tidy.
>
>   Call dma_buf_commit in ide_dma_cb after the early return. Though, this
>   is also a little wonky because this routine does more than clear the
>   list, but it is at the moment the centralized "we're done with the
>   sglist" function and none of the other side effects that occur in
>   dma_buf_commit will interfere with the reset that occurs from
>   ide_restart_bh, I think
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/ide/ahci.c | 3 +--
>  hw/ide/core.c | 1 +
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 6defeed..f3438ad 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -919,6 +919,7 @@ static void ncq_err(NCQTransferState *ncq_tfs)
>      ide_state->error = ABRT_ERR;
>      ide_state->status = READY_STAT | ERR_STAT;
>      ncq_tfs->drive->port_regs.scr_err |= (1 << ncq_tfs->tag);
> +    qemu_sglist_destroy(&ncq_tfs->sglist);
>      ncq_tfs->used = 0;
>  }
>  
> @@ -1025,7 +1026,6 @@ static void execute_ncq_command(NCQTransferState *ncq_tfs)
>      default:
>          DPRINTF(port, "error: unsupported NCQ command (0x%02x) received\n",
>                  ncq_tfs->cmd);
> -        qemu_sglist_destroy(&ncq_tfs->sglist);
>          ncq_err(ncq_tfs);
>      }
>  }
> @@ -1092,7 +1092,6 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
>          error_report("ahci: PRDT length for NCQ command (0x%zx) "
>                       "is smaller than the requested size (0x%zx)",
>                       ncq_tfs->sglist.size, size);
> -        qemu_sglist_destroy(&ncq_tfs->sglist);
>          ncq_err(ncq_tfs);
>          ahci_trigger_irq(ad->hba, ad, PORT_IRQ_OVERFLOW);
>          return;
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index f9c8162..82d7f2a 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -822,6 +822,7 @@ static void ide_dma_cb(void *opaque, int ret)
>          return;
>      }
>      if (ret < 0) {
> +        dma_buf_commit(s, 0);
>          if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s->dma_cmd))) {
>              s->bus->dma->aiocb = NULL;
>              return;

Re: [Qemu-devel] [PATCH for-2.7 v3 32/36] tests: add qtest_add_data_func_full

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Allows one to specify a destroy function for the test data.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/libqtest.c | 15 +++++++++++++++
>  tests/libqtest.h |  7 ++++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
>
> 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);

Umm, are these function casts kosher?

I can't find documentation for g_test_add_vtable().  Got a pointer for
me?  Sure GLib 2.22 provides it?

> +#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..e4c9c39 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -413,15 +413,20 @@ const char *qtest_get_arch(void);
>  void qtest_add_func(const char *str, void (*fn)(void));
>  
>  /**
> - * qtest_add_data_func:
> + * 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().
>   */
> +void qtest_add_data_func_full(const char *str, void *data,
> +                              void (*fn)(const void *),
> +                              GDestroyNotify data_free_func);
> +
>  void qtest_add_data_func(const char *str, const void *data,
>                           void (*fn)(const void *));

Please keep qtest_add_data_func() documented.

Re: [Qemu-devel] [PATCH for-2.7 v3 03/36] qga: free the whole blacklist

[Marc-André Lureau]

Hi

----- Original Message -----
> Marc-André Lureau <mlureau@redhat.com> writes:
> 
> > Hi
> >
> > ----- Original Message -----
> >> Copying the QGA maintainer.
> >> 
> >> marcandre.lureau@redhat.com writes:
> >> 
> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >
> >> > Free the list, not just the elements.
> >> >
> >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> > ---
> >> >  include/glib-compat.h | 9 +++++++++
> >> >  qga/main.c            | 8 ++------
> >> >  2 files changed, 11 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/include/glib-compat.h b/include/glib-compat.h
> >> > index 01aa7b3..6d643b2 100644
> >> > --- a/include/glib-compat.h
> >> > +++ b/include/glib-compat.h
> >> > @@ -260,4 +260,13 @@ static inline void g_hash_table_add(GHashTable
> >> > *hash_table, gpointer key)
> >> >      } while (0)
> >> >  #endif
> >> >  
> >> > +/*
> >> > + * A GFunc function helper freeing the first argument (not part of
> >> > glib)
> >> 
> >> Well, it's not part of GLib, because non-obsolete GLib has no use for
> >> it!  You'd simply pass g_free directly to a _free_full() function
> >> instead of passing a silly wrapper to a _foreach() function.
> >> 
> >> The commit does a bit more than just plug a leak, it also provides a new
> >> helper for general use.  Mention in the commit message?
> >> 
> >> I wonder how many more of these silly g_free() wrappers we have.  A
> >> quick grep reports hits in string-input-visitor.c and
> >> qobject/json-streamer.c.  If you add one to a header, you get to hunt
> >> them down :)
> >> 
> >> > + */
> >> > +static inline void qemu_g_func_free(gpointer data,
> >> > +                                    gpointer user_data)
> >> > +{
> >> > +    g_free(data);
> >> > +}
> >> > +
> >> >  #endif
> >> > diff --git a/qga/main.c b/qga/main.c
> >> > index 4c3b2c7..868508b 100644
> >> > --- a/qga/main.c
> >> > +++ b/qga/main.c
> >> > @@ -1175,6 +1175,8 @@ static void config_free(GAConfig *config)
> >> >  #ifdef CONFIG_FSFREEZE
> >> >      g_free(config->fsfreeze_hook);
> >> >  #endif
> >> > +    g_list_foreach(config->blacklist, qemu_g_func_free, NULL);
> >> > +    g_list_free(config->blacklist);
> >> 
> >> Modern GLib code doesn't need silly wrappers:
> >> 
> >>     g_list_free_full(config->blacklist, g_free);
> >> 
> >> Unfortunately, this requires 2.28, and we're stull stuck at 2.22.
> >> Please explain this in the commit message.
> >> 
> >> Even better, provide a replacement in glib-compat.h #if
> >> !GLIB_CHECK_VERSION(2, 28, 0).  Will facilitate ditching the work-around
> >> when we upgrade to 2.28.
> >
> > ok
> >
> >> 
> >> >      g_free(config);
> >> >  }
> >> >  
> >> > @@ -1310,11 +1312,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 +1376,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);
> >> 
> >> If you at least explain why not g_list_free_full() in the commit
> >> message, you can add
> >> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >> 
> >> But I'd like to encourage you to explore providing a replacement for
> >> g_list_free_full().
> >
> > Such replacement would make use of a (GFunc) cast, glib implementation is
> > calling g_list_foreach (list, (GFunc) free_func, NULL), but Eric didn't
> > want to have such cast in qemu code. He suggested to have the static
> > inline helper in
> > https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06561.html
> 
> Pointlessly dirty:
> 
>     g_list_foreach(list, (GFunc)g_free, NULL)
> 
> Trade dirty for cumbersome:
> 
>     void free_wrapper(gpointer data, gpointer unused)
>     {
>         g_free(data)
>     }
> 
>     g_list_foreach(list, free_wrapper, NULL);
> 
> But this is C.  In C, simple things are best done the simple way.  Even
> when that means we don't get to show off how amazingly well we've been
> educated on higher order functions:
> 
>     for (node = list; node; node = next) {
>         next = node->next;
>         g_free(node);
>     }
> 
> Simple, stupid, and not dirty.

If only we were paid to write more lines of code ;) Anyway, that's fine by me (it's work after all, I'll write elegant code for fun time ;)

> Quote: "Note that it is considered perfectly acceptable to access
> list->next directly."

Funny that quote is only in GList, but GSList also documents and exposes the struct.

Re: [Qemu-devel] [PATCH for-2.7 v3 32/36] tests: add qtest_add_data_func_full

[Marc-André Lureau]

Hi

----- Original Message -----
> marcandre.lureau@redhat.com writes:
> 
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Allows one to specify a destroy function for the test data.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  tests/libqtest.c | 15 +++++++++++++++
> >  tests/libqtest.h |  7 ++++++-
> >  2 files changed, 21 insertions(+), 1 deletion(-)
> >
> > 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);
> 
> Umm, are these function casts kosher?
> 
> I can't find documentation for g_test_add_vtable().  Got a pointer for
> me?  Sure GLib 2.22 provides it?

Yes, https://git.gnome.org/browse/glib/tree/glib/gtestutils.h?h=2.22.0#n214

See also: https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg00073.html


> 
> > +#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..e4c9c39 100644
> > --- a/tests/libqtest.h
> > +++ b/tests/libqtest.h
> > @@ -413,15 +413,20 @@ const char *qtest_get_arch(void);
> >  void qtest_add_func(const char *str, void (*fn)(void));
> >  
> >  /**
> > - * qtest_add_data_func:
> > + * 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().
> >   */
> > +void qtest_add_data_func_full(const char *str, void *data,
> > +                              void (*fn)(const void *),
> > +                              GDestroyNotify data_free_func);
> > +
> >  void qtest_add_data_func(const char *str, const void *data,
> >                           void (*fn)(const void *));
> 
> Please keep qtest_add_data_func() documented.

Ok (I thought it was quite enough based on the _full description)

Re: [Qemu-devel] [PATCH for-2.7 v3 17/36] machine: use class base init generated name

[Markus Armbruster]

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

> Hi
>
> On Thu, Aug 4, 2016 at 5:58 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> marcandre.lureau@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > Remove machine class name initialization from DEFINE_PC_MACHINE, rely on
>> > class base init name generation instead. Get rid of some leaks that way.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@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 eb1d414..afd025a 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 = { \
>>
>> I guess there are is at least one assignment to mc->name not visible in
>> this patch that assigns an allocated string, which leaks before the
>> patch.  The commit message seems to provide a clue "class base init name
>> generation".  I could probably find it with some effort, but patches
>> that take that much work to understand make me grumpy.  Please provide
>> another clue :)
>>
>
> Sorry, thanks for reminding me to write better commit messages.

Good commit messages are hard.

> git grep 'mc->name ='
> hw/core/machine.c:        mc->name = g_strndup(cname,

Aha: the concrete machine type's init function overwrites the strdup()ed
value set by machine_class_base_init(), leaking it.  Your fix removes
the overwrites and adds a free.

As far as I can see, you got all such overwrites.

> Is that better:
>
> Remove machine class name initialization from DEFINE_PC_MACHINE, rely on
> name generation from machine_class_base_init() instead, and free the
> corresponding allocation in machine_class_finalize().

Works for me.  Alternatively:

    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().

Use the one you like better, or mix them up to taste.

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

Re: [Qemu-devel] [PATCH for-2.7 v3 35/36] ahci: fix sglist leak on retry

[John Snow]

On 08/04/2016 10:46 AM, Markus Armbruster wrote:
> John, can you review this one?
>

I'm very sorry for the delay.

> marcandre.lureau@redhat.com writes:
>
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> ahci-test /x86_64/ahci/io/dma/lba28/retry triggers the following leak:
>>
>> Direct leak of 16 byte(s) in 1 object(s) allocated from:
>>     #0 0x7fc4b2a25e20 in malloc (/lib64/libasan.so.3+0xc6e20)
>>     #1 0x7fc4993bce58 in g_malloc (/lib64/libglib-2.0.so.0+0x4ee58)
>>     #2 0x556a187d4b34 in ahci_populate_sglist hw/ide/ahci.c:896
>>     #3 0x556a187d8237 in ahci_dma_prepare_buf hw/ide/ahci.c:1367
>>     #4 0x556a187b5a1a in ide_dma_cb hw/ide/core.c:844
>>     #5 0x556a187d7eec in ahci_start_dma hw/ide/ahci.c:1333
>>     #6 0x556a187b650b in ide_start_dma hw/ide/core.c:921
>>     #7 0x556a187b61e6 in ide_sector_start_dma hw/ide/core.c:911
>>     #8 0x556a187b9e26 in cmd_write_dma hw/ide/core.c:1486
>>     #9 0x556a187bd519 in ide_exec_cmd hw/ide/core.c:2027
>>     #10 0x556a187d71c5 in handle_reg_h2d_fis hw/ide/ahci.c:1204
>>     #11 0x556a187d7681 in handle_cmd hw/ide/ahci.c:1254
>>     #12 0x556a187d168a in check_cmd hw/ide/ahci.c:510
>>     #13 0x556a187d0afc in ahci_port_write hw/ide/ahci.c:314
>>     #14 0x556a187d105d in ahci_mem_write hw/ide/ahci.c:435
>>     #15 0x556a1831d959 in memory_region_write_accessor /home/elmarco/src/qemu/memory.c:525
>>     #16 0x556a1831dc35 in access_with_adjusted_size /home/elmarco/src/qemu/memory.c:591
>>     #17 0x556a18323ce3 in memory_region_dispatch_write /home/elmarco/src/qemu/memory.c:1262
>>     #18 0x556a1828cf67 in address_space_write_continue /home/elmarco/src/qemu/exec.c:2578
>>     #19 0x556a1828d20b in address_space_write /home/elmarco/src/qemu/exec.c:2635
>>     #20 0x556a1828d92b in address_space_rw /home/elmarco/src/qemu/exec.c:2737
>>     #21 0x556a1828daf7 in cpu_physical_memory_rw /home/elmarco/src/qemu/exec.c:2746
>>     #22 0x556a183068d3 in cpu_physical_memory_write /home/elmarco/src/qemu/include/exec/cpu-common.h:72
>>     #23 0x556a18308194 in qtest_process_command /home/elmarco/src/qemu/qtest.c:382
>>     #24 0x556a18309999 in qtest_process_inbuf /home/elmarco/src/qemu/qtest.c:573
>>     #25 0x556a18309a4a in qtest_read /home/elmarco/src/qemu/qtest.c:585
>>     #26 0x556a18598b85 in qemu_chr_be_write_impl /home/elmarco/src/qemu/qemu-char.c:387
>>     #27 0x556a18598c52 in qemu_chr_be_write /home/elmarco/src/qemu/qemu-char.c:399
>>     #28 0x556a185a2afa in tcp_chr_read /home/elmarco/src/qemu/qemu-char.c:2902
>>     #29 0x556a18cbaf52 in qio_channel_fd_source_dispatch io/channel-watch.c:84
>>
>> Follow John Snow recommendation:
>>   Everywhere else ncq_err is used, it is accompanied by a list cleanup
>>   except for ncq_cb, which is the case you are fixing here.
>>
>>   Move the sglist destruction inside of ncq_err and then delete it from
>>   the other two locations to keep it tidy.
>>
>>   Call dma_buf_commit in ide_dma_cb after the early return. Though, this
>>   is also a little wonky because this routine does more than clear the
>>   list, but it is at the moment the centralized "we're done with the
>>   sglist" function and none of the other side effects that occur in
>>   dma_buf_commit will interfere with the reset that occurs from
>>   ide_restart_bh, I think
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  hw/ide/ahci.c | 3 +--
>>  hw/ide/core.c | 1 +
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index 6defeed..f3438ad 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -919,6 +919,7 @@ static void ncq_err(NCQTransferState *ncq_tfs)
>>      ide_state->error = ABRT_ERR;
>>      ide_state->status = READY_STAT | ERR_STAT;
>>      ncq_tfs->drive->port_regs.scr_err |= (1 << ncq_tfs->tag);
>> +    qemu_sglist_destroy(&ncq_tfs->sglist);
>>      ncq_tfs->used = 0;
>>  }
>>
>> @@ -1025,7 +1026,6 @@ static void execute_ncq_command(NCQTransferState *ncq_tfs)
>>      default:
>>          DPRINTF(port, "error: unsupported NCQ command (0x%02x) received\n",
>>                  ncq_tfs->cmd);
>> -        qemu_sglist_destroy(&ncq_tfs->sglist);
>>          ncq_err(ncq_tfs);
>>      }
>>  }
>> @@ -1092,7 +1092,6 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
>>          error_report("ahci: PRDT length for NCQ command (0x%zx) "
>>                       "is smaller than the requested size (0x%zx)",
>>                       ncq_tfs->sglist.size, size);
>> -        qemu_sglist_destroy(&ncq_tfs->sglist);
>>          ncq_err(ncq_tfs);
>>          ahci_trigger_irq(ad->hba, ad, PORT_IRQ_OVERFLOW);
>>          return;
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index f9c8162..82d7f2a 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -822,6 +822,7 @@ static void ide_dma_cb(void *opaque, int ret)
>>          return;
>>      }
>>      if (ret < 0) {
>> +        dma_buf_commit(s, 0);

You may flog me for not replying to your V2, but I believe this cleanup 
needs to be in the early return case. Otherwise, we will perform cleanup 
twice in the 'IGNORE' case.

I thiiink we need to shimmy this down to before the return.

>>          if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s->dma_cmd))) {
>>              s->bus->dma->aiocb = NULL;
>>              return;

If the change looks good to you and seems to work for correcting the 
leak just as well, you may apply:

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH for-2.7 v3 03/36] qga: free the whole blacklist

[Markus Armbruster]

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

> Hi
>
> ----- Original Message -----
>> Marc-André Lureau <mlureau@redhat.com> writes:
>> 
>> > Hi
>> >
>> > ----- Original Message -----
>> >> Copying the QGA maintainer.
>> >> 
>> >> marcandre.lureau@redhat.com writes:
>> >> 
>> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >> >
>> >> > Free the list, not just the elements.
>> >> >
>> >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >> > ---
>> >> >  include/glib-compat.h | 9 +++++++++
>> >> >  qga/main.c            | 8 ++------
>> >> >  2 files changed, 11 insertions(+), 6 deletions(-)
>> >> >
>> >> > diff --git a/include/glib-compat.h b/include/glib-compat.h
>> >> > index 01aa7b3..6d643b2 100644
>> >> > --- a/include/glib-compat.h
>> >> > +++ b/include/glib-compat.h
>> >> > @@ -260,4 +260,13 @@ static inline void g_hash_table_add(GHashTable
>> >> > *hash_table, gpointer key)
>> >> >      } while (0)
>> >> >  #endif
>> >> >  
>> >> > +/*
>> >> > + * A GFunc function helper freeing the first argument (not part of
>> >> > glib)
>> >> 
>> >> Well, it's not part of GLib, because non-obsolete GLib has no use for
>> >> it!  You'd simply pass g_free directly to a _free_full() function
>> >> instead of passing a silly wrapper to a _foreach() function.
>> >> 
>> >> The commit does a bit more than just plug a leak, it also provides a new
>> >> helper for general use.  Mention in the commit message?
>> >> 
>> >> I wonder how many more of these silly g_free() wrappers we have.  A
>> >> quick grep reports hits in string-input-visitor.c and
>> >> qobject/json-streamer.c.  If you add one to a header, you get to hunt
>> >> them down :)
>> >> 
>> >> > + */
>> >> > +static inline void qemu_g_func_free(gpointer data,
>> >> > +                                    gpointer user_data)
>> >> > +{
>> >> > +    g_free(data);
>> >> > +}
>> >> > +
>> >> >  #endif
>> >> > diff --git a/qga/main.c b/qga/main.c
>> >> > index 4c3b2c7..868508b 100644
>> >> > --- a/qga/main.c
>> >> > +++ b/qga/main.c
>> >> > @@ -1175,6 +1175,8 @@ static void config_free(GAConfig *config)
>> >> >  #ifdef CONFIG_FSFREEZE
>> >> >      g_free(config->fsfreeze_hook);
>> >> >  #endif
>> >> > +    g_list_foreach(config->blacklist, qemu_g_func_free, NULL);
>> >> > +    g_list_free(config->blacklist);
>> >> 
>> >> Modern GLib code doesn't need silly wrappers:
>> >> 
>> >>     g_list_free_full(config->blacklist, g_free);
>> >> 
>> >> Unfortunately, this requires 2.28, and we're stull stuck at 2.22.
>> >> Please explain this in the commit message.
>> >> 
>> >> Even better, provide a replacement in glib-compat.h #if
>> >> !GLIB_CHECK_VERSION(2, 28, 0).  Will facilitate ditching the work-around
>> >> when we upgrade to 2.28.
>> >
>> > ok
>> >
>> >> 
>> >> >      g_free(config);
>> >> >  }
>> >> >  
>> >> > @@ -1310,11 +1312,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 +1376,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);
>> >> 
>> >> If you at least explain why not g_list_free_full() in the commit
>> >> message, you can add
>> >> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> >> 
>> >> But I'd like to encourage you to explore providing a replacement for
>> >> g_list_free_full().
>> >
>> > Such replacement would make use of a (GFunc) cast, glib implementation is
>> > calling g_list_foreach (list, (GFunc) free_func, NULL), but Eric didn't
>> > want to have such cast in qemu code. He suggested to have the static
>> > inline helper in
>> > https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06561.html
>> 
>> Pointlessly dirty:
>> 
>>     g_list_foreach(list, (GFunc)g_free, NULL)
>> 
>> Trade dirty for cumbersome:
>> 
>>     void free_wrapper(gpointer data, gpointer unused)
>>     {
>>         g_free(data)
>>     }
>> 
>>     g_list_foreach(list, free_wrapper, NULL);
>> 
>> But this is C.  In C, simple things are best done the simple way.  Even
>> when that means we don't get to show off how amazingly well we've been
>> educated on higher order functions:
>> 
>>     for (node = list; node; node = next) {
>>         next = node->next;
>>         g_free(node);
>>     }
>> 
>> Simple, stupid, and not dirty.
>
> If only we were paid to write more lines of code ;) Anyway, that's fine by me (it's work after all, I'll write elegant code for fun time ;)

Lisp is ---> that way.

At least the stupid loop is hidden away in glib-compat.h.  The actual
uses become neater, e.g.

    g_list_free_full(config->blacklist, g_free);

instead of

    g_list_foreach(config->blacklist, qemu_g_func_free, NULL);
    g_list_free(config->blacklist);

I think that's worth a bit of extra work in glib-compat.h.

>> Quote: "Note that it is considered perfectly acceptable to access
>> list->next directly."
>
> Funny that quote is only in GList, but GSList also documents and exposes the struct.

Re: [Qemu-devel] [PATCH for-2.7 v3 32/36] tests: add qtest_add_data_func_full

[Markus Armbruster]

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

> Hi
>
> ----- Original Message -----
>> marcandre.lureau@redhat.com writes:
>> 
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > Allows one to specify a destroy function for the test data.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  tests/libqtest.c | 15 +++++++++++++++
>> >  tests/libqtest.h |  7 ++++++-
>> >  2 files changed, 21 insertions(+), 1 deletion(-)
>> >
>> > 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);
>> 
>> Umm, are these function casts kosher?
>> 
>> I can't find documentation for g_test_add_vtable().  Got a pointer for
>> me?  Sure GLib 2.22 provides it?
>
> Yes, https://git.gnome.org/browse/glib/tree/glib/gtestutils.h?h=2.22.0#n214
>
> See also: https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg00073.html
>
>
>> 
>> > +#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..e4c9c39 100644
>> > --- a/tests/libqtest.h
>> > +++ b/tests/libqtest.h
>> > @@ -413,15 +413,20 @@ const char *qtest_get_arch(void);
>> >  void qtest_add_func(const char *str, void (*fn)(void));
>> >  
>> >  /**
>> > - * qtest_add_data_func:
>> > + * 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().

Recommend to mention that @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);
>> > +
>> >  void qtest_add_data_func(const char *str, const void *data,
>> >                           void (*fn)(const void *));
>> 
>> Please keep qtest_add_data_func() documented.
>
> Ok (I thought it was quite enough based on the _full description)

You could try something like

    /**
     * qtest_add_data_func and 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().
     */
    void qtest_add_data_func_full(const char *str, void *data,
                                  void (*fn)(const void *),
                                  GDestroyNotify data_free_func);
    void qtest_add_data_func(const char *str, const void *data,

GTK-Doc extraction wouldn't cope with it, but we're not using it[*].

Or something like

    /**
     * qtest_add_data_func:
     * @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().
     */
    void qtest_add_data_func_full(const char *str, void *data,
                                  void (*fn)(const void *),
                                  GDestroyNotify data_free_func);

    /* Like qtest_add_data_func_full() with a null last argument */
    void qtest_add_data_func(const char *str, const void *data,

Again, no GTK-Doc support, and again, I don't care.


[*] We only pay its price of admission: gratuitous waste of screen space
by repeating the obvious.