[PATCH v4 00/11] tests/unit: Fix test-smp-parse

[PATCH v4 00/11] tests/unit: Fix test-smp-parse

[Philippe Mathieu-Daudé]

Missing review: patches #4 to #8 (new)

Yet another approach to fix test-smp-parse.  v2 from Yanan Wang:
https://lore.kernel.org/qemu-devel/20211111024429.10568-1-wangyanan55@huawei.com/

Here we use the QOM class_init() to avoid having to deal with
object_unref() and deinit().

Since v3:
- Restore 'dies_supported' field in test_with_dies (Yanan)
- Add R-b tags
- QOM-ify the TYPE_MACHINE classes

Philippe Mathieu-Daudé (11):
  tests/unit/test-smp-parse: Restore MachineClass fields after modifying
  tests/unit/test-smp-parse: QOM'ify smp_machine_class_init()
  tests/unit/test-smp-parse: Explicit MachineClass name
  tests/unit/test-smp-parse: Pass machine type as argument to tests
  tests/unit/test-smp-parse: Split the 'generic' test in valid / invalid
  tests/unit/test-smp-parse: Add 'smp-with-dies' machine type
  tests/unit/test-smp-parse: Add 'smp-without-dies-invalid' machine type
  tests/unit/test-smp-parse: Add 'smp-without-dies-valid' machine type
  tests/unit/test-smp-parse: Simplify pointer to compound literal use
  tests/unit/test-smp-parse: Constify some pointer/struct
  hw/core: Rename smp_parse() -> machine_parse_smp_config()

 include/hw/boards.h         |   3 +-
 hw/core/machine-smp.c       |   6 +-
 hw/core/machine.c           |   2 +-
 tests/unit/test-smp-parse.c | 221 +++++++++++++++++++++++-------------
 4 files changed, 148 insertions(+), 84 deletions(-)

-- 
2.31.1

[PATCH-for-6.2 v4 01/11] tests/unit/test-smp-parse: Restore MachineClass fields after modifying

[Philippe Mathieu-Daudé]

There is a single MachineClass object, registered with
type_register_static(&smp_machine_info). Since the same
object is used multiple times (an MachineState object
is instantiated in both test_generic and test_with_dies),
we should restore its internal state after modifying for
the test purpose.

Reviewed-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
Tested-by: Yanan Wang <wangyanan55@huawei.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/unit/test-smp-parse.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index cbe0c990494..47774c1ee2a 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -512,7 +512,7 @@ static void test_generic(void)
         smp_parse_test(ms, data, true);
     }
 
-    /* Reset the supported min CPUs and max CPUs */
+    /* Force invalid min CPUs and max CPUs */
     mc->min_cpus = 2;
     mc->max_cpus = 511;
 
@@ -523,6 +523,10 @@ static void test_generic(void)
         smp_parse_test(ms, data, false);
     }
 
+    /* Reset the supported min CPUs and max CPUs */
+    mc->min_cpus = MIN_CPUS;
+    mc->max_cpus = MAX_CPUS;
+
     object_unref(obj);
 }
 
@@ -536,6 +540,7 @@ static void test_with_dies(void)
     int i;
 
     smp_machine_class_init(mc);
+    /* Force the SMP compat properties */
     mc->smp_props.dies_supported = true;
 
     for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
@@ -575,6 +580,9 @@ static void test_with_dies(void)
         smp_parse_test(ms, data, false);
     }
 
+    /* Restore the SMP compat properties */
+    mc->smp_props.dies_supported = false;
+
     object_unref(obj);
 }
 
-- 
2.31.1

[PATCH-for-6.2 v4 02/11] tests/unit/test-smp-parse: QOM'ify smp_machine_class_init()

[Philippe Mathieu-Daudé]

smp_machine_class_init() is the actual TypeInfo::class_init().
Declare it as such in smp_machine_info, and avoid to call it
manually in each test. Move smp_machine_info definition just
before we register the type to avoid a forward declaration.

Reviewed-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
Tested-by: Yanan Wang <wangyanan55@huawei.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/unit/test-smp-parse.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 47774c1ee2a..3fff2af4e27 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -75,14 +75,6 @@ typedef struct SMPTestData {
     const char *expect_error;
 } SMPTestData;
 
-/* Type info of the tested machine */
-static const TypeInfo smp_machine_info = {
-    .name = TYPE_MACHINE,
-    .parent = TYPE_OBJECT,
-    .class_size = sizeof(MachineClass),
-    .instance_size = sizeof(MachineState),
-};
-
 /*
  * List all the possible valid sub-collections of the generic 5
  * topology parameters (i.e. cpus/maxcpus/sockets/cores/threads),
@@ -480,9 +472,10 @@ static void unsupported_params_init(MachineClass *mc, SMPTestData *data)
     }
 }
 
-/* Reset the related machine properties before each sub-test */
-static void smp_machine_class_init(MachineClass *mc)
+static void machine_base_class_init(ObjectClass *oc, void *data)
 {
+    MachineClass *mc = MACHINE_CLASS(oc);
+
     mc->min_cpus = MIN_CPUS;
     mc->max_cpus = MAX_CPUS;
 
@@ -498,8 +491,6 @@ static void test_generic(void)
     SMPTestData *data = &(SMPTestData){{ }};
     int i;
 
-    smp_machine_class_init(mc);
-
     for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
         *data = data_generic_valid[i];
         unsupported_params_init(mc, data);
@@ -539,7 +530,6 @@ static void test_with_dies(void)
     unsigned int num_dies = 2;
     int i;
 
-    smp_machine_class_init(mc);
     /* Force the SMP compat properties */
     mc->smp_props.dies_supported = true;
 
@@ -586,12 +576,24 @@ static void test_with_dies(void)
     object_unref(obj);
 }
 
+/* Type info of the tested machine */
+static const TypeInfo smp_machine_types[] = {
+    {
+        .name           = TYPE_MACHINE,
+        .parent         = TYPE_OBJECT,
+        .class_init     = machine_base_class_init,
+        .class_size     = sizeof(MachineClass),
+        .instance_size  = sizeof(MachineState),
+    }
+};
+
+DEFINE_TYPES(smp_machine_types)
+
 int main(int argc, char *argv[])
 {
-    g_test_init(&argc, &argv, NULL);
-
     module_call_init(MODULE_INIT_QOM);
-    type_register_static(&smp_machine_info);
+
+    g_test_init(&argc, &argv, NULL);
 
     g_test_add_func("/test-smp-parse/generic", test_generic);
     g_test_add_func("/test-smp-parse/with_dies", test_with_dies);
-- 
2.31.1

[PATCH-for-6.2 v4 03/11] tests/unit/test-smp-parse: Explicit MachineClass name

[Philippe Mathieu-Daudé]

If the MachineClass::name pointer is not explicitly set, it is NULL.
Per the C standard, passing a NULL pointer to printf "%s" format is
undefined. Some implementations display it as 'NULL', other as 'null'.
Since we are comparing the formatted output, we need a stable value.
The easiest is to explicit a machine name string.

Reviewed-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
Tested-by: Yanan Wang <wangyanan55@huawei.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/unit/test-smp-parse.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 3fff2af4e27..b02450e25a3 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -23,6 +23,8 @@
 #define MIN_CPUS 1   /* set the min CPUs supported by the machine as 1 */
 #define MAX_CPUS 512 /* set the max CPUs supported by the machine as 512 */
 
+#define SMP_MACHINE_NAME "TEST-SMP"
+
 /*
  * Used to define the generic 3-level CPU topology hierarchy
  *  -sockets/cores/threads
@@ -307,13 +309,13 @@ static struct SMPTestData data_generic_invalid[] = {
          * should tweak the supported min CPUs to 2 for testing */
         .config = SMP_CONFIG_GENERIC(T, 1, F, 0, F, 0, F, 0, F, 0),
         .expect_error = "Invalid SMP CPUs 1. The min CPUs supported "
-                        "by machine '(null)' is 2",
+                        "by machine '" SMP_MACHINE_NAME "' is 2",
     }, {
         /* config: -smp 512
          * should tweak the supported max CPUs to 511 for testing */
         .config = SMP_CONFIG_GENERIC(T, 512, F, 0, F, 0, F, 0, F, 0),
         .expect_error = "Invalid SMP CPUs 512. The max CPUs supported "
-                        "by machine '(null)' is 511",
+                        "by machine '" SMP_MACHINE_NAME "' is 511",
     },
 };
 
