[Qemu-devel] [PATCH for-2.3 0/2] qtest: Fix remaining test paths to include architecture

[Qemu-devel] [PATCH for-2.3 0/2] qtest: Fix remaining test paths to include architecture

[Andreas Färber]

Hello,

This mini-series is a follow-up, fixing the last remaining GTester paths
to indicate the architecture they are executed for (i386 vs. x86_64).

Unlike the previous patch, there is no libqtest wrapper function adding the
architecture yet; other tests manually constructed a correct path to pass to
g_test_add_data_func(). For convenience, add such a helper function.

Based on "[PATCH for-2.3] fw_cfg-test: Fix test path to include architecture",
which missed -rc1. If there's no objection, I'll put these in a pull for -rc2.

Regards,
Andreas

Andreas Färber (2):
  qtest: Add qtest_add_data_func() wrapper function
  i440fx-test: Fix test paths to include architecture

 tests/i440fx-test.c |  8 +++++---
 tests/libqtest.c    |  7 +++++++
 tests/libqtest.h    | 12 ++++++++++++
 3 files changed, 24 insertions(+), 3 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH for-2.3 1/2] qtest: Add qtest_add_data_func() wrapper function

[Andreas Färber]

It calls g_test_add_data_func() with a path supplemented by the
architecture, like qtest_add_func() does.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 tests/libqtest.c |  7 +++++++
 tests/libqtest.h | 12 ++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 9a92aa7..e3f68d1 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -652,6 +652,13 @@ void qtest_add_func(const char *str, void (*fn))
     g_free(path);
 }
 
