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

[Qemu-devel] [PATCH 00/37] 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.

The series is also available for convenience in
https://github.com/elmarco/qemu leak branch.

Marc-André Lureau (37):
  build-sys: use an override for CFLAGS filter
  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: disconnect peer when qemu_chr_free()
  char: free MuxDriver when closing
  tests: fix qom-test leaks
  pc: free i8259
  pci-bus: do not allocate and leak bsel
  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
  tests: free a bunch of qmp responses
  usb: free leaking path
  bus: simplify name handling
  tests: pc-cpu-test
  tests: fix rsp leak in postcopy-test
  ahci: fix sglist leak on retry
  tests: fix postcopy-test leaks

 hw/audio/gus.c                    |  9 ++++++---
 hw/audio/sb16.c                   |  4 +++-
 hw/block/fdc.c                    |  4 +++-
 hw/char/parallel.c                |  3 ++-
 hw/core/bus.c                     | 21 ++++++---------------
 hw/core/machine.c                 |  1 +
 hw/display/vga-isa.c              |  8 ++++++--
 hw/dma/i8257.c                    |  6 ++++--
 hw/i386/acpi-build.c              | 11 ++++-------
 hw/i386/pc.c                      |  9 +++++----
 hw/i386/pc_piix.c                 |  1 +
 hw/i386/pc_q35.c                  |  3 +++
 hw/ide/ahci.c                     |  3 +++
 hw/ide/core.c                     |  6 ++++--
 hw/input/pckbd.c                  |  4 ++--
 hw/input/virtio-input-hid.c       |  1 +
 hw/input/virtio-input.c           | 10 ++++++++++
 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/hw/boards.h               |  3 ++-
 include/hw/i386/pc.h              |  3 +--
 include/hw/ide/internal.h         |  2 ++
 include/hw/isa/i8257.h            |  2 ++
 include/hw/isa/isa.h              |  5 ++++-
 include/hw/pci/pci_bus.h          |  1 +
 include/hw/qdev-core.h            |  2 +-
 include/hw/virtio/virtio-input.h  |  1 +
 migration/qjson.c                 |  1 +
 numa.c                            | 15 ++++++++-------
 pc-bios/optionrom/Makefile        |  4 ++--
 qemu-char.c                       | 10 ++++++++++
 qga/guest-agent-command-state.c   |  6 ++++++
 qga/guest-agent-core.h            |  1 +
 qga/main.c                        | 13 +++++++------
 tests/check-qom-interface.c       |  1 +
 tests/check-qom-proplist.c        | 16 ++++++++++++++++
 tests/libqos/usb.c                |  1 +
 tests/pc-cpu-test.c               |  8 ++++++--
 tests/postcopy-test.c             |  9 ++++++---
 tests/pvpanic-test.c              |  1 +
 tests/qom-test.c                  |  5 +++--
 tests/test-cutils.c               | 24 ++++++++++++++++--------
 tests/test-filter-mirror.c        |  4 +++-
 tests/test-filter-redirector.c    |  8 ++++++--
 tests/test-io-channel-command.c   |  3 ++-
 tests/test-iov.c                  |  7 +++++++
 tests/test-qga.c                  |  5 +++++
 tests/test-string-input-visitor.c |  1 +
 tests/test-vmstate.c              |  8 ++++++--
 tests/virtio-blk-test.c           | 19 +++++++++++++------
 tests/virtio-net-test.c           | 10 +++++++---
 util/qdist.c                      |  3 ++-
 55 files changed, 246 insertions(+), 100 deletions(-)

-- 
2.9.0

[Qemu-devel] [PATCH 01/37] build-sys: use an override for CFLAGS filter

[marcandre.lureau]

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

Even if the user gave CFLAGS=... argument on make command line to
override the configure value, make sure the filter is applied.

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

diff --git a/pc-bios/optionrom/Makefile b/pc-bios/optionrom/Makefile
index d88ce11..fc9be45 100644
--- a/pc-bios/optionrom/Makefile
+++ b/pc-bios/optionrom/Makefile
@@ -24,8 +24,8 @@ 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))
+# Drop gcov, asan, and glib flags
+override CFLAGS := $(filter -O% -g%, $(CFLAGS))
 QEMU_INCLUDES += -I$(SRC_PATH)
 
 Wa = -Wa,
-- 
2.9.0

[Qemu-devel] [PATCH 02/37] 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>
---
 tests/test-qga.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/test-qga.c b/tests/test-qga.c
index 251b201..a23c348 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));
@@ -717,13 +718,17 @@ 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=%s", conf);
     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

Re: [Qemu-devel] [PATCH 02/37] tests: fix test-qga leaks

[Eric Blake]

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

On 07/19/2016 02:53 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/test-qga.c | 5 +++++
>  1 file changed, 5 insertions(+)

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


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


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

[Qemu-devel] [PATCH 03/37] 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>
---
 qga/main.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

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

Re: [Qemu-devel] [PATCH 03/37] qga: free the whole blacklist

[Eric Blake]

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

On 07/19/2016 02:53 AM, marcandre.lureau@redhat.com wrote:
> 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>
> ---
>  qga/main.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index 4c3b2c7..bb48214 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -1175,6 +1175,7 @@ static void config_free(GAConfig *config)
>  #ifdef CONFIG_FSFREEZE
>      g_free(config->fsfreeze_hook);
>  #endif
> +    g_list_free_full(config->blacklist, g_free);
>      g_free(config);

Won't work. We still allow glib 2.22, but g_list_free_full() was added
in 2.28. See also commit ba4dba5, where I had to open-code around the
lack of the relative g_queue_free_full().

You can, of course, do a prereq patch that supplies fallbacks for older
glib, but it may be easiest to just open-code the additional step of
g_list_free(config->blacklist)...

> @@ -1310,11 +1311,6 @@ static int run_agent(GAState *s, GAConfig *config)
>      return EXIT_SUCCESS;
>  }
>  
> -static void free_blacklist_entry(gpointer entry, gpointer unused)
> -{
> -    g_free(entry);
> -}
> -
>  int main(int argc, char **argv)
>  {
>      int ret = EXIT_SUCCESS;
> @@ -1379,7 +1375,6 @@ end:
>      if (s->channel) {
>          ga_channel_free(s->channel);
>      }
> -    g_list_foreach(config->blacklist, free_blacklist_entry, NULL);

...right here, without deleting any of the existing code.


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


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

[Qemu-devel] [PATCH 04/37] 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 | 6 ++++++
 qga/guest-agent-core.h          | 1 +
 qga/main.c                      | 6 ++++++
 3 files changed, 13 insertions(+)

diff --git a/qga/guest-agent-command-state.c b/qga/guest-agent-command-state.c
index 4de229c..e609d32 100644
--- a/qga/guest-agent-command-state.c
+++ b/qga/guest-agent-command-state.c
@@ -71,3 +71,9 @@ GACommandState *ga_command_state_new(void)
     cs->groups = NULL;
     return cs;
 }
+
+void ga_command_state_free(GACommandState *cs)
+{
+    g_slist_free_full(cs->groups, g_free);
+    g_free(cs);
+}
diff --git a/qga/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 bb48214..0b9d04e 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -1371,6 +1371,8 @@ int main(int argc, char **argv)
 end:
     if (s->command_state) {
         ga_command_state_cleanup_all(s->command_state);
+        ga_command_state_free(s->command_state);
+        json_message_parser_destroy(&s->parser);
     }
     if (s->channel) {
         ga_channel_free(s->channel);
@@ -1383,6 +1385,10 @@ end:
     }
 
     config_free(config);
+    if (s->main_loop) {
+        g_main_loop_unref(s->main_loop);
+    }
+    g_free(s);
 
     return ret;
 }
-- 
2.9.0

Re: [Qemu-devel] [PATCH 04/37] qga: free remaining leaking state

[Eric Blake]

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

On 07/19/2016 02:53 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qga/guest-agent-command-state.c | 6 ++++++
>  qga/guest-agent-core.h          | 1 +
>  qga/main.c                      | 6 ++++++
>  3 files changed, 13 insertions(+)
> 
> diff --git a/qga/guest-agent-command-state.c b/qga/guest-agent-command-state.c
> index 4de229c..e609d32 100644
> --- a/qga/guest-agent-command-state.c
> +++ b/qga/guest-agent-command-state.c
> @@ -71,3 +71,9 @@ GACommandState *ga_command_state_new(void)
>      cs->groups = NULL;
>      return cs;
>  }
> +
> +void ga_command_state_free(GACommandState *cs)
> +{
> +    g_slist_free_full(cs->groups, g_free);

Same problem as 3/37 - our minimum glib is 2.22, but this function is 2.28.

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


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

[Qemu-devel] [PATCH 05/37] 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>
---
 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

Re: [Qemu-devel] [PATCH 05/37] tests: fix test-cutils leaks

[Eric Blake]

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

On 07/19/2016 02:54 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Spotted thanks to ASAN.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/test-cutils.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 

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

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


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

[Qemu-devel] [PATCH 06/37] 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>
---
 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

Re: [Qemu-devel] [PATCH 06/37] tests: fix test-vmstate leaks

[Eric Blake]

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

On 07/19/2016 02:54 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Spotted thanks to ASAN.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/test-vmstate.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 

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

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


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

[Qemu-devel] [PATCH 07/37] 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>
---
 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

Re: [Qemu-devel] [PATCH 07/37] tests: fix test-iov leaks

[Eric Blake]

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

On 07/19/2016 02:54 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Spotted thanks to ASAN.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@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);

