[Qemu-devel] [PATCH for-2.5 0/2] tests: A cleanup and a fix

[Qemu-devel] [PATCH for-2.5 0/2] tests: A cleanup and a fix

[Markus Armbruster]

PATCH 1 cleans up unnecessary type punning.

PATCH 2 plugs a massive memory leak in qom-test.  I think it would be
nice to have in 2.5, but at this late stage, it's really up to the
maintainer.

Marc-André Lureau (1):
  qom-test: fix qmp() leaks

Markus Armbruster (1):
  tests: Use proper functions types instead of void (*fn)

 tests/ide-test.c        |  4 ++--
 tests/libqtest.c        | 13 +++++++++----
 tests/libqtest.h        |  6 +++---
 tests/qom-test.c        | 21 ++++++++++++++-------
 tests/vhost-user-test.c |  3 ++-
 5 files changed, 30 insertions(+), 17 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 1/2] tests: Use proper functions types instead of void (*fn)

[Markus Armbruster]

We have several function parameters declared as void (*fn).  This is
just a stupid way to write void *, and the only purpose writing it
like that could serve is obscuring the sin of bypassing the type
system without need.

The original sin is commit 49ee359: its qtest_add_func() is a wrapper
for g_test_add_func().  Fix the parameter type to match
g_test_add_func()'s.  This uncovers type errors in ide-test.c; fix
them.

Commit 7949c0e faithfully repeated the sin for qtest_add_data_func().
Fix it the same way, along with a harmless type error uncovered in
vhost-user-test.c.

Commit 063c23d repeated it for qtest_add_abrt_handler().  The screwy
parameter gets assigned to GHook member func, so change its type to
match.  Requires wrapping kill_qemu() to keep the type checker happy.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/ide-test.c        |  4 ++--
 tests/libqtest.c        | 13 +++++++++----
 tests/libqtest.h        |  6 +++---
 tests/vhost-user-test.c |  3 ++-
 4 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/tests/ide-test.c b/tests/ide-test.c
index c3aacd2..b864701 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -593,12 +593,12 @@ static void test_flush_nodev(void)
     ide_test_quit();
 }
 
-static void test_pci_retry_flush(const char *machine)
+static void test_pci_retry_flush(void)
 {
     test_retry_flush("pc");
 }
 
-static void test_isa_retry_flush(const char *machine)
+static void test_isa_retry_flush(void)
 {
     test_retry_flush("isapc");
 }
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 9753161..c52ceb2 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -110,6 +110,11 @@ static void kill_qemu(QTestState *s)
     }
 }
 
+static void kill_qemu_hook_func(void *s)
+{
+    kill_qemu(s);
+}
+
 static void sigabrt_handler(int signo)
 {
     g_hook_list_invoke(&abrt_hooks, FALSE);
@@ -133,7 +138,7 @@ static void cleanup_sigabrt_handler(void)
     sigaction(SIGABRT, &sigact_old, NULL);
 }
 
