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

[Qemu-devel] [PATCH v2 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.

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

Marc-André Lureau (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
  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: add qtest_add_data_func_full
  tests: pc-cpu-test leaks fixes
  tests: fix rsp leak in postcopy-test
  ahci: fix sglist leak on retry
  tests: fix postcopy-test leaks

 hw/audio/gus.c                    |  9 ++++++---
 hw/audio/sb16.c                   |  4 +++-
 hw/block/fdc.c                    |  4 +++-
 hw/char/parallel.c                |  3 ++-
 hw/core/bus.c                     | 21 ++++++---------------
 hw/core/machine.c                 |  1 +
 hw/display/vga-isa.c              |  8 ++++++--
 hw/dma/i8257.c                    |  6 ++++--
 hw/i386/acpi-build.c              |  4 ++--
 hw/i386/pc.c                      |  9 +++++----
 hw/i386/pc_piix.c                 | 17 ++++++++---------
 hw/i386/pc_q35.c                  | 15 ++++++++-------
 hw/ide/ahci.c                     |  4 ++--
 hw/ide/core.c                     |  7 +++++--
 hw/input/pckbd.c                  |  4 ++--
 hw/input/virtio-input.c           | 11 +++++++++++
 hw/ipmi/ipmi_bmc_extern.c         |  9 +++++++++
 hw/isa/isa-bus.c                  | 14 +++++---------
 hw/sd/sd.c                        |  9 +++++++++
 hw/usb/bus.c                      |  7 +++++++
 hw/usb/desc.c                     |  1 +
 include/hw/boards.h               |  2 +-
 include/hw/i386/pc.h              |  4 ++--
 include/hw/ide/internal.h         |  2 ++
 include/hw/isa/i8257.h            |  2 ++
 include/hw/isa/isa.h              |  5 ++++-
 include/hw/qdev-core.h            |  2 +-
 migration/qjson.c                 |  1 +
 numa.c                            | 15 ++++++++-------
 pc-bios/optionrom/Makefile        |  4 ++--
 qemu-char.c                       | 10 ++++++++++
 qga/guest-agent-command-state.c   |  7 +++++++
 qga/guest-agent-core.h            |  1 +
 qga/main.c                        | 14 ++++++++------
 tests/check-qom-interface.c       |  1 +
 tests/check-qom-proplist.c        | 16 ++++++++++++++++
 tests/libqos/usb.c                |  1 +
 tests/libqtest.c                  | 15 ++++++++++++++-
 tests/libqtest.h                  |  7 ++++++-
 tests/pc-cpu-test.c               | 24 +++++++++++++++++++-----
 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   | 20 +++++++++++++-------
 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 ++-
 54 files changed, 300 insertions(+), 124 deletions(-)

-- 
2.9.0

[Qemu-devel] [PATCH v2 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 v2 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/test-qga.c | 5 +++++
 1 file changed, 5 insertions(+)

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

[Qemu-devel] [PATCH v2 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 | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

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

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

[Eric Blake]

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

On 07/28/2016 08:37 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 | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index 4c3b2c7..67be90b 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -1175,6 +1175,8 @@ static void config_free(GAConfig *config)
>  #ifdef CONFIG_FSFREEZE
>      g_free(config->fsfreeze_hook);
>  #endif
> +    g_list_foreach(config->blacklist, (GFunc)g_free, NULL);

What an ugly cast - undefined behavior according to C. But it happens to
work on all ABI that we support (calling a unary function as cast to
appear as a binary function silently ignores the second argument), so I
could sort of live with it, if it made the patch arguably smaller due to
fewer lines of code than required for a type-correct solution.

> +    g_list_free(config->blacklist);
>      g_free(config);
>  }
>  
> @@ -1310,11 +1312,6 @@ static int run_agent(GAState *s, GAConfig *config)
>      return EXIT_SUCCESS;
>  }
>  
> -static void free_blacklist_entry(gpointer entry, gpointer unused)
> -{
> -    g_free(entry);
> -}

On the other hand, we already had EXACTLY the type-correct forwarder
function in place (it just needs to be hoisted earlier before the point
where you use it).

So I would really prefer that v3 use free_blacklist_entry() rather than
(GFunc)g_free as the argument to g_list_foreach().

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


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

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

[Marc-André Lureau]

----- Original Message -----
> On 07/28/2016 08:37 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 | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/qga/main.c b/qga/main.c
> > index 4c3b2c7..67be90b 100644
> > --- a/qga/main.c
> > +++ b/qga/main.c
> > @@ -1175,6 +1175,8 @@ static void config_free(GAConfig *config)
> >  #ifdef CONFIG_FSFREEZE
> >      g_free(config->fsfreeze_hook);
> >  #endif
> > +    g_list_foreach(config->blacklist, (GFunc)g_free, NULL);
> 
> What an ugly cast - undefined behavior according to C. But it happens to
> work on all ABI that we support (calling a unary function as cast to
> appear as a binary function silently ignores the second argument), so I
> could sort of live with it, if it made the patch arguably smaller due to
> fewer lines of code than required for a type-correct solution.


Note that's how glib implements g_slist_free_full() and others, I just used the same common code/style

> 
> > +    g_list_free(config->blacklist);
> >      g_free(config);
> >  }
> >  
> > @@ -1310,11 +1312,6 @@ static int run_agent(GAState *s, GAConfig *config)
> >      return EXIT_SUCCESS;
> >  }
> >  
> > -static void free_blacklist_entry(gpointer entry, gpointer unused)
> > -{
> > -    g_free(entry);
> > -}
> 
> On the other hand, we already had EXACTLY the type-correct forwarder
> function in place (it just needs to be hoisted earlier before the point
> where you use it).
> 
> So I would really prefer that v3 use free_blacklist_entry() rather than
> (GFunc)g_free as the argument to g_list_foreach().