Calling exit() implicitly frees things, but I'm not opposed to the patch.

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


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


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

[Qemu-devel] [PATCH 08/37] qdist: fix entries memory leak

[marcandre.lureau]

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

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

Spotted thanks to ASAN.

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

Re: [Qemu-devel] [PATCH 08/37] qdist: fix entries memory leak

[Eric Blake]

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

On 07/19/2016 02:54 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> qdist_init() allocates of entries, make sure we don't leak it.

s/of //

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

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

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

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


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

[Qemu-devel] [PATCH 09/37] 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>
---
 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

Re: [Qemu-devel] [PATCH 09/37] tests: fix check-qom-interface leaks

[Eric Blake]

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

On 07/19/2016 02:54 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Found thanks to ASAN.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/check-qom-interface.c | 1 +
>  1 file changed, 1 insertion(+)

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

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

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


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

[Qemu-devel] [PATCH 10/37] 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>
---
 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

Re: [Qemu-devel] [PATCH 10/37] tests: fix check-qom-proplist leaks

[Eric Blake]

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

On 07/19/2016 02:54 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Found thanks to ASAN.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/check-qom-proplist.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 

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

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


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

[Qemu-devel] [PATCH 11/37] tests: fix small leak in test-io-channel-command

[marcandre.lureau]

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

srcfifo && dstfifo must still be freed in this case.

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

diff --git a/tests/test-io-channel-command.c b/tests/test-io-channel-command.c
index 1d1f461..95be370 100644
--- a/tests/test-io-channel-command.c
+++ b/tests/test-io-channel-command.c
@@ -40,7 +40,7 @@ static void test_io_channel_command_fifo(bool async)
 
     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) {
         abort();
@@ -59,6 +59,7 @@ static void test_io_channel_command_fifo(bool async)
     object_unref(OBJECT(src));
     object_unref(OBJECT(dst));
 
+end:
     g_free(srcfifo);
     g_free(dstfifo);
     unlink(TEST_FIFO);
-- 
2.9.0

Re: [Qemu-devel] [PATCH 11/37] tests: fix small leak in test-io-channel-command

[Eric Blake]

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

On 07/19/2016 02:54 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> srcfifo && dstfifo must still be freed in this case.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/test-io-channel-command.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/test-io-channel-command.c b/tests/test-io-channel-command.c
> index 1d1f461..95be370 100644
> --- a/tests/test-io-channel-command.c
> +++ b/tests/test-io-channel-command.c
> @@ -40,7 +40,7 @@ static void test_io_channel_command_fifo(bool async)
>  
>      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 we fail here...

>      if (mkfifo(TEST_FIFO, 0600) < 0) {

...then we don't create a fifo here...

>          abort();
> @@ -59,6 +59,7 @@ static void test_io_channel_command_fifo(bool async)
>      object_unref(OBJECT(src));
>      object_unref(OBJECT(dst));
>  
> +end:
>      g_free(srcfifo);
>      g_free(dstfifo);
>      unlink(TEST_FIFO);

...and unlink() will (hopefully) fail to unlink a missing file.  But in
the worst case, it unlinks someone else's file.  Probably worth being a
bit stricter about only undoing what you have already done.


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


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

Re: [Qemu-devel] [PATCH 11/37] tests: fix small leak in test-io-channel-command

[Marc-André Lureau]

Hi

----- Original Message -----
> On 07/19/2016 02:54 AM, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > srcfifo && dstfifo must still be freed in this case.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  tests/test-io-channel-command.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/test-io-channel-command.c
> > b/tests/test-io-channel-command.c
> > index 1d1f461..95be370 100644
> > --- a/tests/test-io-channel-command.c
> > +++ b/tests/test-io-channel-command.c
> > @@ -40,7 +40,7 @@ static void test_io_channel_command_fifo(bool async)
> >  
> >      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 we fail here...
> 
> >      if (mkfifo(TEST_FIFO, 0600) < 0) {
> 
> ...then we don't create a fifo here...
> 
> >          abort();
> > @@ -59,6 +59,7 @@ static void test_io_channel_command_fifo(bool async)
> >      object_unref(OBJECT(src));
> >      object_unref(OBJECT(dst));
> >  
> > +end:
> >      g_free(srcfifo);
> >      g_free(dstfifo);
> >      unlink(TEST_FIFO);
> 
> ...and unlink() will (hopefully) fail to unlink a missing file.  But in
> the worst case, it unlinks someone else's file.  Probably worth being a
> bit stricter about only undoing what you have already done.

But the test starts by unlinking unconditionally too, so not sure it's really worth.

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

Re: [Qemu-devel] [PATCH 11/37] tests: fix small leak in test-io-channel-command

[Marc-André Lureau]

Hi

On Wed, Jul 20, 2016 at 1:16 AM, Marc-André Lureau <mlureau@redhat.com> wrote:
> Hi
>
> ----- Original Message -----
>> On 07/19/2016 02:54 AM, marcandre.lureau@redhat.com wrote:
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > srcfifo && dstfifo must still be freed in this case.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  tests/test-io-channel-command.c | 3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/tests/test-io-channel-command.c
>> > b/tests/test-io-channel-command.c
>> > index 1d1f461..95be370 100644
>> > --- a/tests/test-io-channel-command.c
>> > +++ b/tests/test-io-channel-command.c
>> > @@ -40,7 +40,7 @@ static void test_io_channel_command_fifo(bool async)
>> >
>> >      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 we fail here...
>>
>> >      if (mkfifo(TEST_FIFO, 0600) < 0) {
>>
>> ...then we don't create a fifo here...
>>
>> >          abort();
>> > @@ -59,6 +59,7 @@ static void test_io_channel_command_fifo(bool async)
>> >      object_unref(OBJECT(src));
>> >      object_unref(OBJECT(dst));
>> >
>> > +end:
>> >      g_free(srcfifo);
>> >      g_free(dstfifo);
>> >      unlink(TEST_FIFO);
>>
>> ...and unlink() will (hopefully) fail to unlink a missing file.  But in
>> the worst case, it unlinks someone else's file.  Probably worth being a
>> bit stricter about only undoing what you have already done.
>
> But the test starts by unlinking unconditionally too, so not sure it's really worth.

I fixed the test by using mkdtemp() instead, which also permits
running tests concurrently.

-- 
Marc-André Lureau

[Qemu-devel] [PATCH 12/37] 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>
---
 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

Re: [Qemu-devel] [PATCH 12/37] tests: fix leak in test-string-input-visitor

[Eric Blake]

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

On 07/19/2016 02:54 AM, marcandre.lureau@redhat.com wrote:
> 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>
> ---
>  tests/test-string-input-visitor.c | 1 +
>  1 file changed, 1 insertion(+)

Hmm, I thought I'd fixed that under valgrind previously.  But browsing
logs, I see where I touched other visitor tests but not this one.  :)

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

> 
> 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);
> 

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


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

[Qemu-devel] [PATCH 13/37] 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>
---
 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 f2d131b..d042a94 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2603,10 +2603,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 14/37] 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>
---
 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

Re: [Qemu-devel] [PATCH 14/37] numa: do not leak NumaOptions

[Eric Blake]

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

On 07/19/2016 02:54 AM, marcandre.lureau@redhat.com wrote:
> 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>
> ---
>  numa.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 

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

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


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

[Qemu-devel] [PATCH 15/37] 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>
---
 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 1b8baa8..57b09c9 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -533,9 +533,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 = {
@@ -1547,7 +1547,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");
@@ -1560,7 +1560,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 e38c95a..fad0873 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -216,7 +216,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

Re: [Qemu-devel] [PATCH 15/37] pc: simplify passing qemu_irq

[Eric Blake]

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

On 07/19/2016 02:54 AM, marcandre.lureau@redhat.com wrote:
> 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>
> ---
>  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 1b8baa8..57b09c9 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -533,9 +533,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)

qemu_irq type naming goes against our normal conventions.  But this
isn't the series to change that.

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

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


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

[Qemu-devel] [PATCH 16/37] 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>
---
 hw/i386/pc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 57b09c9..a20ab58 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1561,6 +1561,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

Re: [Qemu-devel] [PATCH 16/37] pc: don't leak a20_line

[Eric Blake]

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

On 07/19/2016 02:54 AM, marcandre.lureau@redhat.com wrote:
> 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>
> ---
>  hw/i386/pc.c | 1 +
>  1 file changed, 1 insertion(+)
> 

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

> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 57b09c9..a20ab58 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1561,6 +1561,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);
>  
> 

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


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