-void qtest_add_abrt_handler(void (*fn), const void *data)
+void qtest_add_abrt_handler(GHookFunc fn, const void *data)
 {
     GHook *hook;
 
@@ -170,7 +175,7 @@ QTestState *qtest_init(const char *extra_args)
     sock = init_socket(socket_path);
     qmpsock = init_socket(qmp_socket_path);
 
-    qtest_add_abrt_handler(kill_qemu, s);
+    qtest_add_abrt_handler(kill_qemu_hook_func, s);
 
     s->qemu_pid = fork();
     if (s->qemu_pid == 0) {
@@ -755,14 +760,14 @@ void qtest_memread(QTestState *s, uint64_t addr, void *data, size_t size)
     g_strfreev(args);
 }
 
-void qtest_add_func(const char *str, void (*fn))
+void qtest_add_func(const char *str, GTestFunc fn)
 {
     gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
     g_test_add_func(path, fn);
     g_free(path);
 }
 
-void qtest_add_data_func(const char *str, const void *data, void (*fn))
+void qtest_add_data_func(const char *str, const void *data, GTestDataFunc fn)
 {
     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 df08745..d11b5a4 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -416,7 +416,7 @@ const char *qtest_get_arch(void);
  * The path is prefixed with the architecture under test, as
  * returned by qtest_get_arch().
  */
-void qtest_add_func(const char *str, void (*fn));
+void qtest_add_func(const char *str, GTestFunc fn);
 
 /**
  * qtest_add_data_func:
@@ -428,7 +428,7 @@ void qtest_add_func(const char *str, void (*fn));
  * The path is prefixed with the architecture under test, as
  * returned by qtest_get_arch().
  */
-void qtest_add_data_func(const char *str, const void *data, void (*fn));
+void qtest_add_data_func(const char *str, const void *data, GTestDataFunc fn);
 
 /**
  * qtest_add:
@@ -450,7 +450,7 @@ void qtest_add_data_func(const char *str, const void *data, void (*fn));
         g_free(path); \
     } while (0)
 
-void qtest_add_abrt_handler(void (*fn), const void *data);
+void qtest_add_abrt_handler(GHookFunc fn, const void *data);
 
 /**
  * qtest_start:
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index e4c36af..814caf9 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -172,8 +172,9 @@ static void wait_for_fds(TestServer *s)
     g_mutex_unlock(&s->data_mutex);
 }
 
-static void read_guest_mem(TestServer *s)
+static void read_guest_mem(const void *data)
 {
+    TestServer *s = (void *)data;
     uint32_t *guest_mem;
     int i, j;
     size_t size;
-- 
2.4.3

Re: [Qemu-devel] [PATCH for-2.5 1/2] tests: Use proper functions types instead of void (*fn)

[Eric Blake]

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

On 12/02/2015 01:20 PM, Markus Armbruster wrote:
> We have several function parameters declared as void (*fn).  This is
> just a stupid way to write void *, and the only purpose writing it
> like that could serve is obscuring the sin of bypassing the type
> system without need.

Presumably, someone meant to write 'void (*fn)()' or some other argument
list to designate fn as a function pointer; but I agree with the
approach you took of using typedefs rather than spelling it out raw.

> 
> The original sin is commit 49ee359: its qtest_add_func() is a wrapper
> for g_test_add_func().  Fix the parameter type to match
> g_test_add_func()'s.  This uncovers type errors in ide-test.c; fix
> them.
> 
> Commit 7949c0e faithfully repeated the sin for qtest_add_data_func().
> Fix it the same way, along with a harmless type error uncovered in
> vhost-user-test.c.
> 
> Commit 063c23d repeated it for qtest_add_abrt_handler().  The screwy
> parameter gets assigned to GHook member func, so change its type to
> match.  Requires wrapping kill_qemu() to keep the type checker happy.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/ide-test.c        |  4 ++--
>  tests/libqtest.c        | 13 +++++++++----
>  tests/libqtest.h        |  6 +++---
>  tests/vhost-user-test.c |  3 ++-
>  4 files changed, 16 insertions(+), 10 deletions(-)
> 

> +++ b/tests/libqtest.c
> @@ -110,6 +110,11 @@ static void kill_qemu(QTestState *s)
>      }
>  }
>  
> +static void kill_qemu_hook_func(void *s)
> +{
> +    kill_qemu(s);

Getting the implicit conversion from void * to QTestState *, without
having to cast the actual function signature.  Makes sense.

> @@ -133,7 +138,7 @@ static void cleanup_sigabrt_handler(void)
>      sigaction(SIGABRT, &sigact_old, NULL);
>  }
>  
> -void qtest_add_abrt_handler(void (*fn), const void *data)
> +void qtest_add_abrt_handler(GHookFunc fn, const void *data)

I had to look it up:

/usr/include/glib-2.0/glib/ghook.h:typedef void		(*GHookFunc)	
(gpointer	 data);

which is the same as

void (*fn)(void *)

> @@ -755,14 +760,14 @@ void qtest_memread(QTestState *s, uint64_t addr, void *data, size_t size)
>      g_strfreev(args);
>  }
>  
> -void qtest_add_func(const char *str, void (*fn))
> +void qtest_add_func(const char *str, GTestFunc fn)
>  {
>      gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
>      g_test_add_func(path, fn);

/usr/include/glib-2.0/glib/gtestutils.h:typedef void (*GTestFunc)
 (void);

>      g_free(path);
>  }
>  
> -void qtest_add_data_func(const char *str, const void *data, void (*fn))
> +void qtest_add_data_func(const char *str, const void *data, GTestDataFunc fn)

/usr/include/glib-2.0/glib/gtestutils.h:typedef void (*GTestDataFunc)
 (gconstpointer user_data);

> +++ b/tests/vhost-user-test.c
> @@ -172,8 +172,9 @@ static void wait_for_fds(TestServer *s)
>      g_mutex_unlock(&s->data_mutex);
>  }
>  
> -static void read_guest_mem(TestServer *s)
> +static void read_guest_mem(const void *data)
>  {
> +    TestServer *s = (void *)data;

And here you have to cast away const.

At any rate, all the fixes look sane, and the fact that it compiles with
now-tighter types is in its favor.

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 for-2.5 1/2] tests: Use proper functions types instead of void (*fn)

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 12/02/2015 01:20 PM, Markus Armbruster wrote:
>> We have several function parameters declared as void (*fn).  This is
>> just a stupid way to write void *, and the only purpose writing it
>> like that could serve is obscuring the sin of bypassing the type
>> system without need.
>
> Presumably, someone meant to write 'void (*fn)()' or some other argument
> list to designate fn as a function pointer; but I agree with the
> approach you took of using typedefs rather than spelling it out raw.
>
>> 
>> The original sin is commit 49ee359: its qtest_add_func() is a wrapper
>> for g_test_add_func().  Fix the parameter type to match
>> g_test_add_func()'s.  This uncovers type errors in ide-test.c; fix
>> them.
>> 
>> Commit 7949c0e faithfully repeated the sin for qtest_add_data_func().
>> Fix it the same way, along with a harmless type error uncovered in
>> vhost-user-test.c.
>> 
>> Commit 063c23d repeated it for qtest_add_abrt_handler().  The screwy
>> parameter gets assigned to GHook member func, so change its type to
>> match.  Requires wrapping kill_qemu() to keep the type checker happy.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/ide-test.c        |  4 ++--
>>  tests/libqtest.c        | 13 +++++++++----
>>  tests/libqtest.h        |  6 +++---
>>  tests/vhost-user-test.c |  3 ++-
>>  4 files changed, 16 insertions(+), 10 deletions(-)
>> 
>
>> +++ b/tests/libqtest.c
>> @@ -110,6 +110,11 @@ static void kill_qemu(QTestState *s)
>>      }
>>  }
>>  
>> +static void kill_qemu_hook_func(void *s)
>> +{
>> +    kill_qemu(s);
>
> Getting the implicit conversion from void * to QTestState *, without
> having to cast the actual function signature.  Makes sense.

I'm not fond of wrappers, but I really dislike function pointer casts;
I've seen too many of them with undefined behavior.

>> @@ -133,7 +138,7 @@ static void cleanup_sigabrt_handler(void)
>>      sigaction(SIGABRT, &sigact_old, NULL);
>>  }
>>  
>> -void qtest_add_abrt_handler(void (*fn), const void *data)
>> +void qtest_add_abrt_handler(GHookFunc fn, const void *data)
>
> I had to look it up:
>
> /usr/include/glib-2.0/glib/ghook.h:typedef void		(*GHookFunc)	
> (gpointer	 data);
>
> which is the same as
>
> void (*fn)(void *)

Speaking of function pointer cases...  note struct GHook's member func's
documentation:

    gpointer func; the function to call when this hook is invoked. The
                   possible signatures for this function are GHookFunc
                   and GHookCheckFunc

GHookFunc is void (*)(gpointer data), and to be used when the hook is
passed to g_hook_list_invoke().

GHookCheckFunc is gboolean (*)(gpointer data), and to be used when the
hook is passed to g_hook_list_invoke_check().

Undefined behavior when you get it wrong.

This interface displays no evidence of adult supervision.  Oh well,
"strong typing is for people with weak memories."

>> @@ -755,14 +760,14 @@ void qtest_memread(QTestState *s, uint64_t addr, void *data, size_t size)
>>      g_strfreev(args);
>>  }
>>  
>> -void qtest_add_func(const char *str, void (*fn))
>> +void qtest_add_func(const char *str, GTestFunc fn)
>>  {
>>      gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
>>      g_test_add_func(path, fn);
>
> /usr/include/glib-2.0/glib/gtestutils.h:typedef void (*GTestFunc)
>  (void);
>
>>      g_free(path);
>>  }
>>  
>> -void qtest_add_data_func(const char *str, const void *data, void (*fn))
>> +void qtest_add_data_func(const char *str, const void *data, GTestDataFunc fn)
>
> /usr/include/glib-2.0/glib/gtestutils.h:typedef void (*GTestDataFunc)
>  (gconstpointer user_data);
>
>> +++ b/tests/vhost-user-test.c
>> @@ -172,8 +172,9 @@ static void wait_for_fds(TestServer *s)
>>      g_mutex_unlock(&s->data_mutex);
>>  }
>>  
>> -static void read_guest_mem(TestServer *s)
>> +static void read_guest_mem(const void *data)
>>  {
>> +    TestServer *s = (void *)data;
>
> And here you have to cast away const.
>
> At any rate, all the fixes look sane, and the fact that it compiles with
> now-tighter types is in its favor.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

[Qemu-devel] [PATCH] fixup! tests: Use proper functions types instead of void (*fn)

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/libqtest.c | 5 +++--
 tests/libqtest.h | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index c52ceb2..fa314e1 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -760,14 +760,15 @@ void qtest_memread(QTestState *s, uint64_t addr, void *data, size_t size)
     g_strfreev(args);
 }
 
-void qtest_add_func(const char *str, GTestFunc fn)
+void qtest_add_func(const char *str, void (*fn)(void))
 {
     gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
     g_test_add_func(path, fn);
     g_free(path);
 }
 
-void qtest_add_data_func(const char *str, const void *data, GTestDataFunc fn)
+void qtest_add_data_func(const char *str, const void *data,
+                         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 d11b5a4..ebdd5bb 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -416,7 +416,7 @@ const char *qtest_get_arch(void);
  * The path is prefixed with the architecture under test, as
  * returned by qtest_get_arch().
  */
-void qtest_add_func(const char *str, GTestFunc fn);
+void qtest_add_func(const char *str, void (*fn)(void));
 
 /**
  * qtest_add_data_func:
@@ -428,7 +428,8 @@ void qtest_add_func(const char *str, GTestFunc fn);
  * The path is prefixed with the architecture under test, as
  * returned by qtest_get_arch().
  */
-void qtest_add_data_func(const char *str, const void *data, GTestDataFunc fn);
+void qtest_add_data_func(const char *str, const void *data,
+                         void (*fn)(const void *));
 
 /**
  * qtest_add:
-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 2/2] qom-test: fix qmp() leaks

[Markus Armbruster]

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

Before this patch ASAN reported:
SUMMARY: AddressSanitizer: 677165875 byte(s) leaked in 1272437 allocation(s)

After this patch:
SUMMARY: AddressSanitizer: 465 byte(s) leaked in 32 allocation(s)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <1448551895-871-1-git-send-email-marcandre.lureau@redhat.com>
[Straightforwardly rebased onto the previous patch]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qom-test.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/tests/qom-test.c b/tests/qom-test.c
index fde04e7..3e5e873 100644
--- a/tests/qom-test.c
+++ b/tests/qom-test.c
@@ -47,7 +47,7 @@ static bool is_blacklisted(const char *arch, const char *mach)
 static void test_properties(const char *path, bool recurse)
 {
     char *child_path;
-    QDict *response, *tuple;
+    QDict *response, *tuple, *tmp;
     QList *list;
     QListEntry *entry;
 
@@ -57,6 +57,7 @@ static void test_properties(const char *path, bool recurse)
     g_assert(response);
 
     if (!recurse) {
+        QDECREF(response);
         return;
     }
 
@@ -75,14 +76,16 @@ static void test_properties(const char *path, bool recurse)
         } else {
             const char *prop = qdict_get_str(tuple, "name");
             g_test_message("Testing property %s.%s", path, prop);
-            response = qmp("{ 'execute': 'qom-get',"
-                           "  'arguments': { 'path': %s,"
-                           "                 'property': %s } }",
-                           path, prop);
+            tmp = qmp("{ 'execute': 'qom-get',"
+                      "  'arguments': { 'path': %s,"
+                      "                 'property': %s } }",
+                      path, prop);
             /* qom-get may fail but should not, e.g., segfault. */
-            g_assert(response);
+            g_assert(tmp);
+            QDECREF(tmp);
         }
     }