@@ -481,6 +483,8 @@ static void machine_base_class_init(ObjectClass *oc, void *data)
 
     mc->smp_props.prefer_sockets = true;
     mc->smp_props.dies_supported = false;
+
+    mc->name = g_strdup(SMP_MACHINE_NAME);
 }
 
 static void test_generic(void)
-- 
2.31.1

[PATCH-for-7.0 v4 04/11] tests/unit/test-smp-parse: Pass machine type as argument to tests

[Philippe Mathieu-Daudé]

Use g_test_add_data_func() instead of g_test_add_func() so we can
pass the machine type to the tests (we will soon have different
machine types).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/unit/test-smp-parse.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index b02450e25a3..37c6b4981db 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -487,9 +487,10 @@ static void machine_base_class_init(ObjectClass *oc, void *data)
     mc->name = g_strdup(SMP_MACHINE_NAME);
 }
 
-static void test_generic(void)
+static void test_generic(const void *opaque)
 {
-    Object *obj = object_new(TYPE_MACHINE);
+    const char *machine_type = opaque;
+    Object *obj = object_new(machine_type);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData *data = &(SMPTestData){{ }};
@@ -525,9 +526,10 @@ static void test_generic(void)
     object_unref(obj);
 }
 
-static void test_with_dies(void)
+static void test_with_dies(const void *opaque)
 {
-    Object *obj = object_new(TYPE_MACHINE);
+    const char *machine_type = opaque;
+    Object *obj = object_new(machine_type);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData *data = &(SMPTestData){{ }};
@@ -599,8 +601,12 @@ int main(int argc, char *argv[])
 
     g_test_init(&argc, &argv, NULL);
 
-    g_test_add_func("/test-smp-parse/generic", test_generic);
-    g_test_add_func("/test-smp-parse/with_dies", test_with_dies);
+    g_test_add_data_func("/test-smp-parse/generic",
+                         TYPE_MACHINE,
+                         test_generic);
+    g_test_add_data_func("/test-smp-parse/with_dies",
+                         TYPE_MACHINE,
+                         test_with_dies);
 
     g_test_run();
 
-- 
2.31.1

Re: [PATCH-for-7.0 v4 04/11] tests/unit/test-smp-parse: Pass machine type as argument to tests

[Richard Henderson]

On 11/15/21 3:58 PM, Philippe Mathieu-Daudé wrote:
> Use g_test_add_data_func() instead of g_test_add_func() so we can
> pass the machine type to the tests (we will soon have different
> machine types).
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   tests/unit/test-smp-parse.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH-for-7.0 v4 04/11] tests/unit/test-smp-parse: Pass machine type as argument to tests

[wangyanan (Y)]

On 2021/11/15 22:58, Philippe Mathieu-Daudé wrote:
> Use g_test_add_data_func() instead of g_test_add_func() so we can
> pass the machine type to the tests (we will soon have different
> machine types).
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   tests/unit/test-smp-parse.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>

Thanks,
Yanan
> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
> index b02450e25a3..37c6b4981db 100644
> --- a/tests/unit/test-smp-parse.c
> +++ b/tests/unit/test-smp-parse.c
> @@ -487,9 +487,10 @@ static void machine_base_class_init(ObjectClass *oc, void *data)
>       mc->name = g_strdup(SMP_MACHINE_NAME);
>   }
>   
> -static void test_generic(void)
> +static void test_generic(const void *opaque)
>   {
> -    Object *obj = object_new(TYPE_MACHINE);
> +    const char *machine_type = opaque;
> +    Object *obj = object_new(machine_type);
>       MachineState *ms = MACHINE(obj);
>       MachineClass *mc = MACHINE_GET_CLASS(obj);
>       SMPTestData *data = &(SMPTestData){{ }};
> @@ -525,9 +526,10 @@ static void test_generic(void)
>       object_unref(obj);
>   }
>   
> -static void test_with_dies(void)
> +static void test_with_dies(const void *opaque)
>   {
> -    Object *obj = object_new(TYPE_MACHINE);
> +    const char *machine_type = opaque;
> +    Object *obj = object_new(machine_type);
>       MachineState *ms = MACHINE(obj);
>       MachineClass *mc = MACHINE_GET_CLASS(obj);
>       SMPTestData *data = &(SMPTestData){{ }};
> @@ -599,8 +601,12 @@ int main(int argc, char *argv[])
>   
>       g_test_init(&argc, &argv, NULL);
>   
> -    g_test_add_func("/test-smp-parse/generic", test_generic);
> -    g_test_add_func("/test-smp-parse/with_dies", test_with_dies);
> +    g_test_add_data_func("/test-smp-parse/generic",
> +                         TYPE_MACHINE,
> +                         test_generic);
> +    g_test_add_data_func("/test-smp-parse/with_dies",
> +                         TYPE_MACHINE,
> +                         test_with_dies);
>   
>       g_test_run();
>   

[PATCH-for-7.0 v4 05/11] tests/unit/test-smp-parse: Split the 'generic' test in valid / invalid

[Philippe Mathieu-Daudé]

Split the 'generic' test in two tests: 'valid' and 'invalid'.
This will allow us to remove the hack which modifies the
MachineClass internal state.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/unit/test-smp-parse.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 37c6b4981db..e27aedad354 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -487,7 +487,7 @@ static void machine_base_class_init(ObjectClass *oc, void *data)
     mc->name = g_strdup(SMP_MACHINE_NAME);
 }
 
-static void test_generic(const void *opaque)
+static void test_generic_valid(const void *opaque)
 {
     const char *machine_type = opaque;
     Object *obj = object_new(machine_type);
@@ -508,6 +508,19 @@ static void test_generic(const void *opaque)
         smp_parse_test(ms, data, true);
     }
 