[Qemu-devel] [PATCH 17/37] 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 2fe6ff6..07a7a97 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -558,6 +558,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 fad0873..54e0341 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -889,7 +889,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 18/37] 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>
---
 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 fbba461..f4ba3a4 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -761,7 +761,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;
 
@@ -785,7 +785,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

Re: [Qemu-devel] [PATCH 18/37] acpi-build: fix array leak

[Marcel Apfelbaum]

On 07/19/2016 11:54 AM, marcandre.lureau@redhat.com wrote:
> 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,

Right. If I understand, this is the leak.

  however, it shouldn't free the elements themself.

And it didn't, right? otherwise it would not work since these ranges
are used later.

>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@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 fbba461..f4ba3a4 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -761,7 +761,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();

Indeed, we are not going to free the ranges in this array, adding the GDestroyNotify
here is not needed.

>       uint64_t free_base = start;
>       int i;
>
> @@ -785,7 +785,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);

This *is* scary since "true" means delete everything, but looking at documentation:
     "If array contents point to dynamically-allocated memory,
      they should be freed separately if free_seg is TRUE and
      no GDestroyNotify function has been set for array."
So your approach should work.

I think I understand the leak. Previous approach deleted the GArray wrapper,
preserved the pointers (which we need), but also the inner array which we don't.

One question: how did you test that it still works :) ?
Did you run something like -device pxb,id=pxb,bus_nr=0x80,bus=pci.0 -device e1000,bus=pxb and see the device
e100 device gets the required resources?


Thanks,
Marcel

>   }
>
>   /*
>

Re: [Qemu-devel] [PATCH 18/37] acpi-build: fix array leak

[Marc-André Lureau]

Hi

On Thu, Jul 21, 2016 at 6:52 PM, Marcel Apfelbaum
<marcel.apfelbaum@gmail.com> wrote:
> On 07/19/2016 11:54 AM, marcandre.lureau@redhat.com wrote:
>>
>> 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,
>
>
> Right. If I understand, this is the leak.
>
>  however, it shouldn't free the elements themself.
>
> And it didn't, right? otherwise it would not work since these ranges
> are used later.
>
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@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 fbba461..f4ba3a4 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -761,7 +761,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();
>
>
> Indeed, we are not going to free the ranges in this array, adding the
> GDestroyNotify
> here is not needed.
>
>>       uint64_t free_base = start;
>>       int i;
>>
>> @@ -785,7 +785,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);
>
>
> This *is* scary since "true" means delete everything, but looking at
> documentation:
>     "If array contents point to dynamically-allocated memory,
>      they should be freed separately if free_seg is TRUE and
>      no GDestroyNotify function has been set for array."
> So your approach should work.
>
> I think I understand the leak. Previous approach deleted the GArray wrapper,
> preserved the pointers (which we need), but also the inner array which we
> don't.

yes, it's only the inner array we need to free.

>
> One question: how did you test that it still works :) ?
> Did you run something like -device pxb,id=pxb,bus_nr=0x80,bus=pci.0 -device
> e1000,bus=pxb and see the device
> e100 device gets the required resources?

If you run this under it valgrind you get, after the patch the leak is gone:

==20313== 32 bytes in 1 blocks are definitely lost in loss record
5,326 of 10,190
==20313==    at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
==20313==    by 0x1E16AE58: g_malloc (in /usr/lib64/libglib-2.0.so.0.4800.1)
==20313==    by 0x1E181D42: g_slice_alloc (in
/usr/lib64/libglib-2.0.so.0.4800.1)
==20313==    by 0x1E139880: g_ptr_array_sized_new (in
/usr/lib64/libglib-2.0.so.0.4800.1)
==20313==    by 0x1E13991D: g_ptr_array_new_with_free_func (in
/usr/lib64/libglib-2.0.so.0.4800.1)
==20313==    by 0x3E8BE8: crs_range_merge (acpi-build.c:797)
==20313==    by 0x3E8FE6: build_crs (acpi-build.c:918)
==20313==    by 0x3ED857: build_dsdt (acpi-build.c:2014)
==20313==    by 0x3EF659: acpi_build (acpi-build.c:2590)
==20313==    by 0x3EFE61: acpi_setup (acpi-build.c:2793)
==20313==    by 0x3D810D: pc_machine_done (pc.c:1270)
==20313==    by 0x7E6A23: notifier_list_notify (notify.c:40)



>
>
> Thanks,
> Marcel
>
>>   }
>>
>>   /*
>>
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 18/37] acpi-build: fix array leak

[Marcel Apfelbaum]

On 07/21/2016 06:48 PM, Marc-André Lureau wrote:
> Hi
>
> On Thu, Jul 21, 2016 at 6:52 PM, Marcel Apfelbaum
> <marcel.apfelbaum@gmail.com> wrote:
>> On 07/19/2016 11:54 AM, marcandre.lureau@redhat.com wrote:
>>>
>>> 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,
>>
>>
>> Right. If I understand, this is the leak.
>>
>>   however, it shouldn't free the elements themself.
>>
>> And it didn't, right? otherwise it would not work since these ranges
>> are used later.
>>
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@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 fbba461..f4ba3a4 100644
>>> --- a/hw/i386/acpi-build.c
>>> +++ b/hw/i386/acpi-build.c
>>> @@ -761,7 +761,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();
>>
>>
>> Indeed, we are not going to free the ranges in this array, adding the
>> GDestroyNotify
>> here is not needed.
>>
>>>        uint64_t free_base = start;
>>>        int i;
>>>
>>> @@ -785,7 +785,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);
>>
>>
>> This *is* scary since "true" means delete everything, but looking at
>> documentation:
>>      "If array contents point to dynamically-allocated memory,
>>       they should be freed separately if free_seg is TRUE and
>>       no GDestroyNotify function has been set for array."
>> So your approach should work.
>>
>> I think I understand the leak. Previous approach deleted the GArray wrapper,
>> preserved the pointers (which we need), but also the inner array which we
>> don't.
>
> yes, it's only the inner array we need to free.
>
>>
>> One question: how did you test that it still works :) ?
>> Did you run something like -device pxb,id=pxb,bus_nr=0x80,bus=pci.0 -device
>> e1000,bus=pxb and see the device
>> e100 device gets the required resources?
>
> If you run this under it valgrind you get, after the patch the leak is gone:
>
> ==20313== 32 bytes in 1 blocks are definitely lost in loss record
> 5,326 of 10,190
> ==20313==    at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
> ==20313==    by 0x1E16AE58: g_malloc (in /usr/lib64/libglib-2.0.so.0.4800.1)
> ==20313==    by 0x1E181D42: g_slice_alloc (in
> /usr/lib64/libglib-2.0.so.0.4800.1)
> ==20313==    by 0x1E139880: g_ptr_array_sized_new (in
> /usr/lib64/libglib-2.0.so.0.4800.1)
> ==20313==    by 0x1E13991D: g_ptr_array_new_with_free_func (in
> /usr/lib64/libglib-2.0.so.0.4800.1)
> ==20313==    by 0x3E8BE8: crs_range_merge (acpi-build.c:797)
> ==20313==    by 0x3E8FE6: build_crs (acpi-build.c:918)
> ==20313==    by 0x3ED857: build_dsdt (acpi-build.c:2014)
> ==20313==    by 0x3EF659: acpi_build (acpi-build.c:2590)
> ==20313==    by 0x3EFE61: acpi_setup (acpi-build.c:2793)
> ==20313==    by 0x3D810D: pc_machine_done (pc.c:1270)
> ==20313==    by 0x7E6A23: notifier_list_notify (notify.c:40)
>
>
>

I am sure the leak is gone :), but the devices attached to PXB still work?
I'll test it and get back to you.

Thanks,
Marcel



>>
>>
>> Thanks,
>> Marcel
>>
>>>    }
>>>
>>>    /*
>>>
>>
>>
>
>
>

Re: [Qemu-devel] [PATCH 18/37] acpi-build: fix array leak

[Marcel Apfelbaum]

On 07/21/2016 06:51 PM, Marcel Apfelbaum wrote:
> On 07/21/2016 06:48 PM, Marc-André Lureau wrote:
>> Hi
>>
>> On Thu, Jul 21, 2016 at 6:52 PM, Marcel Apfelbaum
>> <marcel.apfelbaum@gmail.com> wrote:
>>> On 07/19/2016 11:54 AM, marcandre.lureau@redhat.com wrote:
>>>>
>>>> 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,
>>>
>>>
>>> Right. If I understand, this is the leak.
>>>
>>>   however, it shouldn't free the elements themself.
>>>
>>> And it didn't, right? otherwise it would not work since these ranges
>>> are used later.
>>>
>>>>
>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@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 fbba461..f4ba3a4 100644
>>>> --- a/hw/i386/acpi-build.c
>>>> +++ b/hw/i386/acpi-build.c
>>>> @@ -761,7 +761,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();
>>>
>>>
>>> Indeed, we are not going to free the ranges in this array, adding the
>>> GDestroyNotify
>>> here is not needed.
>>>
>>>>        uint64_t free_base = start;
>>>>        int i;
>>>>
>>>> @@ -785,7 +785,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);
>>>
>>>
>>> This *is* scary since "true" means delete everything, but looking at
>>> documentation:
>>>      "If array contents point to dynamically-allocated memory,
>>>       they should be freed separately if free_seg is TRUE and
>>>       no GDestroyNotify function has been set for array."
>>> So your approach should work.
>>>
>>> I think I understand the leak. Previous approach deleted the GArray wrapper,
>>> preserved the pointers (which we need), but also the inner array which we
>>> don't.
>>
>> yes, it's only the inner array we need to free.
>>
>>>
>>> One question: how did you test that it still works :) ?
>>> Did you run something like -device pxb,id=pxb,bus_nr=0x80,bus=pci.0 -device
>>> e1000,bus=pxb and see the device
>>> e100 device gets the required resources?
>>
>> If you run this under it valgrind you get, after the patch the leak is gone:
>>
>> ==20313== 32 bytes in 1 blocks are definitely lost in loss record
>> 5,326 of 10,190
>> ==20313==    at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
>> ==20313==    by 0x1E16AE58: g_malloc (in /usr/lib64/libglib-2.0.so.0.4800.1)
>> ==20313==    by 0x1E181D42: g_slice_alloc (in
>> /usr/lib64/libglib-2.0.so.0.4800.1)
>> ==20313==    by 0x1E139880: g_ptr_array_sized_new (in
>> /usr/lib64/libglib-2.0.so.0.4800.1)
>> ==20313==    by 0x1E13991D: g_ptr_array_new_with_free_func (in
>> /usr/lib64/libglib-2.0.so.0.4800.1)
>> ==20313==    by 0x3E8BE8: crs_range_merge (acpi-build.c:797)
>> ==20313==    by 0x3E8FE6: build_crs (acpi-build.c:918)
>> ==20313==    by 0x3ED857: build_dsdt (acpi-build.c:2014)
>> ==20313==    by 0x3EF659: acpi_build (acpi-build.c:2590)
>> ==20313==    by 0x3EFE61: acpi_setup (acpi-build.c:2793)
>> ==20313==    by 0x3D810D: pc_machine_done (pc.c:1270)
>> ==20313==    by 0x7E6A23: notifier_list_notify (notify.c:40)
>>
>>
>>
>
> I am sure the leak is gone :), but the devices attached to PXB still work?
> I'll test it and get back to you.

Everything works, thank you!

Tested-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>


>
> Thanks,
> Marcel
>
>
>
>>>
>>>
>>> Thanks,
>>> Marcel
>>>
>>>>    }
>>>>
>>>>    /*
>>>>
>>>
>>>
>>
>>
>>
>

-- 

[Qemu-devel] [PATCH 19/37] char: disconnect peer when qemu_chr_free()

[marcandre.lureau]

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

chr_disconnect callback is used to cleanup server socket peer. This
removes some leaks when the chardev is freed.

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

diff --git a/qemu-char.c b/qemu-char.c
index e4b8448..6ed6dd6 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -4031,6 +4031,7 @@ static void qemu_chr_free_common(CharDriverState *chr)
 
 void qemu_chr_free(CharDriverState *chr)
 {
+    qemu_chr_disconnect(chr);
     if (chr->chr_close) {
         chr->chr_close(chr);
     }
-- 
2.9.0

[Qemu-devel] [PATCH 20/37] 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>
---
 qemu-char.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/qemu-char.c b/qemu-char.c
index 6ed6dd6..e276485 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -786,6 +786,14 @@ 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;
+
+    /* is more cleanup needed? */
+    g_free(d);
+}
+
 static CharDriverState *qemu_chr_open_mux(const char *id,
                                           ChardevBackend *backend,
                                           ChardevReturn *ret, Error **errp)