+    QDECREF(response);
 }
 
 static void test_machine(gconstpointer data)
@@ -98,9 +101,11 @@ static void test_machine(gconstpointer data)
 
     response = qmp("{ 'execute': 'quit' }");
     g_assert(qdict_haskey(response, "return"));
+    QDECREF(response);
 
     qtest_end();
     g_free(args);
+    g_free((void *)machine);
 }
 
 static void add_machine_test_cases(void)
@@ -129,10 +134,12 @@ static void add_machine_test_cases(void)
         mname = qstring_get_str(qstr);
         if (!is_blacklisted(arch, mname)) {
             path = g_strdup_printf("qom/%s", mname);
-            qtest_add_data_func(path, mname, test_machine);
+            qtest_add_data_func(path, g_strdup(mname), test_machine);
         }
     }
+
     qtest_end();
+    QDECREF(response);
 }
 
 int main(int argc, char **argv)
-- 
2.4.3

Re: [Qemu-devel] [PATCH for-2.5 2/2] qom-test: fix qmp() leaks

[Eric Blake]

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

On 12/02/2015 01:20 PM, Markus Armbruster wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Before this patch ASAN reported:
> SUMMARY: AddressSanitizer: 677165875 byte(s) leaked in 1272437 allocation(s)
> 
> After this patch:
> SUMMARY: AddressSanitizer: 465 byte(s) leaked in 32 allocation(s)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Message-Id: <1448551895-871-1-git-send-email-marcandre.lureau@redhat.com>
> [Straightforwardly rebased onto the previous patch]
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/qom-test.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 

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

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


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