+    object_unref(obj);
+}
+
+static void test_generic_invalid(const void *opaque)
+{
+    const char *machine_type = opaque;
+    Object *obj = object_new(machine_type);
+    MachineState *ms = MACHINE(obj);
+    MachineClass *mc = MACHINE_GET_CLASS(obj);
+    SMPTestData *data = &(SMPTestData){};
+    int i;
+
+
     /* Force invalid min CPUs and max CPUs */
     mc->min_cpus = 2;
     mc->max_cpus = 511;
@@ -601,9 +614,12 @@ int main(int argc, char *argv[])
 
     g_test_init(&argc, &argv, NULL);
 
-    g_test_add_data_func("/test-smp-parse/generic",
+    g_test_add_data_func("/test-smp-parse/generic/valid",
                          TYPE_MACHINE,
-                         test_generic);
+                         test_generic_valid);
+    g_test_add_data_func("/test-smp-parse/generic/invalid",
+                         TYPE_MACHINE,
+                         test_generic_invalid);
     g_test_add_data_func("/test-smp-parse/with_dies",
                          TYPE_MACHINE,
                          test_with_dies);
-- 
2.31.1

Re: [PATCH-for-7.0 v4 05/11] tests/unit/test-smp-parse: Split the 'generic' test in valid / invalid

[Richard Henderson]

On 11/15/21 3:58 PM, Philippe Mathieu-Daudé wrote:
> Split the 'generic' test in two tests: 'valid' and 'invalid'.
> This will allow us to remove the hack which modifies the
> MachineClass internal state.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   tests/unit/test-smp-parse.c | 22 +++++++++++++++++++---
>   1 file changed, 19 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH-for-7.0 v4 05/11] tests/unit/test-smp-parse: Split the 'generic' test in valid / invalid

[wangyanan (Y)]

On 2021/11/15 22:58, Philippe Mathieu-Daudé wrote:
> Split the 'generic' test in two tests: 'valid' and 'invalid'.
> This will allow us to remove the hack which modifies the
> MachineClass internal state.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   tests/unit/test-smp-parse.c | 22 +++++++++++++++++++---
>   1 file changed, 19 insertions(+), 3 deletions(-)
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>

Thanks,
Yanan
> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
> index 37c6b4981db..e27aedad354 100644
> --- a/tests/unit/test-smp-parse.c
> +++ b/tests/unit/test-smp-parse.c
> @@ -487,7 +487,7 @@ static void machine_base_class_init(ObjectClass *oc, void *data)
>       mc->name = g_strdup(SMP_MACHINE_NAME);
>   }
>   
> -static void test_generic(const void *opaque)
> +static void test_generic_valid(const void *opaque)
>   {
>       const char *machine_type = opaque;
>       Object *obj = object_new(machine_type);
> @@ -508,6 +508,19 @@ static void test_generic(const void *opaque)
>           smp_parse_test(ms, data, true);
>       }
>   
> +    object_unref(obj);
> +}
> +
> +static void test_generic_invalid(const void *opaque)
> +{
> +    const char *machine_type = opaque;
> +    Object *obj = object_new(machine_type);
> +    MachineState *ms = MACHINE(obj);
> +    MachineClass *mc = MACHINE_GET_CLASS(obj);
> +    SMPTestData *data = &(SMPTestData){};
> +    int i;
> +
> +
>       /* Force invalid min CPUs and max CPUs */
>       mc->min_cpus = 2;
>       mc->max_cpus = 511;
> @@ -601,9 +614,12 @@ int main(int argc, char *argv[])
>   
>       g_test_init(&argc, &argv, NULL);
>   
> -    g_test_add_data_func("/test-smp-parse/generic",
> +    g_test_add_data_func("/test-smp-parse/generic/valid",
>                            TYPE_MACHINE,
> -                         test_generic);
> +                         test_generic_valid);
> +    g_test_add_data_func("/test-smp-parse/generic/invalid",
> +                         TYPE_MACHINE,
> +                         test_generic_invalid);
>       g_test_add_data_func("/test-smp-parse/with_dies",
>                            TYPE_MACHINE,
>                            test_with_dies);

Re: [PATCH-for-7.0 v4 05/11] tests/unit/test-smp-parse: Split the 'generic' test in valid / invalid

[wangyanan (Y)]

On 2021/11/15 22:58, Philippe Mathieu-Daudé wrote:
> Split the 'generic' test in two tests: 'valid' and 'invalid'.
> This will allow us to remove the hack which modifies the
> MachineClass internal state.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   tests/unit/test-smp-parse.c | 22 +++++++++++++++++++---
>   1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
> index 37c6b4981db..e27aedad354 100644
> --- a/tests/unit/test-smp-parse.c
> +++ b/tests/unit/test-smp-parse.c
> @@ -487,7 +487,7 @@ static void machine_base_class_init(ObjectClass *oc, void *data)
>       mc->name = g_strdup(SMP_MACHINE_NAME);
>   }
>   
> -static void test_generic(const void *opaque)
> +static void test_generic_valid(const void *opaque)
>   {
>       const char *machine_type = opaque;
>       Object *obj = object_new(machine_type);
> @@ -508,6 +508,19 @@ static void test_generic(const void *opaque)
>           smp_parse_test(ms, data, true);
>       }
>   
> +    object_unref(obj);
> +}
> +
> +static void test_generic_invalid(const void *opaque)
> +{
> +    const char *machine_type = opaque;
> +    Object *obj = object_new(machine_type);
> +    MachineState *ms = MACHINE(obj);
> +    MachineClass *mc = MACHINE_GET_CLASS(obj);
> +    SMPTestData *data = &(SMPTestData){};
> +    int i;
> +
> +
Ah, there is an extra empty line which should be deleted.

Thanks,
Yanan
>       /* Force invalid min CPUs and max CPUs */
>       mc->min_cpus = 2;
>       mc->max_cpus = 511;
> @@ -601,9 +614,12 @@ int main(int argc, char *argv[])
>   
>       g_test_init(&argc, &argv, NULL);
>   
> -    g_test_add_data_func("/test-smp-parse/generic",
> +    g_test_add_data_func("/test-smp-parse/generic/valid",
>                            TYPE_MACHINE,
> -                         test_generic);
> +                         test_generic_valid);
> +    g_test_add_data_func("/test-smp-parse/generic/invalid",
> +                         TYPE_MACHINE,
> +                         test_generic_invalid);
>       g_test_add_data_func("/test-smp-parse/with_dies",
>                            TYPE_MACHINE,
>                            test_with_dies);

Re: [PATCH-for-7.0 v4 05/11] tests/unit/test-smp-parse: Split the 'generic' test in valid / invalid

[Richard Henderson]

On 11/16/21 3:07 PM, wangyanan (Y) wrote:
>> +    int i;
>> +
>> +
> Ah, there is an extra empty line which should be deleted.

I noticed that too, but it gets deleted in patch 7, so I let it be.  ;-)


r~

Re: [PATCH-for-7.0 v4 05/11] tests/unit/test-smp-parse: Split the 'generic' test in valid / invalid

[Philippe Mathieu-Daudé]

On 11/16/21 15:22, Richard Henderson wrote:
> On 11/16/21 3:07 PM, wangyanan (Y) wrote:
>>> +    int i;
>>> +
>>> +
>> Ah, there is an extra empty line which should be deleted.
> 
> I noticed that too, but it gets deleted in patch 7, so I let it be.  ;-)

