Re: [Qemu-devel] [PATCH v2 2/4] i440fx-test: give each GTest case its own qtest

[Eduardo Habkost]

On Thu, Nov 28, 2013 at 07:09:13PM +0100, Laszlo Ersek wrote:
> The current two GTest cases, /i440fx/defaults and /i440fx/pam can share a
> qemu process, but the next two cases will need dedicated instances. It is
> messy (and order-dependent) to dynamically configure GTest cases one by
> one to start, stop, or keep the current qtest (*); let's just have each
> GTest work with its own qtest. The performance difference should be
> negligible.
> 
> (*) As g_test_run() can be invoked at most once per process startup, and
> it runs GTest cases in sequence, we'd need clumsy data structures to
> control each GTest case to start/stop/keep the qemu instance. Or, we'd
> have to code the same information into the test methods themselves, which
> would make them even more order-dependent.
> 
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
[...]
>  static void test_i440fx_defaults(gconstpointer opaque)
>  {
>      const TestData *s = opaque;
> +    QPCIBus *bus;
>      QPCIDevice *dev;
>      uint32_t value;
>  
> -    dev = qpci_device_find(s->bus, QPCI_DEVFN(0, 0));
> +    bus = test_start_get_bus(s);
> +    dev = qpci_device_find(bus, QPCI_DEVFN(0, 0));

You can use GTest setup/teardown functions to do this automatically on
all tests, but I am not sure the code really looks better when using it.
I gave it a try and it looks like this:

diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c
index 6ac46bf..c9d6f6a 100644
--- a/tests/i440fx-test.c
+++ b/tests/i440fx-test.c
@@ -25,15 +25,35 @@
 
 #define ARRAY_SIZE(array) (sizeof(array) / sizeof((array)[0]))
 
-typedef struct TestData
+typedef struct TestArgs
 {
     int num_cpus;
+} TestArgs;
+
+typedef struct TestData
+{
     QPCIBus *bus;
 } TestData;
 
-static void test_i440fx_defaults(gconstpointer opaque)
+static void test_setup(TestData *s, gconstpointer user_data)
+{
+    const TestArgs *args = user_data;
+    char *cmdline;
+
+    cmdline = g_strdup_printf("-smp %d", args->num_cpus);
+    qtest_start(cmdline);
+    g_free(cmdline);
+    s->bus = qpci_init_pc();
+}
+
+static void test_teardown(TestData *s, gconstpointer user_data)
+{
+    qtest_end();
+}
+
+static void test_i440fx_defaults(TestData *s, gconstpointer user_data)
 {
-    const TestData *s = opaque;
+    const TestArgs *args = user_data;
     QPCIDevice *dev;
     uint32_t value;
 
@@ -62,7 +82,7 @@ static void test_i440fx_defaults(gconstpointer opaque)
 
     /* 3.2.11 */
     value = qpci_config_readw(dev, 0x50); /* PMCCFG */
-    if (s->num_cpus == 1) { /* WPE */
+    if (args->num_cpus == 1) { /* WPE */
         g_assert(!(value & (1 << 15)));
     } else {
         g_assert((value & (1 << 15)));
@@ -176,9 +196,8 @@ static void write_area(uint32_t start, uint32_t end, 
uint8_t value)
     g_free(data);
 }
 
-static void test_i440fx_pam(gconstpointer opaque)
+static void test_i440fx_pam(TestData *s, gconstpointer user_data)
 {
-    const TestData *s = opaque;
     QPCIDevice *dev;
     int i;
     static struct {
@@ -258,26 +277,16 @@ static void test_i440fx_pam(gconstpointer opaque)
 
 int main(int argc, char **argv)
 {
-    TestData data;
-    char *cmdline;
     int ret;
+    TestArgs args = { .num_cpus = 1 };
 
     g_test_init(&argc, &argv, NULL);
 
-    data.num_cpus = 1;
-
-    cmdline = g_strdup_printf("-smp %d", data.num_cpus);
-    qtest_start(cmdline);
-    g_free(cmdline);
-
-    data.bus = qpci_init_pc();
-
-    g_test_add_data_func("/i440fx/defaults", &data, test_i440fx_defaults);
-    g_test_add_data_func("/i440fx/pam", &data, test_i440fx_pam);
+    g_test_add("/i440fx/defaults", TestData, &args, test_setup,
+               test_i440fx_defaults, test_teardown);
+    g_test_add("/i440fx/pam", TestData, &args, test_setup,
+               test_i440fx_pam, test_teardown);
 
     ret = g_test_run();
-
-    qtest_end();
-
     return ret;
 }
-- 
1.8.3.1

-- 
Eduardo