Re: [Qemu-devel] [PATCH for-2.5 2/2] qom-test: fix qmp() leaks

[Andreas Färber]

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

Am 02.12.2015 um 21:44 schrieb Eric Blake:
> On 12/02/2015 01:20 PM, Markus Armbruster wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Before this patch ASAN reported:
>> SUMMARY: AddressSanitizer: 677165875 byte(s) leaked in 1272437 allocation(s)
>>
>> After this patch:
>> SUMMARY: AddressSanitizer: 465 byte(s) leaked in 32 allocation(s)
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Message-Id: <1448551895-871-1-git-send-email-marcandre.lureau@redhat.com>
>> [Straightforwardly rebased onto the previous patch]
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/qom-test.c | 21 ++++++++++++++-------
>>  1 file changed, 14 insertions(+), 7 deletions(-)
>>
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks, both applied to qom-next.

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)


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

Re: [Qemu-devel] [PATCH for-2.5 0/2] tests: A cleanup and a fix

[Peter Maydell]

On 2 December 2015 at 20:20, Markus Armbruster <armbru@redhat.com> wrote:
> PATCH 1 cleans up unnecessary type punning.
>
> PATCH 2 plugs a massive memory leak in qom-test.  I think it would be
> nice to have in 2.5, but at this late stage, it's really up to the
> maintainer.