Oops sorry, I'll clean that once v7.0 is released.

[PATCH-for-7.0 v4 06/11] tests/unit/test-smp-parse: Add 'smp-with-dies' machine type

[Philippe Mathieu-Daudé]

Avoid modifying the MachineClass internals by adding the
'smp-with-dies' machine, which inherits from TYPE_MACHINE.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/unit/test-smp-parse.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index e27aedad354..ff61da06e3d 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -487,6 +487,16 @@ static void machine_base_class_init(ObjectClass *oc, void *data)
     mc->name = g_strdup(SMP_MACHINE_NAME);
 }
 
+static void machine_with_dies_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    mc->min_cpus = MIN_CPUS;
+    mc->max_cpus = MAX_CPUS;
+
+    mc->smp_props.dies_supported = true;
+}
+
 static void test_generic_valid(const void *opaque)
 {
     const char *machine_type = opaque;
@@ -549,9 +559,6 @@ static void test_with_dies(const void *opaque)
     unsigned int num_dies = 2;
     int i;
 
-    /* Force the SMP compat properties */
-    mc->smp_props.dies_supported = true;
-
     for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
         *data = data_generic_valid[i];
         unsupported_params_init(mc, data);
@@ -589,9 +596,6 @@ static void test_with_dies(const void *opaque)
         smp_parse_test(ms, data, false);
     }
 
-    /* Restore the SMP compat properties */
-    mc->smp_props.dies_supported = false;
-
     object_unref(obj);
 }
 
@@ -603,6 +607,10 @@ static const TypeInfo smp_machine_types[] = {
         .class_init     = machine_base_class_init,
         .class_size     = sizeof(MachineClass),
         .instance_size  = sizeof(MachineState),
+    }, {
+        .name           = MACHINE_TYPE_NAME("smp-with-dies"),
+        .parent         = TYPE_MACHINE,
+        .class_init     = machine_with_dies_class_init,
     }
 };
 
@@ -621,7 +629,7 @@ int main(int argc, char *argv[])
                          TYPE_MACHINE,
                          test_generic_invalid);
     g_test_add_data_func("/test-smp-parse/with_dies",
-                         TYPE_MACHINE,
+                         MACHINE_TYPE_NAME("smp-with-dies"),
                          test_with_dies);
 
     g_test_run();
-- 
2.31.1

Re: [PATCH-for-7.0 v4 06/11] tests/unit/test-smp-parse: Add 'smp-with-dies' machine type

[Richard Henderson]

On 11/15/21 3:58 PM, Philippe Mathieu-Daudé wrote:
> Avoid modifying the MachineClass internals by adding the
> 'smp-with-dies' machine, which inherits from TYPE_MACHINE.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   tests/unit/test-smp-parse.c | 22 +++++++++++++++-------
>   1 file changed, 15 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH-for-7.0 v4 06/11] tests/unit/test-smp-parse: Add 'smp-with-dies' machine type

[wangyanan (Y)]

On 2021/11/15 22:58, Philippe Mathieu-Daudé wrote:
> Avoid modifying the MachineClass internals by adding the
> 'smp-with-dies' machine, which inherits from TYPE_MACHINE.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   tests/unit/test-smp-parse.c | 22 +++++++++++++++-------
>   1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
> index e27aedad354..ff61da06e3d 100644
> --- a/tests/unit/test-smp-parse.c
> +++ b/tests/unit/test-smp-parse.c
> @@ -487,6 +487,16 @@ static void machine_base_class_init(ObjectClass *oc, void *data)
>       mc->name = g_strdup(SMP_MACHINE_NAME);
>   }
>   
> +static void machine_with_dies_class_init(ObjectClass *oc, void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
> +    mc->min_cpus = MIN_CPUS;
> +    mc->max_cpus = MAX_CPUS;
> +
> +    mc->smp_props.dies_supported = true;
> +}
> +
>   static void test_generic_valid(const void *opaque)
>   {
>       const char *machine_type = opaque;
> @@ -549,9 +559,6 @@ static void test_with_dies(const void *opaque)
>       unsigned int num_dies = 2;
>       int i;
>   
> -    /* Force the SMP compat properties */
> -    mc->smp_props.dies_supported = true;
> -
>       for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
>           *data = data_generic_valid[i];
>           unsupported_params_init(mc, data);
> @@ -589,9 +596,6 @@ static void test_with_dies(const void *opaque)
>           smp_parse_test(ms, data, false);
>       }
>   
> -    /* Restore the SMP compat properties */
> -    mc->smp_props.dies_supported = false;
> -
>       object_unref(obj);
>   }
>   
> @@ -603,6 +607,10 @@ static const TypeInfo smp_machine_types[] = {
>           .class_init     = machine_base_class_init,
>           .class_size     = sizeof(MachineClass),
>           .instance_size  = sizeof(MachineState),
> +    }, {
> +        .name           = MACHINE_TYPE_NAME("smp-with-dies"),
> +        .parent         = TYPE_MACHINE,
> +        .class_init     = machine_with_dies_class_init,
>       }
>   };
>   
> @@ -621,7 +629,7 @@ int main(int argc, char *argv[])
>                            TYPE_MACHINE,
>                            test_generic_invalid);
>       g_test_add_data_func("/test-smp-parse/with_dies",
> -                         TYPE_MACHINE,
> +                         MACHINE_TYPE_NAME("smp-with-dies"),
>                            test_with_dies);
>   
>       g_test_run();
I have tested this patch locally and all works as expected:

Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
Tested-by: Yanan Wang <wangyanan55@huawei.com>

Thanks,
Yanan

[PATCH-for-7.0 v4 07/11] tests/unit/test-smp-parse: Add 'smp-without-dies-invalid' machine type

[Philippe Mathieu-Daudé]

Avoid modifying the MachineClass internals by adding the
'smp-without-dies-invalid' machine, which inherits from TYPE_MACHINE.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/unit/test-smp-parse.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index ff61da06e3d..dfe7f1313b0 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -487,6 +487,17 @@ static void machine_base_class_init(ObjectClass *oc, void *data)
     mc->name = g_strdup(SMP_MACHINE_NAME);
 }
 
+static void machine_without_dies_invalid_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    /* Force invalid min CPUs and max CPUs */
+    mc->min_cpus = 2;
+    mc->max_cpus = 511;
+
+    mc->smp_props.dies_supported = false;
+}
+
 static void machine_with_dies_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -530,11 +541,6 @@ static void test_generic_invalid(const void *opaque)
     SMPTestData *data = &(SMPTestData){};
     int i;
 
-
-    /* Force invalid min CPUs and max CPUs */
-    mc->min_cpus = 2;
-    mc->max_cpus = 511;
-
     for (i = 0; i < ARRAY_SIZE(data_generic_invalid); i++) {
         *data = data_generic_invalid[i];
         unsupported_params_init(mc, data);
@@ -542,10 +548,6 @@ static void test_generic_invalid(const void *opaque)
         smp_parse_test(ms, data, false);
     }
 
-    /* Reset the supported min CPUs and max CPUs */
-    mc->min_cpus = MIN_CPUS;
-    mc->max_cpus = MAX_CPUS;
-
     object_unref(obj);
 }
 