@@ -810,6 +818,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 21/37] 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>
---
 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

Re: [Qemu-devel] [PATCH 21/37] tests: fix qom-test leaks

[Eric Blake]

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

On 07/19/2016 02:54 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/qom-test.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

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

> 
> 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);
>          }
>      }
>  
> 

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


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

[Qemu-devel] [PATCH 22/37] 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>
---
 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

Re: [Qemu-devel] [PATCH 22/37] pc: free i8259

[Marcel Apfelbaum]

On 07/19/2016 11:54 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Simiarly to 2ba154cf4eb8636cdd3aa90f392ca9e77206ca39
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@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");
>      }
>

Hi,
It seems  my previous reply didn't make it to the mailing list.

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

[Qemu-devel] [PATCH 23/37] pci-bus: do not allocate and leak bsel

[marcandre.lureau]

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

Use a PCIBus attribute to store the bsel value, instead of allocating
and leaking it elsewhere.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/i386/acpi-build.c     | 7 ++-----
 include/hw/pci/pci_bus.h | 1 +
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index f4ba3a4..c1e7d93 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -428,14 +428,11 @@ build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms)
 static void *acpi_set_bsel(PCIBus *bus, void *opaque)
 {
     unsigned *bsel_alloc = opaque;
-    unsigned *bus_bsel;
 
     if (qbus_is_hotpluggable(BUS(bus))) {
-        bus_bsel = g_malloc(sizeof *bus_bsel);
-
-        *bus_bsel = (*bsel_alloc)++;
+        bus->bus_bsel = (*bsel_alloc)++;
         object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
-                                       bus_bsel, NULL);
+                                       &bus->bus_bsel, NULL);
     }
 
     return bsel_alloc;
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index 5484a9b..686cc7c 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -41,6 +41,7 @@ struct PCIBus {
     int *irq_count;
 
     Notifier machine_done;
+    unsigned bus_bsel;
 };
 
 typedef struct PCIBridgeWindows PCIBridgeWindows;
-- 
2.9.0

[Qemu-devel] [PATCH 24/37] 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   | 1 +
 hw/i386/pc_q35.c    | 1 +
 include/hw/boards.h | 1 +
 3 files changed, 3 insertions(+)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a07dc81..b2db274 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -190,6 +190,7 @@ static void pc_init1(MachineState *machine,
     } else {
         gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
     }