+void qtest_add_data_func(const char *str, void *data, void (*fn))
+{
+    gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
+    g_test_add_data_func(path, data, fn);
+    g_free(path);
+}
+
 void qtest_memwrite(QTestState *s, uint64_t addr, const void *data, size_t size)
 {
     const uint8_t *ptr = data;
diff --git a/tests/libqtest.h b/tests/libqtest.h
index e7413d5..e363f7f 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -345,6 +345,18 @@ const char *qtest_get_arch(void);
 void qtest_add_func(const char *str, void (*fn));
 
 /**
+ * qtest_add_data_func:
+ * @str: Test case path.
+ * @data: Test case data
+ * @fn: Test case function
+ *
+ * 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(const char *str, void *data, void (*fn));
+
+/**
  * qtest_start:
  * @args: other arguments to pass to QEMU
  *
-- 
2.1.4

Re: [Qemu-devel] [PATCH for-2.3 1/2] qtest: Add qtest_add_data_func() wrapper function

[John Snow]

On 03/24/2015 06:45 PM, Andreas Färber wrote:
> It calls g_test_add_data_func() with a path supplemented by the
> architecture, like qtest_add_func() does.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>   tests/libqtest.c |  7 +++++++
>   tests/libqtest.h | 12 ++++++++++++
>   2 files changed, 19 insertions(+)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 9a92aa7..e3f68d1 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -652,6 +652,13 @@ void qtest_add_func(const char *str, void (*fn))
>       g_free(path);
>   }
>
> +void qtest_add_data_func(const char *str, void *data, void (*fn))
> +{
> +    gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
> +    g_test_add_data_func(path, data, fn);
> +    g_free(path);
> +}
> +
>   void qtest_memwrite(QTestState *s, uint64_t addr, const void *data, size_t size)
>   {
>       const uint8_t *ptr = data;
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index e7413d5..e363f7f 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -345,6 +345,18 @@ const char *qtest_get_arch(void);
>   void qtest_add_func(const char *str, void (*fn));
>
>   /**
> + * qtest_add_data_func:
> + * @str: Test case path.
> + * @data: Test case data
> + * @fn: Test case function
> + *
> + * 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(const char *str, void *data, void (*fn));
> +
> +/**
>    * qtest_start:
>    * @args: other arguments to pass to QEMU
>    *
>

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

[Qemu-devel] [PATCH for-2.3 2/2] i440fx-test: Fix test paths to include architecture

[Andreas Färber]

Replace g_test_add_func() with new qtest_add_func() and modify the path
passed to g_test_add() macro.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 tests/i440fx-test.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c
index a3f7279..bc3f54c 100644
--- a/tests/i440fx-test.c
+++ b/tests/i440fx-test.c
@@ -383,8 +383,10 @@ static void add_firmware_test(const char *testpath,
                               void (*setup_fixture)(FirmwareTestFixture *f,
                                                     gconstpointer test_data))
 {
-    g_test_add(testpath, FirmwareTestFixture, NULL, setup_fixture,
+    char *path = g_strdup_printf("/%s%s", qtest_get_arch(), testpath);
+    g_test_add(path, FirmwareTestFixture, NULL, setup_fixture,
                test_i440fx_firmware, NULL);
+    g_free(path);
 }
 
 static void request_bios(FirmwareTestFixture *fixture,
@@ -408,8 +410,8 @@ int main(int argc, char **argv)
 
     data.num_cpus = 1;
 
-    g_test_add_data_func("/i440fx/defaults", &data, test_i440fx_defaults);
-    g_test_add_data_func("/i440fx/pam", &data, test_i440fx_pam);
+    qtest_add_data_func("i440fx/defaults", &data, test_i440fx_defaults);
+    qtest_add_data_func("i440fx/pam", &data, test_i440fx_pam);
     add_firmware_test("/i440fx/firmware/bios", request_bios);
     add_firmware_test("/i440fx/firmware/pflash", request_pflash);
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH for-2.3 2/2] i440fx-test: Fix test paths to include architecture

[John Snow]

On 03/24/2015 06:45 PM, Andreas Färber wrote:
> Replace g_test_add_func() with new qtest_add_func() and modify the path
> passed to g_test_add() macro.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>   tests/i440fx-test.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c
> index a3f7279..bc3f54c 100644
> --- a/tests/i440fx-test.c
> +++ b/tests/i440fx-test.c
> @@ -383,8 +383,10 @@ static void add_firmware_test(const char *testpath,
>                                 void (*setup_fixture)(FirmwareTestFixture *f,
>                                                       gconstpointer test_data))
>   {
> -    g_test_add(testpath, FirmwareTestFixture, NULL, setup_fixture,
> +    char *path = g_strdup_printf("/%s%s", qtest_get_arch(), testpath);
> +    g_test_add(path, FirmwareTestFixture, NULL, setup_fixture,
>                  test_i440fx_firmware, NULL);
> +    g_free(path);
>   }
>

Is it not worth adding an even more generic wrapper to prevent future 
desynch from our preferred path format?

>   static void request_bios(FirmwareTestFixture *fixture,
> @@ -408,8 +410,8 @@ int main(int argc, char **argv)
>
>       data.num_cpus = 1;
>
> -    g_test_add_data_func("/i440fx/defaults", &data, test_i440fx_defaults);
> -    g_test_add_data_func("/i440fx/pam", &data, test_i440fx_pam);
> +    qtest_add_data_func("i440fx/defaults", &data, test_i440fx_defaults);
> +    qtest_add_data_func("i440fx/pam", &data, test_i440fx_pam);
>       add_firmware_test("/i440fx/firmware/bios", request_bios);
>       add_firmware_test("/i440fx/firmware/pflash", request_pflash);
>
>

Similar to above, is it worth replacing other test's usage of 
g_test_add_data_func to force this standard path format everywhere?

--js

Re: [Qemu-devel] [PATCH for-2.3 2/2] i440fx-test: Fix test paths to include architecture

[Andreas Färber]

Am 25.03.2015 um 00:09 schrieb John Snow:
> On 03/24/2015 06:45 PM, Andreas Färber wrote:
>> Replace g_test_add_func() with new qtest_add_func() and modify the path
>> passed to g_test_add() macro.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>   tests/i440fx-test.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c
>> index a3f7279..bc3f54c 100644
>> --- a/tests/i440fx-test.c
>> +++ b/tests/i440fx-test.c
>> @@ -383,8 +383,10 @@ static void add_firmware_test(const char *testpath,
>>                                 void
>> (*setup_fixture)(FirmwareTestFixture *f,
>>                                                       gconstpointer
>> test_data))
>>   {
>> -    g_test_add(testpath, FirmwareTestFixture, NULL, setup_fixture,
>> +    char *path = g_strdup_printf("/%s%s", qtest_get_arch(), testpath);
>> +    g_test_add(path, FirmwareTestFixture, NULL, setup_fixture,
>>                  test_i440fx_firmware, NULL);
>> +    g_free(path);
>>   }
>>
> 
> Is it not worth adding an even more generic wrapper to prevent future
> desynch from our preferred path format?

As mentioned in the commit message, g_test_add() is a macro, not a
function, so seemed more complicated to wrap. Can you post a patch if
you have an idea? :)

>>   static void request_bios(FirmwareTestFixture *fixture,
>> @@ -408,8 +410,8 @@ int main(int argc, char **argv)
>>
>>       data.num_cpus = 1;
>>
>> -    g_test_add_data_func("/i440fx/defaults", &data,
>> test_i440fx_defaults);
>> -    g_test_add_data_func("/i440fx/pam", &data, test_i440fx_pam);
>> +    qtest_add_data_func("i440fx/defaults", &data, test_i440fx_defaults);
>> +    qtest_add_data_func("i440fx/pam", &data, test_i440fx_pam);
>>       add_firmware_test("/i440fx/firmware/bios", request_bios);
>>       add_firmware_test("/i440fx/firmware/pflash", request_pflash);
>>
>>
> 
> Similar to above, is it worth replacing other test's usage of
> g_test_add_data_func to force this standard path format everywhere?

I reviewed the test output from -rc0 while going through test failures.
fw_cfg-test and i440fx-test were the only nits I found. I wondered
whether we might poison the g_test_* versions or convert the other
tests, but did not want to unnecessarily risk this for 2.3.

Another issue that I've come across is that several tests misuse qmp(),
ignoring the return value instead of using qmp_discard_response().

Also I had the impression that my qom-test has noticeably degraded in
performance, and I wondered whether Paolo's changes to qmp(), parsing
the string argument into a QObject, might be to blame, given that a lot
of QMP qom-gets are performed by my test, and the number of objects and
properties tested keeps increasing (MemoryRegion, qemu_irq, machines).

Thanks a lot for your review.

Regards,
Andreas

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

Re: [Qemu-devel] [PATCH for-2.3 2/2] i440fx-test: Fix test paths to include architecture

[John Snow]

On 03/24/2015 07:20 PM, Andreas Färber wrote:
> Am 25.03.2015 um 00:09 schrieb John Snow:
>> On 03/24/2015 06:45 PM, Andreas Färber wrote:
>>> Replace g_test_add_func() with new qtest_add_func() and modify the path
>>> passed to g_test_add() macro.
>>>
>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>> ---
>>>    tests/i440fx-test.c | 8 +++++---
>>>    1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c
>>> index a3f7279..bc3f54c 100644
>>> --- a/tests/i440fx-test.c
>>> +++ b/tests/i440fx-test.c
>>> @@ -383,8 +383,10 @@ static void add_firmware_test(const char *testpath,
>>>                                  void
>>> (*setup_fixture)(FirmwareTestFixture *f,
>>>                                                        gconstpointer
>>> test_data))
>>>    {
>>> -    g_test_add(testpath, FirmwareTestFixture, NULL, setup_fixture,
>>> +    char *path = g_strdup_printf("/%s%s", qtest_get_arch(), testpath);
>>> +    g_test_add(path, FirmwareTestFixture, NULL, setup_fixture,
>>>                   test_i440fx_firmware, NULL);
>>> +    g_free(path);
>>>    }
>>>
>>
>> Is it not worth adding an even more generic wrapper to prevent future
>> desynch from our preferred path format?
>
> As mentioned in the commit message, g_test_add() is a macro, not a
> function, so seemed more complicated to wrap. Can you post a patch if
> you have an idea? :)
>

Not enough of one to bother delaying this for 2.3 -- I did forget this 
was a macro.

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

>>>    static void request_bios(FirmwareTestFixture *fixture,
>>> @@ -408,8 +410,8 @@ int main(int argc, char **argv)
>>>
>>>        data.num_cpus = 1;
>>>
>>> -    g_test_add_data_func("/i440fx/defaults", &data,
>>> test_i440fx_defaults);
>>> -    g_test_add_data_func("/i440fx/pam", &data, test_i440fx_pam);
>>> +    qtest_add_data_func("i440fx/defaults", &data, test_i440fx_defaults);
>>> +    qtest_add_data_func("i440fx/pam", &data, test_i440fx_pam);
>>>        add_firmware_test("/i440fx/firmware/bios", request_bios);
>>>        add_firmware_test("/i440fx/firmware/pflash", request_pflash);
>>>
>>>
>>
>> Similar to above, is it worth replacing other test's usage of
>> g_test_add_data_func to force this standard path format everywhere?
>
> I reviewed the test output from -rc0 while going through test failures.
> fw_cfg-test and i440fx-test were the only nits I found. I wondered
> whether we might poison the g_test_* versions or convert the other
> tests, but did not want to unnecessarily risk this for 2.3.
>

I can get behind "change as little as possible."

> Another issue that I've come across is that several tests misuse qmp(),
> ignoring the return value instead of using qmp_discard_response().
>
> Also I had the impression that my qom-test has noticeably degraded in
> performance, and I wondered whether Paolo's changes to qmp(), parsing
> the string argument into a QObject, might be to blame, given that a lot
> of QMP qom-gets are performed by my test, and the number of objects and
> properties tested keeps increasing (MemoryRegion, qemu_irq, machines).

I can always fire up valgrind and figure out something to point a finger 
at :)

> Thanks a lot for your review.
>
> Regards,
> Andreas
>

Re: [Qemu-devel] [PATCH for-2.3 2/2] i440fx-test: Fix test paths to include architecture

[Paolo Bonzini]

On 25/03/2015 00:20, Andreas Färber wrote:
>>> >> -    g_test_add(testpath, FirmwareTestFixture, NULL, setup_fixture,
>>> >> +    char *path = g_strdup_printf("/%s%s", qtest_get_arch(), testpath);
>>> >> +    g_test_add(path, FirmwareTestFixture, NULL, setup_fixture,
>>> >>                  test_i440fx_firmware, NULL);
>>> >> +    g_free(path);
>>> >>   }
>>> >>
>> > 
>> > Is it not worth adding an even more generic wrapper to prevent future
>> > desynch from our preferred path format?
> As mentioned in the commit message, g_test_add() is a macro, not a
> function, so seemed more complicated to wrap. Can you post a patch if
> you have an idea? :)
> 

You would have to wrap g_test_add_vtable with qtest_add_vtable, and then
add a macro that mimicks g_test_add:

/* hook up a test with fixture under test path */
#define qtest_add(testpath, Fixture, tdata, fsetup, ftest, fteardown) \
                                        G_STMT_START {                  \
                                         void (*add_vtable) (const char*,       \
                                                    gsize,             \
                                                    gconstpointer,     \
                                                    void (*) (Fixture*, gconstpointer),   \
                                                    void (*) (Fixture*, gconstpointer),   \
                                                    void (*) (Fixture*, gconstpointer)) =  (void (*) (const gchar *, gsize, gconstpointer, void (
*) (Fixture*, gconstpointer), void (*) (Fixture*, gconstpointer), void (*) (Fixture*, gconstpointer))) qtest_add_vtable; \
                                         add_vtable \
                                          (testpath, sizeof (Fixture), tdata, fsetup, ftest, fteardown); \
                                        } G_STMT_END


Paolo

Re: [Qemu-devel] [PATCH for-2.3 0/2] qtest: Fix remaining test paths to include architecture

[Stefan Hajnoczi]

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

On Tue, Mar 24, 2015 at 11:45:46PM +0100, Andreas Färber wrote:
> Hello,
> 
> This mini-series is a follow-up, fixing the last remaining GTester paths
> to indicate the architecture they are executed for (i386 vs. x86_64).
> 
> Unlike the previous patch, there is no libqtest wrapper function adding the
> architecture yet; other tests manually constructed a correct path to pass to
> g_test_add_data_func(). For convenience, add such a helper function.
> 
> Based on "[PATCH for-2.3] fw_cfg-test: Fix test path to include architecture",
> which missed -rc1. If there's no objection, I'll put these in a pull for -rc2.
> 
> Regards,
> Andreas
> 
> Andreas Färber (2):
>   qtest: Add qtest_add_data_func() wrapper function
>   i440fx-test: Fix test paths to include architecture
> 
>  tests/i440fx-test.c |  8 +++++---
>  tests/libqtest.c    |  7 +++++++
>  tests/libqtest.h    | 12 ++++++++++++
>  3 files changed, 24 insertions(+), 3 deletions(-)
> 
> -- 
> 2.1.4
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]