ok ;)

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

[Eric Blake]

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

On 07/28/2016 03:36 PM, Marc-André Lureau wrote:

>>> +    g_list_foreach(config->blacklist, (GFunc)g_free, NULL);
>>
>> What an ugly cast - undefined behavior according to C. But it happens to
>> work on all ABI that we support (calling a unary function as cast to
>> appear as a binary function silently ignores the second argument), so I
>> could sort of live with it, if it made the patch arguably smaller due to
>> fewer lines of code than required for a type-correct solution.
> 
> 
> Note that's how glib implements g_slist_free_full() and others, I just used the same common code/style

Just because glib disregards C type safety and relies on platform ABI
doesn't mean we have to be equally cavalier in our code.  Again, I'm not
outright rejecting your shorter style, precisely because glib already
uses the style (so it already happens to work), but do think that it is
easier to maintain code that doesn't have ugly casts (where I have to
think about ABI issues) because it is C-compliant to begin with.


-- 
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 v2 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 | 7 +++++++
 qga/guest-agent-core.h          | 1 +
 qga/main.c                      | 6 ++++++
 3 files changed, 14 insertions(+)

diff --git a/qga/guest-agent-command-state.c b/qga/guest-agent-command-state.c
index 4de229c..56e63b1 100644
--- a/qga/guest-agent-command-state.c
+++ b/qga/guest-agent-command-state.c
@@ -71,3 +71,10 @@ GACommandState *ga_command_state_new(void)
     cs->groups = NULL;
     return cs;
 }