+    machine->gsi = gsi;
 
     if (pcmc->pci_enabled) {
         pci_bus = i440fx_init(host_type,
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index c5e8367..5dfb14f 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -158,6 +158,7 @@ static void pc_q35_init(MachineState *machine)
     } else {
         gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
     }
+    machine->gsi = gsi;
 
     /* create pci host bus */
     q35_host = Q35_HOST_DEVICE(qdev_create(NULL, TYPE_Q35_HOST_DEVICE));
diff --git a/include/hw/boards.h b/include/hw/boards.h
index e46a744..289ba52 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -139,6 +139,7 @@ struct MachineState {
     /*< private >*/
     Object parent_obj;
     Notifier sysbus_notifier;
+    qemu_irq *gsi;
 
     /*< public >*/
 
-- 
2.9.0

Re: [Qemu-devel] [PATCH 24/37] pc: keep gsi reference

[Eduardo Habkost]

On Tue, Jul 19, 2016 at 12:54:19PM +0400, marcandre.lureau@redhat.com wrote:
> 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>

Is there a way to make ASAN happy without having to add a field
to MachineState that we're not going to use for anything?

> ---
>  hw/i386/pc_piix.c   | 1 +
>  hw/i386/pc_q35.c    | 1 +
>  include/hw/boards.h | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index a07dc81..b2db274 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -190,6 +190,7 @@ static void pc_init1(MachineState *machine,
>      } else {
>          gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
>      }
> +    machine->gsi = gsi;
>  
>      if (pcmc->pci_enabled) {
>          pci_bus = i440fx_init(host_type,
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index c5e8367..5dfb14f 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -158,6 +158,7 @@ static void pc_q35_init(MachineState *machine)
>      } else {
>          gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
>      }
> +    machine->gsi = gsi;
>  
>      /* create pci host bus */
>      q35_host = Q35_HOST_DEVICE(qdev_create(NULL, TYPE_Q35_HOST_DEVICE));
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index e46a744..289ba52 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -139,6 +139,7 @@ struct MachineState {
>      /*< private >*/
>      Object parent_obj;
>      Notifier sysbus_notifier;
> +    qemu_irq *gsi;
>  
>      /*< public >*/
>  
> -- 
> 2.9.0
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 24/37] pc: keep gsi reference

[Marc-André Lureau]

Hi

----- Original Message -----
> On Tue, Jul 19, 2016 at 12:54:19PM +0400, marcandre.lureau@redhat.com wrote:
> > 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>
> 
> Is there a way to make ASAN happy without having to add a field
> to MachineState that we're not going to use for anything?

Well, the plan is rather to release it when no longer needed. Would it be fine to call qemu_free_irqs() in machine_finalize()?

> 
> > ---
> >  hw/i386/pc_piix.c   | 1 +
> >  hw/i386/pc_q35.c    | 1 +
> >  include/hw/boards.h | 1 +
> >  3 files changed, 3 insertions(+)
> > 
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index a07dc81..b2db274 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -190,6 +190,7 @@ static void pc_init1(MachineState *machine,
> >      } else {
> >          gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
> >      }
> > +    machine->gsi = gsi;
> >  
> >      if (pcmc->pci_enabled) {
> >          pci_bus = i440fx_init(host_type,
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index c5e8367..5dfb14f 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -158,6 +158,7 @@ static void pc_q35_init(MachineState *machine)
> >      } else {
> >          gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
> >      }
> > +    machine->gsi = gsi;
> >  
> >      /* create pci host bus */
> >      q35_host = Q35_HOST_DEVICE(qdev_create(NULL, TYPE_Q35_HOST_DEVICE));
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index e46a744..289ba52 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -139,6 +139,7 @@ struct MachineState {
> >      /*< private >*/
> >      Object parent_obj;
> >      Notifier sysbus_notifier;
> > +    qemu_irq *gsi;
> >  
> >      /*< public >*/
> >  
> > --
> > 2.9.0
> > 
> > 
> 
> --
> Eduardo
> 

Re: [Qemu-devel] [PATCH 24/37] pc: keep gsi reference

[Eduardo Habkost]

On Thu, Jul 21, 2016 at 01:27:35PM -0400, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > On Tue, Jul 19, 2016 at 12:54:19PM +0400, marcandre.lureau@redhat.com wrote:
> > > 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>
> > 
> > Is there a way to make ASAN happy without having to add a field
> > to MachineState that we're not going to use for anything?
> 
> Well, the plan is rather to release it when no longer needed.
> Would it be fine to call qemu_free_irqs() in
> machine_finalize()?

It would be fine, I guess, but it looks pointless if we have lots
of other resources allocated during PC machine initialization
that are never released.

But, see additional comment below:

> 
> > 
> > > ---
> > >  hw/i386/pc_piix.c   | 1 +
> > >  hw/i386/pc_q35.c    | 1 +
> > >  include/hw/boards.h | 1 +
> > >  3 files changed, 3 insertions(+)
> > > 
> > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > index a07dc81..b2db274 100644
> > > --- a/hw/i386/pc_piix.c
> > > +++ b/hw/i386/pc_piix.c
> > > @@ -190,6 +190,7 @@ static void pc_init1(MachineState *machine,
> > >      } else {
> > >          gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
> > >      }
> > > +    machine->gsi = gsi;
> > >  
> > >      if (pcmc->pci_enabled) {
> > >          pci_bus = i440fx_init(host_type,
> > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > > index c5e8367..5dfb14f 100644
> > > --- a/hw/i386/pc_q35.c
> > > +++ b/hw/i386/pc_q35.c
> > > @@ -158,6 +158,7 @@ static void pc_q35_init(MachineState *machine)
> > >      } else {
> > >          gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
> > >      }
> > > +    machine->gsi = gsi;
> > >  
> > >      /* create pci host bus */
> > >      q35_host = Q35_HOST_DEVICE(qdev_create(NULL, TYPE_Q35_HOST_DEVICE));
> > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > index e46a744..289ba52 100644
> > > --- a/include/hw/boards.h
> > > +++ b/include/hw/boards.h
> > > @@ -139,6 +139,7 @@ struct MachineState {
> > >      /*< private >*/
> > >      Object parent_obj;
> > >      Notifier sysbus_notifier;
> > > +    qemu_irq *gsi;

If this is used only by PC, doesn't it belong to PCMachineState?

Anyway, the new field would be very useful to help reduce the
number of parameters of PC initialization functions (by making
them just get a PCMachineState* argument). I would go even
further and remove the local 'gsi' variable and replace it with
'pcms->gsi' everywhere.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 24/37] pc: keep gsi reference

[Marc-André Lureau]

Hi

----- Original Message -----
> On Thu, Jul 21, 2016 at 01:27:35PM -0400, Marc-André Lureau wrote:
> > Hi
> > 
> > ----- Original Message -----
> > > On Tue, Jul 19, 2016 at 12:54:19PM +0400, marcandre.lureau@redhat.com
> > > wrote:
> > > > 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>
> > > 
> > > Is there a way to make ASAN happy without having to add a field
> > > to MachineState that we're not going to use for anything?
> > 
> > Well, the plan is rather to release it when no longer needed.
> > Would it be fine to call qemu_free_irqs() in
> > machine_finalize()?
> 
> It would be fine, I guess, but it looks pointless if we have lots
> of other resources allocated during PC machine initialization
> that are never released.

The main point, right now, is to have no direct leaks when running ASAN or valgrind, as they hide new introduced leaks that may be much worse. (it would also be good if we had no indirect leaks either, as this may also grow over time)

> But, see additional comment below:
> 
> > 
> > > 
> > > > ---
> > > >  hw/i386/pc_piix.c   | 1 +
> > > >  hw/i386/pc_q35.c    | 1 +
> > > >  include/hw/boards.h | 1 +
> > > >  3 files changed, 3 insertions(+)
> > > > 
> > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > > index a07dc81..b2db274 100644
> > > > --- a/hw/i386/pc_piix.c
> > > > +++ b/hw/i386/pc_piix.c
> > > > @@ -190,6 +190,7 @@ static void pc_init1(MachineState *machine,
> > > >      } else {
> > > >          gsi = qemu_allocate_irqs(gsi_handler, gsi_state,
> > > >          GSI_NUM_PINS);
> > > >      }
> > > > +    machine->gsi = gsi;
> > > >  
> > > >      if (pcmc->pci_enabled) {
> > > >          pci_bus = i440fx_init(host_type,
> > > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > > > index c5e8367..5dfb14f 100644
> > > > --- a/hw/i386/pc_q35.c
> > > > +++ b/hw/i386/pc_q35.c
> > > > @@ -158,6 +158,7 @@ static void pc_q35_init(MachineState *machine)
> > > >      } else {
> > > >          gsi = qemu_allocate_irqs(gsi_handler, gsi_state,
> > > >          GSI_NUM_PINS);
> > > >      }
> > > > +    machine->gsi = gsi;
> > > >  
> > > >      /* create pci host bus */
> > > >      q35_host = Q35_HOST_DEVICE(qdev_create(NULL,
> > > >      TYPE_Q35_HOST_DEVICE));
> > > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > > index e46a744..289ba52 100644
> > > > --- a/include/hw/boards.h
> > > > +++ b/include/hw/boards.h
> > > > @@ -139,6 +139,7 @@ struct MachineState {
> > > >      /*< private >*/
> > > >      Object parent_obj;
> > > >      Notifier sysbus_notifier;
> > > > +    qemu_irq *gsi;
> 
> If this is used only by PC, doesn't it belong to PCMachineState?

right, i'll try to put it there

> Anyway, the new field would be very useful to help reduce the
> number of parameters of PC initialization functions (by making
> them just get a PCMachineState* argument). I would go even

Which functions do you have in mind?

> further and remove the local 'gsi' variable and replace it with
> 'pcms->gsi' everywhere.

ok, why not.

Re: [Qemu-devel] [PATCH 24/37] pc: keep gsi reference

[Eduardo Habkost]

On Thu, Jul 21, 2016 at 02:28:33PM -0400, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > On Thu, Jul 21, 2016 at 01:27:35PM -0400, Marc-André Lureau wrote:
> > > Hi
> > > 
> > > ----- Original Message -----
> > > > On Tue, Jul 19, 2016 at 12:54:19PM +0400, marcandre.lureau@redhat.com
> > > > wrote:
> > > > > 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>
> > > > 
> > > > Is there a way to make ASAN happy without having to add a field
> > > > to MachineState that we're not going to use for anything?
> > > 
> > > Well, the plan is rather to release it when no longer needed.
> > > Would it be fine to call qemu_free_irqs() in
> > > machine_finalize()?
> > 
> > It would be fine, I guess, but it looks pointless if we have lots
> > of other resources allocated during PC machine initialization
> > that are never released.
> 
> The main point, right now, is to have no direct leaks when
> running ASAN or valgrind, as they hide new introduced leaks
> that may be much worse. (it would also be good if we had no
> indirect leaks either, as this may also grow over time)

I see. And you don't need to release it on finalize to reach that
goal, right? In this case I don't think we need the extra work.

(In case my previous message was unclear, I believe the field
will be useful, even if we don't release anything on finalize.)

> 
> > But, see additional comment below:
> > 
> > > 
> > > > 
> > > > > ---
> > > > >  hw/i386/pc_piix.c   | 1 +
> > > > >  hw/i386/pc_q35.c    | 1 +
> > > > >  include/hw/boards.h | 1 +
> > > > >  3 files changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > > > index a07dc81..b2db274 100644
> > > > > --- a/hw/i386/pc_piix.c
> > > > > +++ b/hw/i386/pc_piix.c
> > > > > @@ -190,6 +190,7 @@ static void pc_init1(MachineState *machine,
> > > > >      } else {
> > > > >          gsi = qemu_allocate_irqs(gsi_handler, gsi_state,
> > > > >          GSI_NUM_PINS);
> > > > >      }
> > > > > +    machine->gsi = gsi;
> > > > >  
> > > > >      if (pcmc->pci_enabled) {
> > > > >          pci_bus = i440fx_init(host_type,
> > > > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > > > > index c5e8367..5dfb14f 100644
> > > > > --- a/hw/i386/pc_q35.c
> > > > > +++ b/hw/i386/pc_q35.c
> > > > > @@ -158,6 +158,7 @@ static void pc_q35_init(MachineState *machine)
> > > > >      } else {
> > > > >          gsi = qemu_allocate_irqs(gsi_handler, gsi_state,
> > > > >          GSI_NUM_PINS);
> > > > >      }
> > > > > +    machine->gsi = gsi;
> > > > >  
> > > > >      /* create pci host bus */
> > > > >      q35_host = Q35_HOST_DEVICE(qdev_create(NULL,
> > > > >      TYPE_Q35_HOST_DEVICE));
> > > > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > > > index e46a744..289ba52 100644
> > > > > --- a/include/hw/boards.h
> > > > > +++ b/include/hw/boards.h
> > > > > @@ -139,6 +139,7 @@ struct MachineState {
> > > > >      /*< private >*/
> > > > >      Object parent_obj;
> > > > >      Notifier sysbus_notifier;
> > > > > +    qemu_irq *gsi;
> > 
> > If this is used only by PC, doesn't it belong to PCMachineState?
> 
> right, i'll try to put it there
> 
> > Anyway, the new field would be very useful to help reduce the
> > number of parameters of PC initialization functions (by making
> > them just get a PCMachineState* argument). I would go even
> 
> Which functions do you have in mind?

i440fx_init(), pc_basic_device_init(), maybe others.

> 
> > further and remove the local 'gsi' variable and replace it with
> > 'pcms->gsi' everywhere.
> 
> ok, why not.

-- 
Eduardo

[Qemu-devel] [PATCH 25/37] 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>
---
 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

Re: [Qemu-devel] [PATCH 25/37] ahci: free irqs array

[John Snow]

On 07/19/2016 04:54 AM, marcandre.lureau@redhat.com wrote:
> 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>
> ---
>  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)
> 

Oh, we pass these by-value, so yes.

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

And if this entire series gets picked up, have an ACK to mean "I don't
care who stages this:"

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

[Qemu-devel] [PATCH 26/37] 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>
---
 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

Re: [Qemu-devel] [PATCH 26/37] sd: free timer

[Andrew Baumann]

> From: Marc-André Lureau [mailto:marcandre.lureau@gmail.com]
> Sent: Thursday, 21 July 2016 4:15
> Hi Andrew,
> 
> Since you introduced the timer, could you review this patch?
> 
> thanks
> 
> 
> ---------- Forwarded message ----------
> From:  <marcandre.lureau@redhat.com>
> Date: Tue, Jul 19, 2016 at 12:54 PM
> Subject: [Qemu-devel] [PATCH 26/37] sd: free timer
> To: qemu-devel@nongnu.org
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> 
> 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>
> ---
>  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)

Thanks for the fix. This was based on some other timer code I found in the tree that was evidently also leaky (I don't remember where unfortunately).

One thing: are you sure it is safe to call timer_del() again if the timer may already have been deleted? It looks that way from the implementation, but the header comment isn't explicit.

Otherwise,
Reviewed-by: Andrew Baumann <Andrew.Baumann@microsoft.com>

Cheers,
Andrew

Re: [Qemu-devel] [PATCH 26/37] sd: free timer

[Marc-André Lureau]

Hi

On Thu, Jul 21, 2016 at 8:53 PM, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:
> Thanks for the fix. This was based on some other timer code I found in the tree that was evidently also leaky (I don't remember where unfortunately).
>
> One thing: are you sure it is safe to call timer_del() again if the timer may already have been deleted? It looks that way from the implementation, but the header comment isn't explicit.

Yes, it is safe, afaik, it's removing it from the list of active timers.

> Otherwise,
> Reviewed-by: Andrew Baumann <Andrew.Baumann@microsoft.com>

thanks



-- 
Marc-André Lureau

[Qemu-devel] [PATCH 27/37] 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>
---
 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

Re: [Qemu-devel] [PATCH 27/37] qjson: free str

[Eric Blake]

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

On 07/19/2016 02:54 AM, marcandre.lureau@redhat.com wrote:
> 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>
> ---
>  migration/qjson.c | 1 +
>  1 file changed, 1 insertion(+)
> 

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

> 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);
>  }
> 

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


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

[Qemu-devel] [PATCH 28/37] 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().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/input/virtio-input-hid.c      |  1 +
 hw/input/virtio-input.c          | 10 ++++++++++
 include/hw/virtio/virtio-input.h |  1 +
 3 files changed, 12 insertions(+)

diff --git a/hw/input/virtio-input-hid.c b/hw/input/virtio-input-hid.c
index 3ee0c18..0e5596e 100644
--- a/hw/input/virtio-input-hid.c
+++ b/hw/input/virtio-input-hid.c
@@ -340,6 +340,7 @@ static const TypeInfo virtio_input_hid_info = {
     .instance_size = sizeof(VirtIOInputHID),
     .class_init    = virtio_input_hid_class_init,
     .abstract      = true,
+    .instance_finalize = virtio_input_finalize,
 };
 
 /* ----------------------------------------------------------------- */
diff --git a/hw/input/virtio-input.c b/hw/input/virtio-input.c
index edf6990..36a7d5e 100644
--- a/hw/input/virtio-input.c
+++ b/hw/input/virtio-input.c
@@ -285,6 +285,16 @@ static void virtio_input_device_realize(DeviceState *dev, Error **errp)
                     virtio_input_save, virtio_input_load, vinput);
 }
 
+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);
diff --git a/include/hw/virtio/virtio-input.h b/include/hw/virtio/virtio-input.h
index 55db310..8fd790f 100644
--- a/include/hw/virtio/virtio-input.h
+++ b/include/hw/virtio/virtio-input.h
@@ -104,5 +104,6 @@ void virtio_input_add_config(VirtIOInput *vinput,
                              virtio_input_config *config);
 void virtio_input_idstr_config(VirtIOInput *vinput,
                                uint8_t select, const char *string);
+void virtio_input_finalize(Object *obj);
 
 #endif /* QEMU_VIRTIO_INPUT_H */
-- 
2.9.0

Re: [Qemu-devel] [PATCH 28/37] virtio-input: free config list

[Gerd Hoffmann]

> --- a/hw/input/virtio-input-hid.c
> +++ b/hw/input/virtio-input-hid.c

> +    .instance_finalize = virtio_input_finalize,

> --- a/hw/input/virtio-input.c
> +++ b/hw/input/virtio-input.c

> +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);
> +    }
> +}

I think you can keep this local to virtio-input.c and simply hook it
into the abstract base class (TYPE_VIRTIO_INPUT).

Other than that it looks fine to me.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 28/37] virtio-input: free config list

[Marc-André Lureau]

Hi

----- Original Message -----
> > --- a/hw/input/virtio-input-hid.c
> > +++ b/hw/input/virtio-input-hid.c
> 
> > +    .instance_finalize = virtio_input_finalize,
> 
> > --- a/hw/input/virtio-input.c
> > +++ b/hw/input/virtio-input.c
> 
> > +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);
> > +    }
> > +}
> 
> I think you can keep this local to virtio-input.c and simply hook it
> into the abstract base class (TYPE_VIRTIO_INPUT).
> 