To go into 2.5 it needs to be reviewed and either be in a pull
request or have a request from the maintainer for me to apply it
directly by the end of today UK time.

Is the memory leak a regression, or have we always leaked and
not noticed?

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.5 0/2] tests: A cleanup and a fix

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 2 December 2015 at 20:20, Markus Armbruster <armbru@redhat.com> wrote:
>> PATCH 1 cleans up unnecessary type punning.
>>
>> PATCH 2 plugs a massive memory leak in qom-test.  I think it would be
>> nice to have in 2.5, but at this late stage, it's really up to the
>> maintainer.
>
> To go into 2.5 it needs to be reviewed and either be in a pull
> request or have a request from the maintainer for me to apply it
> directly by the end of today UK time.

Understood.

> Is the memory leak a regression, or have we always leaked and
> not noticed?

As far as I can see, a minor leak was introduced in commit 5c1904f, but
the major leakage comes from commit dc06cbd, both v2.0.

Re: [Qemu-devel] [PATCH for-2.5 0/2] tests: A cleanup and a fix

[Andreas Färber]

Am 03.12.2015 um 13:06 schrieb Markus Armbruster:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> On 2 December 2015 at 20:20, Markus Armbruster <armbru@redhat.com> wrote:
>>> PATCH 1 cleans up unnecessary type punning.
>>>
>>> PATCH 2 plugs a massive memory leak in qom-test.  I think it would be
>>> nice to have in 2.5, but at this late stage, it's really up to the
>>> maintainer.
>>
>> To go into 2.5 it needs to be reviewed and either be in a pull
>> request or have a request from the maintainer for me to apply it
>> directly by the end of today UK time.
> 
> Understood.
> 
>> Is the memory leak a regression, or have we always leaked and
>> not noticed?
> 
> As far as I can see, a minor leak was introduced in commit 5c1904f, but
> the major leakage comes from commit dc06cbd, both v2.0.