@@ -607,6 +609,10 @@ static const TypeInfo smp_machine_types[] = {
         .class_init     = machine_base_class_init,
         .class_size     = sizeof(MachineClass),
         .instance_size  = sizeof(MachineState),
+    }, {
+        .name           = MACHINE_TYPE_NAME("smp-without-dies-invalid"),
+        .parent         = TYPE_MACHINE,
+        .class_init     = machine_without_dies_invalid_class_init,
     }, {
         .name           = MACHINE_TYPE_NAME("smp-with-dies"),
         .parent         = TYPE_MACHINE,
@@ -626,7 +632,7 @@ int main(int argc, char *argv[])
                          TYPE_MACHINE,
                          test_generic_valid);
     g_test_add_data_func("/test-smp-parse/generic/invalid",
-                         TYPE_MACHINE,
+                         MACHINE_TYPE_NAME("smp-without-dies-invalid"),
                          test_generic_invalid);
     g_test_add_data_func("/test-smp-parse/with_dies",
                          MACHINE_TYPE_NAME("smp-with-dies"),
-- 
2.31.1

Re: [PATCH-for-7.0 v4 07/11] tests/unit/test-smp-parse: Add 'smp-without-dies-invalid' machine type

[Richard Henderson]

On 11/15/21 3:58 PM, Philippe Mathieu-Daudé wrote:
> Avoid modifying the MachineClass internals by adding the
> 'smp-without-dies-invalid' machine, which inherits from TYPE_MACHINE.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   tests/unit/test-smp-parse.c | 26 ++++++++++++++++----------
>   1 file changed, 16 insertions(+), 10 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH-for-7.0 v4 08/11] tests/unit/test-smp-parse: Add 'smp-without-dies-valid' machine type

[Philippe Mathieu-Daudé]

Keep the common TYPE_MACHINE class initialization in
machine_base_class_init(), make it abstract, and move
the non-common code to a new class: "smp-without-dies-valid".

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/unit/test-smp-parse.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index dfe7f1313b0..90a249fe8c8 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -478,13 +478,19 @@ static void machine_base_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
 
+    mc->smp_props.prefer_sockets = true;
+
+    mc->name = g_strdup(SMP_MACHINE_NAME);
+}
+
+static void machine_without_dies_valid_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
     mc->min_cpus = MIN_CPUS;
     mc->max_cpus = MAX_CPUS;
 
-    mc->smp_props.prefer_sockets = true;
     mc->smp_props.dies_supported = false;
-
-    mc->name = g_strdup(SMP_MACHINE_NAME);
 }
 
 static void machine_without_dies_invalid_class_init(ObjectClass *oc, void *data)