Yes, not sure why I didn't do that in the first, place. fixed.

[Qemu-devel] [PATCH 29/37] 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>
---
 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

Re: [Qemu-devel] [PATCH 29/37] ipmi: free extern timer

[Corey Minyard]

I believe this is correct:

Reviewed-by: Corey Minyard <cminyard@mvista.com>

I looked around at other devices with timers, I found several (two 
watchdogs, and ipmi_bmc_sim.c, for instance) that allocate the timer in 
the realize function but don't deallocate it in the unrealize function.  
Do those need to be fixed, too?

-corey


On 07/19/2016 03:54 AM, marcandre.lureau@redhat.com wrote:
> 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>
> ---
>   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,
>    };
>   

Re: [Qemu-devel] [PATCH 29/37] ipmi: free extern timer

[Marc-André Lureau]

Hi

----- Original Message -----
> I believe this is correct:
> 
> Reviewed-by: Corey Minyard <cminyard@mvista.com>
> 
> I looked around at other devices with timers, I found several (two
> watchdogs, and ipmi_bmc_sim.c, for instance) that allocate the timer in
> the realize function but don't deallocate it in the unrealize function.
> Do those need to be fixed, too?

Yes, I haven't found them myself, but I can include fixes in the series too.

thanks

> 
> -corey
> 
> 
> On 07/19/2016 03:54 AM, marcandre.lureau@redhat.com wrote:
> > 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>
> > ---
> >   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,
> >    };
> >   
> 
> 