Well, as the qom-test maintainer I have been struggling with the two 1GB
Exynos4 machines on i586 hosts from 2.3 on already
(http://bugzilla.opensuse.org/show_bug.cgi?id=957379). While this
happens really early and we have been able to work around it by
decoupling compilation and execution of the tests a little (blaming make
or whomever for memory fragmentation), I'd like to investigate whether
this helps that regression. Otherwise memory cleanup has not been a
topic for non-QAPI allocations in tests either so it does not seem too
urgent to me if it's just cosmetic.

The preceding patch looks fine to me on a first look.

An unrelated question to consider going forward is whether we should
conditionalize and by default skip my property tests for time reasons.

Thanks,
Andreas

P.S. Since Peter insists on a pull, I will prepare one to finally fix
the make test breakage Marc and me both pointed out to no effect. If you
desire changes beyond dropping my one cosmetic patch, do comment.

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH for-2.5 0/2] tests: A cleanup and a fix

[Peter Maydell]

On 3 December 2015 at 12:57, Andreas Färber <afaerber@suse.de> wrote:
> P.S. Since Peter insists on a pull, I will prepare one to finally fix
> the make test breakage Marc and me both pointed out to no effect. If you
> desire changes beyond dropping my one cosmetic patch, do comment.

I don't insist on a pull. I said "either a pull request or
a request from the maintainer", ie I didn't want to apply the patches
without you saying you were OK with them. (Rereading that I should
have avoided the word 'request' in the second clause there, it
was confusing.)
If you want me to apply them directly I'm happy to do that.

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.5 0/2] tests: A cleanup and a fix

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 03.12.2015 um 13:06 schrieb Markus Armbruster:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> 
>>> On 2 December 2015 at 20:20, Markus Armbruster <armbru@redhat.com> wrote:
>>>> PATCH 1 cleans up unnecessary type punning.
>>>>
>>>> PATCH 2 plugs a massive memory leak in qom-test.  I think it would be
>>>> nice to have in 2.5, but at this late stage, it's really up to the
>>>> maintainer.
>>>
>>> To go into 2.5 it needs to be reviewed and either be in a pull
>>> request or have a request from the maintainer for me to apply it
>>> directly by the end of today UK time.
>> 
>> Understood.
>> 
>>> Is the memory leak a regression, or have we always leaked and
>>> not noticed?
>> 
>> As far as I can see, a minor leak was introduced in commit 5c1904f, but
>> the major leakage comes from commit dc06cbd, both v2.0.
>
> Well, as the qom-test maintainer I have been struggling with the two 1GB
> Exynos4 machines on i586 hosts from 2.3 on already
> (http://bugzilla.opensuse.org/show_bug.cgi?id=957379). While this
> happens really early and we have been able to work around it by
> decoupling compilation and execution of the tests a little (blaming make
> or whomever for memory fragmentation), I'd like to investigate whether
> this helps that regression. Otherwise memory cleanup has not been a
> topic for non-QAPI allocations in tests either so it does not seem too
> urgent to me if it's just cosmetic.

I agree chasing small memory leaks in tests is not a priority.  However,
tests dirtying hundreds of megabytes aren't so nice to developers with
small machines.  But as I said, for-2.5 is maintainer's discretion.

> The preceding patch looks fine to me on a first look.
>
> An unrelated question to consider going forward is whether we should
> conditionalize and by default skip my property tests for time reasons.

qom-test has caught bugs early for me, but it is slooow.  Perhaps mark
the tests for all the old machine types as slow?

[...]