@@ -606,9 +612,14 @@ static const TypeInfo smp_machine_types[] = {
     {
         .name           = TYPE_MACHINE,
         .parent         = TYPE_OBJECT,
+        .abstract       = true,
         .class_init     = machine_base_class_init,
         .class_size     = sizeof(MachineClass),
         .instance_size  = sizeof(MachineState),
+    }, {
+        .name           = MACHINE_TYPE_NAME("smp-without-dies-valid"),
+        .parent         = TYPE_MACHINE,
+        .class_init     = machine_without_dies_valid_class_init,
     }, {
         .name           = MACHINE_TYPE_NAME("smp-without-dies-invalid"),
         .parent         = TYPE_MACHINE,
@@ -629,7 +640,7 @@ int main(int argc, char *argv[])
     g_test_init(&argc, &argv, NULL);
 
     g_test_add_data_func("/test-smp-parse/generic/valid",
-                         TYPE_MACHINE,
+                         MACHINE_TYPE_NAME("smp-without-dies-valid"),
                          test_generic_valid);
     g_test_add_data_func("/test-smp-parse/generic/invalid",
                          MACHINE_TYPE_NAME("smp-without-dies-invalid"),
-- 
2.31.1

Re: [PATCH-for-7.0 v4 08/11] tests/unit/test-smp-parse: Add 'smp-without-dies-valid' machine type

[Richard Henderson]

On 11/15/21 3:58 PM, Philippe Mathieu-Daudé wrote:
> Keep the common TYPE_MACHINE class initialization in
> machine_base_class_init(), make it abstract, and move
> the non-common code to a new class: "smp-without-dies-valid".
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   tests/unit/test-smp-parse.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH-for-7.0 v4 08/11] tests/unit/test-smp-parse: Add 'smp-without-dies-valid' machine type

[wangyanan (Y)]

Hi Philippe,

On 2021/11/15 22:58, Philippe Mathieu-Daudé wrote:
> Keep the common TYPE_MACHINE class initialization in
> machine_base_class_init(), make it abstract, and move
> the non-common code to a new class: "smp-without-dies-valid".
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   tests/unit/test-smp-parse.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
> index dfe7f1313b0..90a249fe8c8 100644
> --- a/tests/unit/test-smp-parse.c
> +++ b/tests/unit/test-smp-parse.c
> @@ -478,13 +478,19 @@ static void machine_base_class_init(ObjectClass *oc, void *data)
>   {
>       MachineClass *mc = MACHINE_CLASS(oc);
>   
> +    mc->smp_props.prefer_sockets = true;
> +
> +    mc->name = g_strdup(SMP_MACHINE_NAME);
> +}
> +
> +static void machine_without_dies_valid_class_init(ObjectClass *oc, void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
>       mc->min_cpus = MIN_CPUS;
>       mc->max_cpus = MAX_CPUS;
>   
> -    mc->smp_props.prefer_sockets = true;
>       mc->smp_props.dies_supported = false;
> -
> -    mc->name = g_strdup(SMP_MACHINE_NAME);
>   }
>   
>   static void machine_without_dies_invalid_class_init(ObjectClass *oc, void *data)
> @@ -606,9 +612,14 @@ static const TypeInfo smp_machine_types[] = {
>       {
>           .name           = TYPE_MACHINE,
>           .parent         = TYPE_OBJECT,
> +        .abstract       = true,
>           .class_init     = machine_base_class_init,
>           .class_size     = sizeof(MachineClass),
>           .instance_size  = sizeof(MachineState),
> +    }, {
> +        .name           = MACHINE_TYPE_NAME("smp-without-dies-valid"),
> +        .parent         = TYPE_MACHINE,
> +        .class_init     = machine_without_dies_valid_class_init,
>       }, {
>           .name           = MACHINE_TYPE_NAME("smp-without-dies-invalid"),
>           .parent         = TYPE_MACHINE,
> @@ -629,7 +640,7 @@ int main(int argc, char *argv[])
>       g_test_init(&argc, &argv, NULL);
>   
>       g_test_add_data_func("/test-smp-parse/generic/valid",
> -                         TYPE_MACHINE,
> +                         MACHINE_TYPE_NAME("smp-without-dies-valid"),
>                            test_generic_valid);
>       g_test_add_data_func("/test-smp-parse/generic/invalid",
>                            MACHINE_TYPE_NAME("smp-without-dies-invalid"),
After patch #7 and #8, we will have sub-tests as below. It looks nice, 
but it will
probably be better to tweak "smp-without-dies-valid" to "smp-generic-valid",
and "smp-without-dies-invalid" to "smp-generic-invalid", which will be more
consistent with the corresponding sub-test name.

It's Ok now as we only have dies currently besides generic 
sockets/cores/threads,
but "smp-without-dies-xxx" will become a bit confusing when new topology
members are introduced and tested here.

int main(int argc, char *argv[])
{
module_call_init(MODULE_INIT_QOM);

     g_test_init(&argc, &argv, NULL);

g_test_add_data_func("/test-smp-parse/generic/valid",
MACHINE_TYPE_NAME("smp-without-dies-valid"),
test_generic_valid);
g_test_add_data_func("/test-smp-parse/generic/invalid",
MACHINE_TYPE_NAME("smp-without-dies-invalid"),
test_generic_invalid);
g_test_add_data_func("/test-smp-parse/with_dies",
MACHINE_TYPE_NAME("smp-with-dies"),
test_with_dies);

g_test_run();

     return 0;
}

Otherwise, #7 and #8 look nice. Thanks for your work!

Thanks,
Yanan

Re: [PATCH-for-7.0 v4 08/11] tests/unit/test-smp-parse: Add 'smp-without-dies-valid' machine type

[Philippe Mathieu-Daudé]

Hi Yanan,

On 11/17/21 08:37, wangyanan (Y) wrote:
> On 2021/11/15 22:58, Philippe Mathieu-Daudé wrote:
>> Keep the common TYPE_MACHINE class initialization in
>> machine_base_class_init(), make it abstract, and move
>> the non-common code to a new class: "smp-without-dies-valid".
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   tests/unit/test-smp-parse.c | 19 +++++++++++++++----
>>   1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
>> index dfe7f1313b0..90a249fe8c8 100644
>> --- a/tests/unit/test-smp-parse.c
>> +++ b/tests/unit/test-smp-parse.c
>> @@ -478,13 +478,19 @@ static void machine_base_class_init(ObjectClass
>> *oc, void *data)
>>   {
>>       MachineClass *mc = MACHINE_CLASS(oc);
>>   +    mc->smp_props.prefer_sockets = true;
>> +
>> +    mc->name = g_strdup(SMP_MACHINE_NAME);
>> +}
>> +
>> +static void machine_without_dies_valid_class_init(ObjectClass *oc,
>> void *data)
>> +{
>> +    MachineClass *mc = MACHINE_CLASS(oc);
>> +
>>       mc->min_cpus = MIN_CPUS;
>>       mc->max_cpus = MAX_CPUS;
>>   -    mc->smp_props.prefer_sockets = true;
>>       mc->smp_props.dies_supported = false;
>> -
>> -    mc->name = g_strdup(SMP_MACHINE_NAME);
>>   }
>>     static void machine_without_dies_invalid_class_init(ObjectClass
>> *oc, void *data)
>> @@ -606,9 +612,14 @@ static const TypeInfo smp_machine_types[] = {
>>       {
>>           .name           = TYPE_MACHINE,
>>           .parent         = TYPE_OBJECT,
>> +        .abstract       = true,
>>           .class_init     = machine_base_class_init,
>>           .class_size     = sizeof(MachineClass),
>>           .instance_size  = sizeof(MachineState),
>> +    }, {
>> +        .name           = MACHINE_TYPE_NAME("smp-without-dies-valid"),
>> +        .parent         = TYPE_MACHINE,
>> +        .class_init     = machine_without_dies_valid_class_init,
>>       }, {
>>           .name           =
>> MACHINE_TYPE_NAME("smp-without-dies-invalid"),
>>           .parent         = TYPE_MACHINE,
>> @@ -629,7 +640,7 @@ int main(int argc, char *argv[])
>>       g_test_init(&argc, &argv, NULL);
>>         g_test_add_data_func("/test-smp-parse/generic/valid",
>> -                         TYPE_MACHINE,
>> +                         MACHINE_TYPE_NAME("smp-without-dies-valid"),
>>                            test_generic_valid);
>>       g_test_add_data_func("/test-smp-parse/generic/invalid",
>>                            MACHINE_TYPE_NAME("smp-without-dies-invalid"),
> After patch #7 and #8, we will have sub-tests as below. It looks nice,
> but it will
> probably be better to tweak "smp-without-dies-valid" to
> "smp-generic-valid",
> and "smp-without-dies-invalid" to "smp-generic-invalid", which will be more
> consistent with the corresponding sub-test name.
> 
> It's Ok now as we only have dies currently besides generic
> sockets/cores/threads,
> but "smp-without-dies-xxx" will become a bit confusing when new topology
> members are introduced and tested here.

OK I modified it and will respin once v6.2 is released.

Also test_with_dies() should be split in 2 tests: valid/invalid;
then smp_parse_test() should split tests further regarding the
socket preference. But I'll let that to you, I wanted to 1/ fix
our Windows CI and 2/ show you how to structure the tests.

Regards,

Phil.

Re: [PATCH-for-7.0 v4 08/11] tests/unit/test-smp-parse: Add 'smp-without-dies-valid' machine type

[wangyanan (Y)]

On 2021/11/17 16:08, Philippe Mathieu-Daudé wrote:
> Hi Yanan,
>
> On 11/17/21 08:37, wangyanan (Y) wrote:
>> On 2021/11/15 22:58, Philippe Mathieu-Daudé wrote:
>>> Keep the common TYPE_MACHINE class initialization in
>>> machine_base_class_init(), make it abstract, and move
>>> the non-common code to a new class: "smp-without-dies-valid".
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>    tests/unit/test-smp-parse.c | 19 +++++++++++++++----
>>>    1 file changed, 15 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
>>> index dfe7f1313b0..90a249fe8c8 100644
>>> --- a/tests/unit/test-smp-parse.c
>>> +++ b/tests/unit/test-smp-parse.c
>>> @@ -478,13 +478,19 @@ static void machine_base_class_init(ObjectClass
>>> *oc, void *data)
>>>    {
>>>        MachineClass *mc = MACHINE_CLASS(oc);
>>>    +    mc->smp_props.prefer_sockets = true;
>>> +
>>> +    mc->name = g_strdup(SMP_MACHINE_NAME);
>>> +}
>>> +
>>> +static void machine_without_dies_valid_class_init(ObjectClass *oc,
>>> void *data)
>>> +{
>>> +    MachineClass *mc = MACHINE_CLASS(oc);
>>> +
>>>        mc->min_cpus = MIN_CPUS;
>>>        mc->max_cpus = MAX_CPUS;
>>>    -    mc->smp_props.prefer_sockets = true;
>>>        mc->smp_props.dies_supported = false;
>>> -
>>> -    mc->name = g_strdup(SMP_MACHINE_NAME);
>>>    }
>>>      static void machine_without_dies_invalid_class_init(ObjectClass
>>> *oc, void *data)
>>> @@ -606,9 +612,14 @@ static const TypeInfo smp_machine_types[] = {
>>>        {
>>>            .name           = TYPE_MACHINE,
>>>            .parent         = TYPE_OBJECT,
>>> +        .abstract       = true,
>>>            .class_init     = machine_base_class_init,
>>>            .class_size     = sizeof(MachineClass),
>>>            .instance_size  = sizeof(MachineState),
>>> +    }, {
>>> +        .name           = MACHINE_TYPE_NAME("smp-without-dies-valid"),
>>> +        .parent         = TYPE_MACHINE,
>>> +        .class_init     = machine_without_dies_valid_class_init,
>>>        }, {
>>>            .name           =
>>> MACHINE_TYPE_NAME("smp-without-dies-invalid"),
>>>            .parent         = TYPE_MACHINE,
>>> @@ -629,7 +640,7 @@ int main(int argc, char *argv[])
>>>        g_test_init(&argc, &argv, NULL);
>>>          g_test_add_data_func("/test-smp-parse/generic/valid",
>>> -                         TYPE_MACHINE,
>>> +                         MACHINE_TYPE_NAME("smp-without-dies-valid"),
>>>                             test_generic_valid);
>>>        g_test_add_data_func("/test-smp-parse/generic/invalid",
>>>                             MACHINE_TYPE_NAME("smp-without-dies-invalid"),
>> After patch #7 and #8, we will have sub-tests as below. It looks nice,
>> but it will
>> probably be better to tweak "smp-without-dies-valid" to
>> "smp-generic-valid",
>> and "smp-without-dies-invalid" to "smp-generic-invalid", which will be more
>> consistent with the corresponding sub-test name.
>>
>> It's Ok now as we only have dies currently besides generic
>> sockets/cores/threads,
>> but "smp-without-dies-xxx" will become a bit confusing when new topology
>> members are introduced and tested here.
> OK I modified it and will respin once v6.2 is released.
>
> Also test_with_dies() should be split in 2 tests: valid/invalid;
> then smp_parse_test() should split tests further regarding the
> socket preference. But I'll let that to you,
Sure, I can do this in an appropriate time. But IMHO, I don't see a
strong necessity to split test_with_dies for now, as the valid/invalid
scenarios share the same class properties. We can probably keep it
as is until we have to split it.

As for socket preference, I can foresee code duplication if we split
all the tests into two parts just regarding the socket preference.
Isn't it clear enough to use current smp_parse_test() for each
test data sample? Do we have other concern here?
> I wanted to 1/ fix
> our Windows CI and 2/ show you how to structure the tests.
Understood. The test architecture is indeed improved a lot.
Thanks for your education. 😉

Thanks,
Yanan
> Regards,
>
> Phil.
>
> .

[PATCH-for-7.0 v4 09/11] tests/unit/test-smp-parse: Simplify pointer to compound literal use

[Philippe Mathieu-Daudé]

We can simply use a local variable (and pass its pointer) instead
of a pointer to a compound literal.

Reviewed-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
Tested-by: Yanan Wang <wangyanan55@huawei.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/unit/test-smp-parse.c | 66 ++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 90a249fe8c8..2f3bcf198a5 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -520,19 +520,19 @@ static void test_generic_valid(const void *opaque)
     Object *obj = object_new(machine_type);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
-    SMPTestData *data = &(SMPTestData){{ }};
+    SMPTestData data = {};
     int i;
 
     for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
-        *data = data_generic_valid[i];
-        unsupported_params_init(mc, data);
+        data = data_generic_valid[i];
+        unsupported_params_init(mc, &data);
 
-        smp_parse_test(ms, data, true);
+        smp_parse_test(ms, &data, true);
 
         /* Unsupported parameters can be provided with their values as 1 */
-        data->config.has_dies = true;
-        data->config.dies = 1;
-        smp_parse_test(ms, data, true);
+        data.config.has_dies = true;
+        data.config.dies = 1;
+        smp_parse_test(ms, &data, true);
     }
 
     object_unref(obj);
@@ -544,14 +544,14 @@ static void test_generic_invalid(const void *opaque)
     Object *obj = object_new(machine_type);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
-    SMPTestData *data = &(SMPTestData){};
+    SMPTestData data = {};
     int i;
 
     for (i = 0; i < ARRAY_SIZE(data_generic_invalid); i++) {
-        *data = data_generic_invalid[i];
-        unsupported_params_init(mc, data);
+        data = data_generic_invalid[i];
+        unsupported_params_init(mc, &data);
 
-        smp_parse_test(ms, data, false);
+        smp_parse_test(ms, &data, false);
     }
 
     object_unref(obj);
@@ -563,45 +563,45 @@ static void test_with_dies(const void *opaque)
     Object *obj = object_new(machine_type);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
-    SMPTestData *data = &(SMPTestData){{ }};
+    SMPTestData data = {};
     unsigned int num_dies = 2;
     int i;
 
     for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
-        *data = data_generic_valid[i];
-        unsupported_params_init(mc, data);
+        data = data_generic_valid[i];
+        unsupported_params_init(mc, &data);
 
         /* when dies parameter is omitted, it will be set as 1 */
-        data->expect_prefer_sockets.dies = 1;
-        data->expect_prefer_cores.dies = 1;
+        data.expect_prefer_sockets.dies = 1;
+        data.expect_prefer_cores.dies = 1;
 
-        smp_parse_test(ms, data, true);
+        smp_parse_test(ms, &data, true);
 
         /* when dies parameter is specified */
-        data->config.has_dies = true;
-        data->config.dies = num_dies;
-        if (data->config.has_cpus) {
-            data->config.cpus *= num_dies;
+        data.config.has_dies = true;
+        data.config.dies = num_dies;
+        if (data.config.has_cpus) {
+            data.config.cpus *= num_dies;
         }
-        if (data->config.has_maxcpus) {
-            data->config.maxcpus *= num_dies;
+        if (data.config.has_maxcpus) {
+            data.config.maxcpus *= num_dies;
         }
 
-        data->expect_prefer_sockets.dies = num_dies;
-        data->expect_prefer_sockets.cpus *= num_dies;
-        data->expect_prefer_sockets.max_cpus *= num_dies;
-        data->expect_prefer_cores.dies = num_dies;
-        data->expect_prefer_cores.cpus *= num_dies;
-        data->expect_prefer_cores.max_cpus *= num_dies;
+        data.expect_prefer_sockets.dies = num_dies;
+        data.expect_prefer_sockets.cpus *= num_dies;
+        data.expect_prefer_sockets.max_cpus *= num_dies;
+        data.expect_prefer_cores.dies = num_dies;
+        data.expect_prefer_cores.cpus *= num_dies;
+        data.expect_prefer_cores.max_cpus *= num_dies;
 
-        smp_parse_test(ms, data, true);
+        smp_parse_test(ms, &data, true);
     }
 
     for (i = 0; i < ARRAY_SIZE(data_with_dies_invalid); i++) {
-        *data = data_with_dies_invalid[i];
-        unsupported_params_init(mc, data);
+        data = data_with_dies_invalid[i];
+        unsupported_params_init(mc, &data);
 
-        smp_parse_test(ms, data, false);
+        smp_parse_test(ms, &data, false);
     }
 
     object_unref(obj);
-- 
2.31.1

[PATCH-for-7.0 v4 10/11] tests/unit/test-smp-parse: Constify some pointer/struct

[Philippe Mathieu-Daudé]

Declare structures const when we don't need to modify
them at runtime.

Reviewed-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
Tested-by: Yanan Wang <wangyanan55@huawei.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/unit/test-smp-parse.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 2f3bcf198a5..8f47a2e65f6 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -83,7 +83,7 @@ typedef struct SMPTestData {
  * then test the automatic calculation algorithm of the missing
  * values in the parser.
  */
-static struct SMPTestData data_generic_valid[] = {
+static const struct SMPTestData data_generic_valid[] = {
     {
         /* config: no configuration provided
          * expect: cpus=1,sockets=1,cores=1,threads=1,maxcpus=1 */
@@ -285,7 +285,7 @@ static struct SMPTestData data_generic_valid[] = {
     },
 };
 
-static struct SMPTestData data_generic_invalid[] = {
+static const struct SMPTestData data_generic_invalid[] = {
     {
         /* config: -smp 2,dies=2 */
         .config = SMP_CONFIG_WITH_DIES(T, 2, F, 0, T, 2, F, 0, F, 0, F, 0),
@@ -319,7 +319,7 @@ static struct SMPTestData data_generic_invalid[] = {
     },
 };
 
-static struct SMPTestData data_with_dies_invalid[] = {
+static const struct SMPTestData data_with_dies_invalid[] = {
     {
         /* config: -smp 16,sockets=2,dies=2,cores=4,threads=2,maxcpus=16 */
         .config = SMP_CONFIG_WITH_DIES(T, 16, T, 2, T, 2, T, 4, T, 2, T, 16),
@@ -356,7 +356,7 @@ static char *smp_config_to_string(SMPConfiguration *config)
         config->has_maxcpus ? "true" : "false", config->maxcpus);
 }
 
-static char *cpu_topology_to_string(CpuTopology *topo)
+static char *cpu_topology_to_string(const CpuTopology *topo)
 {
     return g_strdup_printf(
         "(CpuTopology) {\n"
@@ -372,7 +372,7 @@ static char *cpu_topology_to_string(CpuTopology *topo)
 }
 
 static void check_parse(MachineState *ms, SMPConfiguration *config,
-                        CpuTopology *expect_topo, const char *expect_err,
+                        const CpuTopology *expect_topo, const char *expect_err,
                         bool is_valid)
 {
     g_autofree char *config_str = smp_config_to_string(config);
@@ -466,7 +466,7 @@ static void smp_parse_test(MachineState *ms, SMPTestData *data, bool is_valid)
 }
 
 /* The parsed results of the unsupported parameters should be 1 */
-static void unsupported_params_init(MachineClass *mc, SMPTestData *data)
+static void unsupported_params_init(const MachineClass *mc, SMPTestData *data)
 {
     if (!mc->smp_props.dies_supported) {
         data->expect_prefer_sockets.dies = 1;
-- 
2.31.1

[PATCH-for-7.0 v4 11/11] hw/core: Rename smp_parse() -> machine_parse_smp_config()

[Philippe Mathieu-Daudé]

All methods related to MachineState are prefixed with "machine_".
smp_parse() does not need to be an exception. Rename it and
const'ify the SMPConfiguration argument, since it doesn't need
to be modified.

Reviewed-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
Tested-by: Yanan Wang <wangyanan55@huawei.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/boards.h         | 3 ++-
 hw/core/machine-smp.c       | 6 ++++--
 hw/core/machine.c           | 2 +-
 tests/unit/test-smp-parse.c | 8 ++++----
 4 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 9c1c1901046..7597cec4400 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -34,7 +34,8 @@ HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine);
 void machine_set_cpu_numa_node(MachineState *machine,
                                const CpuInstanceProperties *props,
                                Error **errp);
-void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp);
+void machine_parse_smp_config(MachineState *ms,
+                              const SMPConfiguration *config, Error **errp);
 
 /**
  * machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices
diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index 116a0cbbfab..2cbfd574293 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -44,7 +44,8 @@ static char *cpu_hierarchy_to_string(MachineState *ms)
 }
 
 /*
- * smp_parse - Generic function used to parse the given SMP configuration
+ * machine_parse_smp_config: Generic function used to parse the given
+ *                           SMP configuration
  *
  * Any missing parameter in "cpus/maxcpus/sockets/cores/threads" will be
  * automatically computed based on the provided ones.
@@ -63,7 +64,8 @@ static char *cpu_hierarchy_to_string(MachineState *ms)
  * introduced topology members which are likely to be target specific should
  * be directly set as 1 if they are omitted (e.g. dies for PC since 4.1).
  */
-void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
+void machine_parse_smp_config(MachineState *ms,
+                              const SMPConfiguration *config, Error **errp)
 {
     MachineClass *mc = MACHINE_GET_CLASS(ms);
     unsigned cpus    = config->has_cpus ? config->cpus : 0;
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 26ec54e7261..a2d3c9969d9 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -760,7 +760,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name,
         return;
     }
 
-    smp_parse(ms, config, errp);
+    machine_parse_smp_config(ms, config, errp);
 }
 
 static void machine_class_init(ObjectClass *oc, void *data)
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 8f47a2e65f6..8e488e95145 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -337,7 +337,7 @@ static const struct SMPTestData data_with_dies_invalid[] = {
     },
 };
 
-static char *smp_config_to_string(SMPConfiguration *config)
+static char *smp_config_to_string(const SMPConfiguration *config)
 {
     return g_strdup_printf(
         "(SMPConfiguration) {\n"
@@ -371,7 +371,7 @@ static char *cpu_topology_to_string(const CpuTopology *topo)
         topo->cores, topo->threads, topo->max_cpus);
 }
 
-static void check_parse(MachineState *ms, SMPConfiguration *config,
+static void check_parse(MachineState *ms, const SMPConfiguration *config,
                         const CpuTopology *expect_topo, const char *expect_err,
                         bool is_valid)
 {
@@ -380,8 +380,8 @@ static void check_parse(MachineState *ms, SMPConfiguration *config,
     g_autofree char *output_topo_str = NULL;
     Error *err = NULL;
 
-    /* call the generic parser smp_parse() */
-    smp_parse(ms, config, &err);
+    /* call the generic parser */
+    machine_parse_smp_config(ms, config, &err);
 
     output_topo_str = cpu_topology_to_string(&ms->smp);
 
-- 
2.31.1

Re: [PATCH v4 00/11] tests/unit: Fix test-smp-parse

[Philippe Mathieu-Daudé]

On 11/15/21 15:58, Philippe Mathieu-Daudé wrote:
> Missing review: patches #4 to #8 (new)
> 
> Yet another approach to fix test-smp-parse.  v2 from Yanan Wang:
> https://lore.kernel.org/qemu-devel/20211111024429.10568-1-wangyanan55@huawei.com/
> 
> Here we use the QOM class_init() to avoid having to deal with
> object_unref() and deinit().
> 
> Since v3:
> - Restore 'dies_supported' field in test_with_dies (Yanan)
> - Add R-b tags
> - QOM-ify the TYPE_MACHINE classes
> 
> Philippe Mathieu-Daudé (11):
>   tests/unit/test-smp-parse: Restore MachineClass fields after modifying
>   tests/unit/test-smp-parse: QOM'ify smp_machine_class_init()
>   tests/unit/test-smp-parse: Explicit MachineClass name

Patches 1-3 queued.