[Qemu-devel] [PATCH 30/37] 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>
---
 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

Re: [Qemu-devel] [PATCH 30/37] usb: free USBDevice.strings

[Gerd Hoffmann]

On Di, 2016-07-19 at 12:54 +0400, marcandre.lureau@redhat.com wrote:
> The list is created during instance init and further populated with
> usb_desc_set_string(). Clear it when unrealizing the device.

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

[Qemu-devel] [PATCH 31/37] tests: free a bunch of qmp responses

[marcandre.lureau]

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/libqos/usb.c             |  1 +
 tests/postcopy-test.c          |  3 ++-
 tests/pvpanic-test.c           |  1 +
 tests/test-filter-mirror.c     |  4 +++-
 tests/test-filter-redirector.c |  8 ++++++--
 tests/virtio-blk-test.c        | 19 +++++++++++++------
 tests/virtio-net-test.c        | 10 +++++++---
 7 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/tests/libqos/usb.c b/tests/libqos/usb.c
index f794d92..0b7a610 100644
--- a/tests/libqos/usb.c
+++ b/tests/libqos/usb.c
@@ -66,4 +66,5 @@ void usb_test_hotplug(const char *hcd_id, const int port,
     g_assert(response);
     g_assert(qdict_haskey(response, "event"));
     g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
+    QDECREF(response);
 }
diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
index 16465ab..74fa969 100644
--- a/tests/postcopy-test.c
+++ b/tests/postcopy-test.c
@@ -397,7 +397,8 @@ static void test_migrate(void)
         usleep(10 * 1000);
     } while (dest_byte_a == dest_byte_b);
 
-    qmp("{ 'execute' : 'stop'}");
+    rsp = qmp("{ 'execute' : 'stop'}");
+    QDECREF(rsp);
     /* With it stopped, check nothing changes */
     qtest_memread(to, start_address, &dest_byte_c, 1);
     sleep(1);
diff --git a/tests/pvpanic-test.c b/tests/pvpanic-test.c
index 3bfa678..71ebb5c 100644
--- a/tests/pvpanic-test.c
+++ b/tests/pvpanic-test.c
@@ -27,6 +27,7 @@ static void test_panic(void)
     data = qdict_get_qdict(response, "data");
     g_assert(qdict_haskey(data, "action"));
     g_assert_cmpstr(qdict_get_str(data, "action"), ==, "pause");
+    QDECREF(response);
 }
 
 int main(int argc, char **argv)
diff --git a/tests/test-filter-mirror.c b/tests/test-filter-mirror.c
index ffaaffa..8050a6f 100644
--- a/tests/test-filter-mirror.c
+++ b/tests/test-filter-mirror.c
@@ -26,6 +26,7 @@ static void test_mirror(void)
     char send_buf[] = "Hello! filter-mirror~";
     char sock_path[] = "filter-mirror.XXXXXX";
     char *recv_buf;
+    QDict *resp;
     uint32_t size = sizeof(send_buf);
     size = htonl(size);
 
@@ -57,7 +58,8 @@ static void test_mirror(void)
     };
 
     /* send a qmp command to guarantee that 'connected' is setting to true. */
-    qmp("{ 'execute' : 'query-status'}");
+    resp = qmp("{ 'execute' : 'query-status'}");
+    QDECREF(resp);
     ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) + sizeof(send_buf));
     g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size));
     close(send_sock[0]);
diff --git a/tests/test-filter-redirector.c b/tests/test-filter-redirector.c
index c63b68f..779a4f3 100644
--- a/tests/test-filter-redirector.c
+++ b/tests/test-filter-redirector.c
@@ -69,6 +69,7 @@ static void test_redirector_tx(void)
     char sock_path0[] = "filter-redirector0.XXXXXX";
     char sock_path1[] = "filter-redirector1.XXXXXX";
     char *recv_buf;
+    QDict *resp;
     uint32_t size = sizeof(send_buf);
     size = htonl(size);
 
@@ -99,7 +100,8 @@ static void test_redirector_tx(void)
     g_assert_cmpint(recv_sock, !=, -1);
 
     /* send a qmp command to guarantee that 'connected' is setting to true. */
-    qmp("{ 'execute' : 'query-status'}");
+    resp = qmp("{ 'execute' : 'query-status'}");
+    QDECREF(resp);
 
     struct iovec iov[] = {
         {
@@ -145,6 +147,7 @@ static void test_redirector_rx(void)
     char sock_path0[] = "filter-redirector0.XXXXXX";
     char sock_path1[] = "filter-redirector1.XXXXXX";
     char *recv_buf;
+    QDict *resp;
     uint32_t size = sizeof(send_buf);
     size = htonl(size);
 
@@ -184,7 +187,8 @@ static void test_redirector_rx(void)
     send_sock = unix_connect(sock_path1, NULL);
     g_assert_cmpint(send_sock, !=, -1);
     /* send a qmp command to guarantee that 'connected' is setting to true. */
-    qmp("{ 'execute' : 'query-status'}");
+    resp = qmp("{ 'execute' : 'query-status'}");
+    QDECREF(resp);
 
     ret = iov_send(send_sock, iov, 2, 0, sizeof(size) + sizeof(send_buf));
     g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size));
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 811cf75..48822a2 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -416,6 +416,7 @@ static void pci_config(void)
     int n_size = TEST_IMAGE_SIZE / 2;
     void *addr;
     uint64_t capacity;
+    QDict *resp;
 
     bus = pci_test_start();
 
@@ -430,8 +431,10 @@ static void pci_config(void)
 
     qvirtio_set_driver_ok(&qvirtio_pci, &dev->vdev);
 