+
+void ga_command_state_free(GACommandState *cs)
+{
+    g_slist_foreach(cs->groups, (GFunc)g_free, NULL);
+    g_slist_free(cs->groups);
+    g_free(cs);
+}
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
index 0a49516..63e9d39 100644
--- a/qga/guest-agent-core.h
+++ b/qga/guest-agent-core.h
@@ -28,6 +28,7 @@ void ga_command_state_add(GACommandState *cs,
 void ga_command_state_init_all(GACommandState *cs);
 void ga_command_state_cleanup_all(GACommandState *cs);
 GACommandState *ga_command_state_new(void);
+void ga_command_state_free(GACommandState *cs);
 bool ga_logging_enabled(GAState *s);
 void ga_disable_logging(GAState *s);
 void ga_enable_logging(GAState *s);
diff --git a/qga/main.c b/qga/main.c
index 67be90b..522a609 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -1372,6 +1372,8 @@ int main(int argc, char **argv)
 end:
     if (s->command_state) {
         ga_command_state_cleanup_all(s->command_state);
+        ga_command_state_free(s->command_state);
+        json_message_parser_destroy(&s->parser);
     }
     if (s->channel) {
         ga_channel_free(s->channel);
@@ -1384,6 +1386,10 @@ end:
     }
 
     config_free(config);
+    if (s->main_loop) {
+        g_main_loop_unref(s->main_loop);
+    }
+    g_free(s);
 
     return ret;
 }
-- 
2.9.0

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

[Eric Blake]

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