-    qmp("{ 'execute': 'block_resize', 'arguments': { 'device': 'drive0', "
-                                                    " 'size': %d } }", n_size);
+    resp = qmp("{ 'execute': 'block_resize', 'arguments': "
+               "{ 'device': 'drive0', 'size': %d } }", n_size);
+    QDECREF(resp);
+
     qvirtio_wait_config_isr(&qvirtio_pci, &dev->vdev, QVIRTIO_BLK_TIMEOUT_US);
 
     capacity = qvirtio_config_readq(&qvirtio_pci, &dev->vdev,
@@ -459,6 +462,7 @@ static void pci_msix(void)
     uint32_t free_head;
     uint8_t status;
     char *data;
+    QDict *resp;
 
     bus = pci_test_start();
     alloc = pc_alloc_init();
@@ -488,8 +492,9 @@ static void pci_msix(void)
 
     qvirtio_set_driver_ok(&qvirtio_pci, &dev->vdev);
 
-    qmp("{ 'execute': 'block_resize', 'arguments': { 'device': 'drive0', "
-                                                    " 'size': %d } }", n_size);
+    resp = qmp("{ 'execute': 'block_resize', 'arguments': "
+               "{ 'device': 'drive0', 'size': %d } }", n_size);
+    QDECREF(resp);
 
     qvirtio_wait_config_isr(&qvirtio_pci, &dev->vdev, QVIRTIO_BLK_TIMEOUT_US);
 
@@ -717,6 +722,7 @@ static void mmio_basic(void)
     QGuestAllocator *alloc;
     int n_size = TEST_IMAGE_SIZE / 2;
     uint64_t capacity;
+    QDict *resp;
 
     arm_test_start();
 
@@ -734,8 +740,9 @@ static void mmio_basic(void)
     test_basic(&qvirtio_mmio, &dev->vdev, alloc, vq,
                             QVIRTIO_MMIO_DEVICE_SPECIFIC);
 
-    qmp("{ 'execute': 'block_resize', 'arguments': { 'device': 'drive0', "
-                                                    " 'size': %d } }", n_size);
+    resp = qmp("{ 'execute': 'block_resize', 'arguments': "
+               "{ 'device': 'drive0', 'size': %d } }", n_size);
+    QDECREF(resp);
 
     qvirtio_wait_queue_isr(&qvirtio_mmio, &dev->vdev, vq,
                            QVIRTIO_BLK_TIMEOUT_US);
diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
index a34a939..43de303 100644
--- a/tests/virtio-net-test.c
+++ b/tests/virtio-net-test.c
@@ -159,13 +159,15 @@ static void rx_stop_cont_test(const QVirtioBus *bus, QVirtioDevice *dev,
         },
     };
     int ret;
+    QDict *resp;
 
     req_addr = guest_alloc(alloc, 64);
 
     free_head = qvirtqueue_add(vq, req_addr, 64, true, false);
     qvirtqueue_kick(bus, dev, vq, free_head);
 
-    qmp("{ 'execute' : 'stop'}");
+    resp = qmp("{ 'execute' : 'stop'}");
+    QDECREF(resp);
 
     ret = iov_send(socket, iov, 2, 0, sizeof(len) + sizeof(test));
     g_assert_cmpint(ret, ==, sizeof(test) + sizeof(len));
@@ -173,8 +175,10 @@ static void rx_stop_cont_test(const QVirtioBus *bus, QVirtioDevice *dev,
     /* We could check the status, but this command is more importantly to
      * ensure the packet data gets queued in QEMU, before we do 'cont'.
      */
-    qmp("{ 'execute' : 'query-status'}");
-    qmp("{ 'execute' : 'cont'}");
+    resp = qmp("{ 'execute' : 'query-status'}");
+    QDECREF(resp);
+    resp = qmp("{ 'execute' : 'cont'}");
+    QDECREF(resp);
 
     qvirtio_wait_queue_isr(bus, dev, vq, QVIRTIO_NET_TIMEOUT_US);
     memread(req_addr + VNET_HDR_SIZE, buffer, sizeof(test));
-- 
2.9.0

Re: [Qemu-devel] [PATCH 31/37] tests: free a bunch of qmp responses

[Eric Blake]

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

On 07/19/2016 02:54 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/libqos/usb.c             |  1 +
>  tests/postcopy-test.c          |  3 ++-
>  tests/pvpanic-test.c           |  1 +
>  tests/test-filter-mirror.c     |  4 +++-
>  tests/test-filter-redirector.c |  8 ++++++--
>  tests/virtio-blk-test.c        | 19 +++++++++++++------
>  tests/virtio-net-test.c        | 10 +++++++---
>  7 files changed, 33 insertions(+), 13 deletions(-)

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


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


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

[Qemu-devel] [PATCH 32/37] 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>
---
 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

Re: [Qemu-devel] [PATCH 32/37] usb: free leaking path

[Gerd Hoffmann]

On Di, 2016-07-19 at 12:54 +0400, marcandre.lureau@redhat.com wrote:
> qdev_get_dev_path() returns an allocated string, free it when no
> longer
> needed.

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

[Qemu-devel] [PATCH 33/37] 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>
---
 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 1d1f861..aa5b4dc 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 34/37] tests: pc-cpu-test

[marcandre.lureau]

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

---
 tests/pc-cpu-test.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/pc-cpu-test.c b/tests/pc-cpu-test.c
index 4428cea..06afca3 100644
--- a/tests/pc-cpu-test.c
+++ b/tests/pc-cpu-test.c
@@ -78,7 +78,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 +100,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;
@@ -120,13 +121,16 @@ static void add_pc_test_cases(void)
                                    mname, data->sockets, data->cores,
                                    data->threads, data->maxcpus);
             qtest_add_data_func(path, data, test_pc_without_cpu_add);
+            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);
+            g_free(path);
         }
     }
+    QDECREF(response);
     qtest_end();
 }
 
-- 
2.9.0

Re: [Qemu-devel] [PATCH 34/37] tests: pc-cpu-test

[Marc-André Lureau]

Hi,

Sorry, nack that one, it is missing some explanations, signed-off etc.

----- Original Message -----
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> ---
>  tests/pc-cpu-test.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/pc-cpu-test.c b/tests/pc-cpu-test.c
> index 4428cea..06afca3 100644
> --- a/tests/pc-cpu-test.c
> +++ b/tests/pc-cpu-test.c
> @@ -78,7 +78,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 +100,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;
> @@ -120,13 +121,16 @@ static void add_pc_test_cases(void)
>                                     mname, data->sockets, data->cores,
>                                     data->threads, data->maxcpus);
>              qtest_add_data_func(path, data, test_pc_without_cpu_add);
> +            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);
> +            g_free(path);
>          }
>      }
> +    QDECREF(response);
>      qtest_end();
>  }
>  
> --
> 2.9.0
> 
> 

[Qemu-devel] [PATCH 35/37] 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>
---
 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 74fa969..f6f6598 100644
--- a/tests/postcopy-test.c
+++ b/tests/postcopy-test.c
@@ -198,8 +198,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

Re: [Qemu-devel] [PATCH 35/37] tests: fix rsp leak in postcopy-test

[Eric Blake]

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

On 07/19/2016 02:54 AM, marcandre.lureau@redhat.com wrote:
> 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>
> ---
>  tests/postcopy-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

> 
> diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
> index 74fa969..f6f6598 100644
> --- a/tests/postcopy-test.c
> +++ b/tests/postcopy-test.c
> @@ -198,8 +198,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;
>  }
>  
> 

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


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

[Qemu-devel] [PATCH 36/37] 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

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

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 6defeed..0ea5805 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;
 }
 
@@ -1330,6 +1331,7 @@ static void ahci_start_dma(IDEDMA *dma, IDEState *s,
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
     DPRINTF(ad->port_no, "\n");
     s->io_buffer_offset = 0;
+    qemu_sglist_destroy(&s->sg);
     dma_cb(s, 0);
 }
 
-- 
2.9.0

Re: [Qemu-devel] [PATCH 36/37] ahci: fix sglist leak on retry

[John Snow]

On 07/19/2016 04:54 AM, marcandre.lureau@redhat.com wrote:
> 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
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/ide/ahci.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 6defeed..0ea5805 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;
>  }
>  

I wish I could remember why I didn't want to put the sglist_destroy into
ncq_err ... Well, probably not important.

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.

> @@ -1330,6 +1331,7 @@ static void ahci_start_dma(IDEDMA *dma, IDEState *s,
>      AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
>      DPRINTF(ad->port_no, "\n");
>      s->io_buffer_offset = 0;
> +    qemu_sglist_destroy(&s->sg);

Hm, I'd rather the list destruction go in ide_dma_cb when it has become
clear we are going to be halting instead of in ahci_start_dma.

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

>      dma_cb(s, 0);
>  }
>  
> 

Thanks!
--js

[Qemu-devel] [PATCH 37/37] 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>
---
 tests/postcopy-test.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
index f6f6598..9b32c5e 100644
--- a/tests/postcopy-test.c
+++ b/tests/postcopy-test.c
@@ -132,6 +132,7 @@ static void wait_for_serial(const char *side)
     char *serialpath = g_strdup_printf("%s/%s", tmpfs, side);
     FILE *serialfile = fopen(serialpath, "r");
 
+    g_free(serialpath);
     do {
         int readvalue = fgetc(serialfile);
 
@@ -143,7 +144,6 @@ static void wait_for_serial(const char *side)
         case 'B':
             /* It's alive! */
             fclose(serialfile);
-            g_free(serialpath);
             return;
 
         case EOF:
@@ -288,6 +288,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)
@@ -319,6 +320,7 @@ static void test_migrate(void)
                           " -drive file=%s,format=raw"
                           " -incoming %s",
                           tmpfs, bootpath, uri);
+    g_free(bootpath);
     to = qtest_init(cmd);
     g_free(cmd);
 
-- 
2.9.0