On 07/28/2016 08:37 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 | 7 +++++++
>  qga/guest-agent-core.h          | 1 +
>  qga/main.c                      | 6 ++++++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/qga/guest-agent-command-state.c b/qga/guest-agent-command-state.c
> index 4de229c..56e63b1 100644
> --- a/qga/guest-agent-command-state.c
> +++ b/qga/guest-agent-command-state.c
> @@ -71,3 +71,10 @@ GACommandState *ga_command_state_new(void)
>      cs->groups = NULL;
>      return cs;
>  }
> +
> +void ga_command_state_free(GACommandState *cs)
> +{
> +    g_slist_foreach(cs->groups, (GFunc)g_free, NULL);

Here, the ugly cast is because we don't have a static forwarder function
with the correct signature handy.  Hmm, I wonder if it is worth adding a
static inline forwarder function in one of our headers that ALL files
can use, when they want to use a 2-arg callback that merely calls
g_free() on its first argument and ignores the second, since we have now
proved it is a commonly-used forwarder.

-- 
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 v2 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/test-cutils.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

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

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/test-vmstate.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

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

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/test-iov.c | 7 +++++++
 1 file changed, 7 insertions(+)

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

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

[marcandre.lureau]

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

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

Spotted thanks to ASAN.

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

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

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/check-qom-interface.c | 1 +
 1 file changed, 1 insertion(+)

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

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/check-qom-proplist.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

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

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

[marcandre.lureau]

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

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

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

diff --git a/tests/test-io-channel-command.c b/tests/test-io-channel-command.c
index 1d1f461..f99118e 100644
--- a/tests/test-io-channel-command.c
+++ b/tests/test-io-channel-command.c
@@ -18,6 +18,7 @@
  *
  */
 
+#include <glib/gstdio.h>
 #include "qemu/osdep.h"
 #include "io/channel-command.h"
 #include "io-channel-helpers.h"
@@ -26,11 +27,14 @@
 #ifndef WIN32
 static void test_io_channel_command_fifo(bool async)
 {
-#define TEST_FIFO "tests/test-io-channel-command.fifo"
     QIOChannel *src, *dst;
     QIOChannelTest *test;
-    char *srcfifo = g_strdup_printf("PIPE:%s,wronly", TEST_FIFO);
-    char *dstfifo = g_strdup_printf("PIPE:%s,rdonly", TEST_FIFO);
+    char *tmpdir = g_strdup("/tmp/test-io-channel.XXXXXX");
+    g_assert_nonnull(mkdtemp(tmpdir));
+
+    char *fifo = g_strdup_printf("%s/command.fifo", tmpdir);
+    char *srcfifo = g_strdup_printf("PIPE:%s,wronly", fifo);
+    char *dstfifo = g_strdup_printf("PIPE:%s,rdonly", fifo);
     const char *srcargv[] = {
         "/bin/socat", "-", srcfifo, NULL,
     };
@@ -38,11 +42,10 @@ static void test_io_channel_command_fifo(bool async)
         "/bin/socat", dstfifo, "-", NULL,
     };
 
-    unlink(TEST_FIFO);
     if (access("/bin/socat", X_OK) < 0) {
-        return; /* Pretend success if socat is not present */
+        goto end; /* Pretend success if socat is not present */
     }
-    if (mkfifo(TEST_FIFO, 0600) < 0) {
+    if (mkfifo(fifo, 0600) < 0) {
         abort();
     }
     src = QIO_CHANNEL(qio_channel_command_new_spawn(srcargv,
@@ -59,9 +62,12 @@ static void test_io_channel_command_fifo(bool async)
     object_unref(OBJECT(src));
     object_unref(OBJECT(dst));
 
+end:
+    g_free(fifo);
     g_free(srcfifo);
     g_free(dstfifo);
-    unlink(TEST_FIFO);
+    g_rmdir(tmpdir);
+    g_free(tmpdir);
 }
 
 
-- 
2.9.0

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

[Eric Blake]

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

On 07/28/2016 08:37 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> srcfifo && dstfifo must be freed in error case, however unlink() may
> delete a file from a different context. Instead, use mkdtemp()/rmdir()
> for the temporary files.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/test-io-channel-command.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/test-io-channel-command.c b/tests/test-io-channel-command.c
> index 1d1f461..f99118e 100644
> --- a/tests/test-io-channel-command.c
> +++ b/tests/test-io-channel-command.c
> @@ -18,6 +18,7 @@
>   *
>   */
>  
> +#include <glib/gstdio.h>
>  #include "qemu/osdep.h"
>  #include "io/channel-command.h"
>  #include "io-channel-helpers.h"
> @@ -26,11 +27,14 @@
>  #ifndef WIN32
>  static void test_io_channel_command_fifo(bool async)
>  {
> -#define TEST_FIFO "tests/test-io-channel-command.fifo"
>      QIOChannel *src, *dst;
>      QIOChannelTest *test;
> -    char *srcfifo = g_strdup_printf("PIPE:%s,wronly", TEST_FIFO);
> -    char *dstfifo = g_strdup_printf("PIPE:%s,rdonly", TEST_FIFO);
> +    char *tmpdir = g_strdup("/tmp/test-io-channel.XXXXXX");
> +    g_assert_nonnull(mkdtemp(tmpdir));

I'm always wary of side effects inside assert().  But if I recall
correctly, g_assert_nonnull() is unconditional (there's no way like
NDEBUG to completely compile away the side effect), so I guess you're
safe, even though I would have stored the results of mkdtemp() in a
temporary and only then asserted that the temporary is non-null.

Whether or not you clean that up,

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 v2 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/test-string-input-visitor.c | 1 +
 1 file changed, 1 insertion(+)

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

[Qemu-devel] [PATCH v2 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 081c9eb..95790cc 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2617,10 +2617,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

Re: [Qemu-devel] [PATCH v2 13/37] portio: keep references on portio

[Paolo Bonzini]

On 28/07/2016 16:37, marcandre.lureau@redhat.com wrote:
> 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().

This is mostly not an issue because ISA devices are not hot-unpluggable,
but the commit message is correct.

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

> 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 081c9eb..95790cc 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2617,10 +2617,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);
>  
> 

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 numa.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

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

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hw/i386/pc.c         | 8 ++++----
 hw/input/pckbd.c     | 4 ++--
 include/hw/i386/pc.h | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

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

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hw/i386/pc.c | 1 +
 1 file changed, 1 insertion(+)

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

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

[Qemu-devel] [PATCH v2 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>
Tested-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/i386/acpi-build.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 77c40d9..017bb51 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -764,7 +764,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;
 
@@ -788,7 +788,7 @@ static void crs_replace_with_free_ranges(GPtrArray *ranges,
         g_ptr_array_add(ranges, g_ptr_array_index(free_ranges, i));
     }
 
-    g_ptr_array_free(free_ranges, false);
+    g_ptr_array_free(free_ranges, true);
 }
 
 /*
-- 
2.9.0

[Qemu-devel] [PATCH v2 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

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

[Eric Blake]

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

On 07/28/2016 08:37 AM, marcandre.lureau@redhat.com wrote:
> 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(+)

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

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

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


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

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

[Paolo Bonzini]

On 28/07/2016 16:37, marcandre.lureau@redhat.com wrote:
> 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);
>      }
> 

The patch, as is, would cause for example two calls to
qemu_chr_be_event(chr, CHR_EVENT_CLOSED).  It's also unnecessary to set
up a watch on s->listen_ioc and to call qemu_chr_socket_restart_timer.

I think the right fix is to extract the relevant parts of
tcp_chr_disconnect into a new function (e.g. tcp_chr_free_connection)
and call the function from tcp_chr_close.

Paolo

[Qemu-devel] [PATCH v2 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

Re: [Qemu-devel] [PATCH v2 20/37] char: free MuxDriver when closing

[Eric Blake]

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

On 07/28/2016 08:37 AM, marcandre.lureau@redhat.com wrote:
> 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? */

This comment is not re-assuring.

> +    g_free(d);
> +}

I looked at MuxDriver; the pointers in chr_can_read, chr_read, and
chr_event are not allocated (since they are addresses of callback
functions), ext_opaque is untouched (owned by the caller, not something
we allocate), and drv is a back-reference; and there are no other
pointers.  So this looks correct.

With the bad comment removed,
Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [PATCH v2 20/37] char: free MuxDriver when closing

[Paolo Bonzini]

On 28/07/2016 16:37, marcandre.lureau@redhat.com wrote:
> 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? */

Perhaps qemu_chr_free(d->drv)?

Paolo

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

Re: [Qemu-devel] [PATCH v2 20/37] char: free MuxDriver when closing

[Paolo Bonzini]

On 01/08/2016 14:50, Paolo Bonzini wrote:
> 
> 
> On 28/07/2016 16:37, marcandre.lureau@redhat.com wrote:
>> 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? */
> 
> Perhaps qemu_chr_free(d->drv)?

Hmm, no, a qemu_chr_delete(mux) could conceivably delete d->drv too, but
not a free.  So the patch is okay with the comment removed.

Paolo

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/qom-test.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

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

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/i386/pc_q35.c | 2 ++
 1 file changed, 2 insertions(+)

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

[Qemu-devel] [PATCH v2 23/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    | 17 ++++++++---------
 hw/i386/pc_q35.c     | 13 ++++++-------
 include/hw/i386/pc.h |  1 +
 3 files changed, 15 insertions(+), 16 deletions(-)

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

[Qemu-devel] [PATCH v2 24/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>
Reviewed-by: John Snow <jsnow@redhat.com>
Acked-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 1 +
 1 file changed, 1 insertion(+)

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

[Qemu-devel] [PATCH v2 25/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>
Reviewed-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
---
 hw/sd/sd.c | 9 +++++++++
 1 file changed, 9 insertions(+)

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

[Qemu-devel] [PATCH v2 26/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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 migration/qjson.c | 1 +
 1 file changed, 1 insertion(+)

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

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

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

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

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

[Qemu-devel] [PATCH v2 28/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>
Reviewed-by: Corey Minyard <cminyard@mvista.com>
---
 hw/ipmi/ipmi_bmc_extern.c | 9 +++++++++
 1 file changed, 9 insertions(+)

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

[Qemu-devel] [PATCH v2 29/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>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/bus.c | 7 +++++++
 1 file changed, 7 insertions(+)

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

[Qemu-devel] [PATCH v2 30/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>
Reviewed-by: Eric Blake <eblake@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

[Qemu-devel] [PATCH v2 31/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>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/desc.c | 1 +
 1 file changed, 1 insertion(+)

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

[Qemu-devel] [PATCH v2 32/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 4b4b33b..2c97347 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -224,7 +224,7 @@ typedef struct BusChild {
 struct BusState {
     Object obj;
     DeviceState *parent;
-    const char *name;
+    char *name;
     HotplugHandler *hotplug_handler;
     int max_index;
     bool realized;
-- 
2.9.0

Re: [Qemu-devel] [PATCH v2 32/37] bus: simplify name handling

[Eric Blake]

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

On 07/28/2016 08:38 AM, marcandre.lureau@redhat.com wrote:
> 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(-)

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 v2 33/37] tests: add qtest_add_data_func_full

[marcandre.lureau]

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

Allows to specify a destroy function for the test data.

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

diff --git a/tests/libqtest.c b/tests/libqtest.c
index eb00f13..6ec56a6 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -758,8 +758,21 @@ void qtest_add_func(const char *str, void (*fn)(void))
     g_free(path);
 }
 
+void qtest_add_data_func_full(const char *str, void *data,
+                              void (*fn)(const void *),
+                              GDestroyNotify data_free_func)
+{
+#if GLIB_CHECK_VERSION(2, 34, 0)
+    gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
+    g_test_add_data_func_full(path, data, fn, data_free_func);
+    g_free(path);
+#else
+    qtest_add_data_func(str, data, fn);
+#endif
+}
+
 void qtest_add_data_func(const char *str, const void *data,
-                         void (*fn)(const void *))
+                              void (*fn)(const void *))
 {
     gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
     g_test_add_data_func(path, data, fn);
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 37f37ad..e4c9c39 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -413,15 +413,20 @@ const char *qtest_get_arch(void);
 void qtest_add_func(const char *str, void (*fn)(void));
 
 /**
- * qtest_add_data_func:
+ * qtest_add_data_func_full:
  * @str: Test case path.
  * @data: Test case data
  * @fn: Test case function
+ * @data_free_func: GDestroyNotify for data
  *
  * Add a GTester testcase with the given name, data and function.
  * The path is prefixed with the architecture under test, as
  * returned by qtest_get_arch().
  */
+void qtest_add_data_func_full(const char *str, void *data,
+                              void (*fn)(const void *),
+                              GDestroyNotify data_free_func);
+
 void qtest_add_data_func(const char *str, const void *data,
                          void (*fn)(const void *));
 
-- 
2.9.0

Re: [Qemu-devel] [PATCH v2 33/37] tests: add qtest_add_data_func_full

[Eric Blake]

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

On 07/28/2016 08:38 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Allows to specify a destroy function for the test data.

"Allows to" is not idiomatic English. Alternatives that sound better are
"Allows $who to specify" (most simply, "Allows one to"), or "Allows
specifying"

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/libqtest.c | 15 ++++++++++++++-
>  tests/libqtest.h |  7 ++++++-
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index eb00f13..6ec56a6 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -758,8 +758,21 @@ void qtest_add_func(const char *str, void (*fn)(void))
>      g_free(path);
>  }
>  
> +void qtest_add_data_func_full(const char *str, void *data,
> +                              void (*fn)(const void *),
> +                              GDestroyNotify data_free_func)
> +{
> +#if GLIB_CHECK_VERSION(2, 34, 0)
> +    gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
> +    g_test_add_data_func_full(path, data, fn, data_free_func);
> +    g_free(path);
> +#else
> +    qtest_add_data_func(str, data, fn);
> +#endif

The commit message doesn't mention that the code is dependent on glib
versions, nor that you are still leaking the data (data_free_func
remains uncalled) on older glib.  If it is intentional (under the
argument that "anyone running on older glib can't care too much about
memory leaks encountered only by the testsuite, and the leaks don't
affect main qemu"), then stating that in the commit message would let me
feel more comfortable giving an R-b.

Is there anything we can do even in older glib to unconditionally invoke
the cleanup function in the right places?

> +
>  void qtest_add_data_func(const char *str, const void *data,
> -                         void (*fn)(const void *))
> +                              void (*fn)(const void *))

Why the spurious indentation change?


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


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

Re: [Qemu-devel] [PATCH v2 33/37] tests: add qtest_add_data_func_full

[Marc-André Lureau]

Hi

On Fri, Jul 29, 2016 at 2:16 AM, Eric Blake <eblake@redhat.com> wrote:
> On 07/28/2016 08:38 AM, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Allows to specify a destroy function for the test data.
>
> "Allows to" is not idiomatic English. Alternatives that sound better are
> "Allows $who to specify" (most simply, "Allows one to"), or "Allows
> specifying"
>
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  tests/libqtest.c | 15 ++++++++++++++-
>>  tests/libqtest.h |  7 ++++++-
>>  2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> index eb00f13..6ec56a6 100644
>> --- a/tests/libqtest.c
>> +++ b/tests/libqtest.c
>> @@ -758,8 +758,21 @@ void qtest_add_func(const char *str, void (*fn)(void))
>>      g_free(path);
>>  }
>>
>> +void qtest_add_data_func_full(const char *str, void *data,
>> +                              void (*fn)(const void *),
>> +                              GDestroyNotify data_free_func)
>> +{
>> +#if GLIB_CHECK_VERSION(2, 34, 0)
>> +    gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
>> +    g_test_add_data_func_full(path, data, fn, data_free_func);
>> +    g_free(path);
>> +#else
>> +    qtest_add_data_func(str, data, fn);
>> +#endif
>
> The commit message doesn't mention that the code is dependent on glib
> versions, nor that you are still leaking the data (data_free_func
> remains uncalled) on older glib.  If it is intentional (under the
> argument that "anyone running on older glib can't care too much about
> memory leaks encountered only by the testsuite, and the leaks don't
> affect main qemu"), then stating that in the commit message would let me
> feel more comfortable giving an R-b.

ok

> Is there anything we can do even in older glib to unconditionally invoke
> the cleanup function in the right places?

Yes, calling the undocumented g_test_add_vtable(), with some casts. Is
that acceptable?

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 33/37] tests: add qtest_add_data_func_full

[Eric Blake]

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

On 07/29/2016 02:48 AM, Marc-André Lureau wrote:

>>> +#if GLIB_CHECK_VERSION(2, 34, 0)
>>> +    gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
>>> +    g_test_add_data_func_full(path, data, fn, data_free_func);
>>> +    g_free(path);
>>> +#else
>>> +    qtest_add_data_func(str, data, fn);
>>> +#endif
>>
>> The commit message doesn't mention that the code is dependent on glib
>> versions, nor that you are still leaking the data (data_free_func
>> remains uncalled) on older glib.  If it is intentional (under the
>> argument that "anyone running on older glib can't care too much about
>> memory leaks encountered only by the testsuite, and the leaks don't
>> affect main qemu"), then stating that in the commit message would let me
>> feel more comfortable giving an R-b.
> 
> ok
> 
>> Is there anything we can do even in older glib to unconditionally invoke
>> the cleanup function in the right places?
> 
> Yes, calling the undocumented g_test_add_vtable(), with some casts. Is
> that acceptable?

Since the older versions aren't changing, then yes, I would view
back-compat casts along with a note saying "remove this once we can
require new-enough glib" as acceptable.

-- 
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 v2 34/37] tests: pc-cpu-test leaks fixes

[marcandre.lureau]

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

The path is allocated and should be freed.

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

Use a destroy function for the PCTestData.

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

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

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

[Eric Blake]

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

On 07/28/2016 08:38 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> The path is allocated and should be freed.
> 
> The qmp response should be unref, but then 'machine' must be duplicated.
> 
> Use a destroy function for the PCTestData.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/pc-cpu-test.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 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 v2 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/postcopy-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
index 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

[Qemu-devel] [PATCH v2 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

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

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

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

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

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

[Qemu-devel] [PATCH v2 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

Re: [Qemu-devel] [PATCH v2 37/37] tests: fix postcopy-test leaks

[Eric Blake]

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

On 07/28/2016 08:38 AM, marcandre.lureau@redhat.com wrote:
> 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(-)

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

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


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

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

[Eric Blake]

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

On 07/28/2016 08:37 AM, marcandre.lureau@redhat.com wrote:
> 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.

Are you trying to get this series into 2.7?  It qualifies as bug fixes,
so it should mostly be safe even though we are in hard freeze.


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


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