[PATCH-for-6.2 v3 0/6] tests/unit: Fix test-smp-parse

[PATCH-for-6.2 v3 0/6] tests/unit: Fix test-smp-parse

[Philippe Mathieu-Daudé]

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().

I suggest to rename smp_parse() -> machine_parse_smp_config()
after the rc0 more as a documentation change rather than an
API change, since this method got added last week and doesn't
follow the rest of the machine API.

Supersedes: <20211111024429.10568-1-wangyanan55@huawei.com>

Philippe Mathieu-Daudé (6):
  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: 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 | 123 +++++++++++++++++++-----------------
 4 files changed, 72 insertions(+), 62 deletions(-)

-- 
2.31.1

[PATCH-for-6.2 v3 1/6] 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.

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

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index cbe0c990494..bd11fbe91de 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);
 }
 
-- 
2.31.1

[PATCH-for-6.2 v3 2/6] 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.

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

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index bd11fbe91de..51670297bf9 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_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);
     mc->smp_props.dies_supported = true;
 
     for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
@@ -582,6 +572,15 @@ static void test_with_dies(void)
     object_unref(obj);
 }
 
+/* Type info of the tested machine */
+static const TypeInfo smp_machine_info = {
+    .name = TYPE_MACHINE,
+    .parent = TYPE_OBJECT,
+    .class_init = machine_class_init,
+    .class_size = sizeof(MachineClass),
+    .instance_size = sizeof(MachineState),
+};
+
 int main(int argc, char *argv[])
 {
     g_test_init(&argc, &argv, NULL);
-- 
2.31.1

[PATCH-for-6.2 v3 3/6] 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.

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 51670297bf9..de6d226b455 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_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-6.2 v3 4/6] 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.

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

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index de6d226b455..83a5b8ffdcf 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -492,19 +492,19 @@ static void test_generic(void)
     Object *obj = object_new(TYPE_MACHINE);
     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);
     }
 
     /* Force invalid min CPUs and max CPUs */
@@ -512,10 +512,10 @@ static void test_generic(void)
     mc->max_cpus = 511;
 
     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);
     }
 
     /* Reset the supported min CPUs and max CPUs */
@@ -530,47 +530,47 @@ static void test_with_dies(void)
     Object *obj = object_new(TYPE_MACHINE);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
-    SMPTestData *data = &(SMPTestData){{ }};
+    SMPTestData data = {};
     unsigned int num_dies = 2;
     int i;
 
     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);
+        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-6.2 v3 5/6] 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.

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 83a5b8ffdcf..11109752799 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-6.2 v3 6/6] 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.

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 11109752799..b158ebb16b1 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-for-6.2 v3 3/6] tests/unit/test-smp-parse: Explicit MachineClass name

[Andrew Jones]

On Thu, Nov 11, 2021 at 11:03:48AM +0100, Philippe Mathieu-Daudé wrote:
> 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.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Fixes: 9e8e393bb7 ("tests/unit: Add an unit test for smp parsing")
Reported-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Suggested-by: Yanan Wang <wangyanan55@huawei.com>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH-for-6.2 v3 0/6] tests/unit: Fix test-smp-parse

[Andrew Jones]

On Thu, Nov 11, 2021 at 11:03:45AM +0100, Philippe Mathieu-Daudé wrote:
> 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().
> 
> I suggest to rename smp_parse() -> machine_parse_smp_config()
> after the rc0 more as a documentation change rather than an
> API change, since this method got added last week and doesn't
> follow the rest of the machine API.
> 
> Supersedes: <20211111024429.10568-1-wangyanan55@huawei.com>
> 
> Philippe Mathieu-Daudé (6):
>   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: 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 | 123 +++++++++++++++++++-----------------
>  4 files changed, 72 insertions(+), 62 deletions(-)
> 
> -- 
> 2.31.1
> 
>

For the series

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH-for-6.2 v3 1/6] tests/unit/test-smp-parse: Restore MachineClass fields after modifying

[Richard Henderson]

On 11/11/21 11:03 AM, Philippe Mathieu-Daudé wrote:
> 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.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   tests/unit/test-smp-parse.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)

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

r~

Re: [PATCH-for-6.2 v3 2/6] tests/unit/test-smp-parse: QOM'ify smp_machine_class_init()

[Richard Henderson]

On 11/11/21 11:03 AM, Philippe Mathieu-Daudé wrote:
> 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.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   tests/unit/test-smp-parse.c | 25 ++++++++++++-------------
>   1 file changed, 12 insertions(+), 13 deletions(-)

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

r~

Re: [PATCH-for-6.2 v3 3/6] tests/unit/test-smp-parse: Explicit MachineClass name

[Richard Henderson]

On 11/11/21 11:03 AM, Philippe Mathieu-Daudé wrote:
> 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.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   tests/unit/test-smp-parse.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)

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

r~

Re: [PATCH-for-6.2 v3 4/6] tests/unit/test-smp-parse: Simplify pointer to compound literal use

[Richard Henderson]

On 11/11/21 11:03 AM, Philippe Mathieu-Daudé wrote:
> We can simply use a local variable (and pass its pointer) instead
> of a pointer to a compound literal.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   tests/unit/test-smp-parse.c | 64 ++++++++++++++++++-------------------
>   1 file changed, 32 insertions(+), 32 deletions(-)

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

r~

Re: [PATCH-for-6.2 v3 5/6] tests/unit/test-smp-parse: Constify some pointer/struct

[Richard Henderson]

On 11/11/21 11:03 AM, Philippe Mathieu-Daudé wrote:
> Declare structures const when we don't need to modify
> them at runtime.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   tests/unit/test-smp-parse.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)

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

r~

Re: [PATCH-for-6.2 v3 6/6] hw/core: Rename smp_parse() -> machine_parse_smp_config()

[Richard Henderson]

On 11/11/21 11:03 AM, Philippe Mathieu-Daudé wrote:
> 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.
> 
> 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(-)

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

r~

Re: [PATCH-for-6.2 v3 1/6] tests/unit/test-smp-parse: Restore MachineClass fields after modifying

[wangyanan (Y)]

On 2021/11/11 18:03, Philippe Mathieu-Daudé wrote:
> 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.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   tests/unit/test-smp-parse.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
> index cbe0c990494..bd11fbe91de 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);
>   }
>   
Just want to have a note:
Besides the supported min/max CPUs, mc->smp_props is dirtied
too for test purpose in each sub-test function. But for now, it is
not functionally necessary to also restore them at the final of each
sub-test function. We need to do this when new specific parameters
are tested in separate tests. At that time, for example, we will need
to at least add:

/* Restore the SMP compat properties */
mc->smp_props.dies_supported = false;

at the bottom of test_with_dies()

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

Thanks,
Yanan

Re: [PATCH-for-6.2 v3 2/6] tests/unit/test-smp-parse: QOM'ify smp_machine_class_init()

[wangyanan (Y)]

On 2021/11/11 18:03, Philippe Mathieu-Daudé wrote:
> 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.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   tests/unit/test-smp-parse.c | 25 ++++++++++++-------------
>   1 file changed, 12 insertions(+), 13 deletions(-)
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
Tested-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 bd11fbe91de..51670297bf9 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_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);
>       mc->smp_props.dies_supported = true;
>   
>       for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
> @@ -582,6 +572,15 @@ static void test_with_dies(void)
>       object_unref(obj);
>   }
>   
> +/* Type info of the tested machine */
> +static const TypeInfo smp_machine_info = {
> +    .name = TYPE_MACHINE,
> +    .parent = TYPE_OBJECT,
> +    .class_init = machine_class_init,
> +    .class_size = sizeof(MachineClass),
> +    .instance_size = sizeof(MachineState),
> +};
> +
>   int main(int argc, char *argv[])
>   {
>       g_test_init(&argc, &argv, NULL);

Re: [PATCH-for-6.2 v3 3/6] tests/unit/test-smp-parse: Explicit MachineClass name

[wangyanan (Y)]

On 2021/11/11 18:03, Philippe Mathieu-Daudé wrote:
> 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.
>
> 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 51670297bf9..de6d226b455 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_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);
I'm not very familiar with Qom code, so it may be a stupid question.
The mc->name will be automatically freed elsewhere when all the
testing is finished and exits, right? :)
>   }
>   
>   static void test_generic(void)
With my uncertainty clarified:
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
Tested-by: Yanan Wang <wangyanan55@huawei.com>

Thanks,
Yanan

Re: [PATCH-for-6.2 v3 4/6] tests/unit/test-smp-parse: Simplify pointer to compound literal use

[wangyanan (Y)]

On 2021/11/11 18:03, Philippe Mathieu-Daudé wrote:
> We can simply use a local variable (and pass its pointer) instead
> of a pointer to a compound literal.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   tests/unit/test-smp-parse.c | 64 ++++++++++++++++++-------------------
>   1 file changed, 32 insertions(+), 32 deletions(-)
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
Tested-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 de6d226b455..83a5b8ffdcf 100644
> --- a/tests/unit/test-smp-parse.c
> +++ b/tests/unit/test-smp-parse.c
> @@ -492,19 +492,19 @@ static void test_generic(void)
>       Object *obj = object_new(TYPE_MACHINE);
>       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);
>       }
>   
>       /* Force invalid min CPUs and max CPUs */
> @@ -512,10 +512,10 @@ static void test_generic(void)
>       mc->max_cpus = 511;
>   
>       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);
>       }
>   
>       /* Reset the supported min CPUs and max CPUs */
> @@ -530,47 +530,47 @@ static void test_with_dies(void)
>       Object *obj = object_new(TYPE_MACHINE);
>       MachineState *ms = MACHINE(obj);
>       MachineClass *mc = MACHINE_GET_CLASS(obj);
> -    SMPTestData *data = &(SMPTestData){{ }};
> +    SMPTestData data = {};
>       unsigned int num_dies = 2;
>       int i;
>   
>       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);
> +        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);

Re: [PATCH-for-6.2 v3 5/6] tests/unit/test-smp-parse: Constify some pointer/struct

[wangyanan (Y)]

On 2021/11/11 18:03, Philippe Mathieu-Daudé wrote:
> Declare structures const when we don't need to modify
> them at runtime.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   tests/unit/test-smp-parse.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
Tested-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 83a5b8ffdcf..11109752799 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;

Re: [PATCH-for-6.2 v3 6/6] hw/core: Rename smp_parse() -> machine_parse_smp_config()

[wangyanan (Y)]

On 2021/11/11 18:03, Philippe Mathieu-Daudé wrote:
> 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.
>
> 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(-)
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
Tested-by: Yanan Wang <wangyanan55@huawei.com>

Thanks,
Yanan
> 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 11109752799..b158ebb16b1 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);
>   

Re: [PATCH-for-6.2 v3 3/6] tests/unit/test-smp-parse: Explicit MachineClass name

[Philippe Mathieu-Daudé]

On 11/12/21 03:28, wangyanan (Y) wrote:
> On 2021/11/11 18:03, Philippe Mathieu-Daudé wrote:
>> 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.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   tests/unit/test-smp-parse.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)

>>   @@ -481,6 +483,8 @@ static void machine_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);
> I'm not very familiar with Qom code, so it may be a stupid question.
> The mc->name will be automatically freed elsewhere when all the
> testing is finished and exits, right? :)

I'll defer that to Eduardo / Markus, but meanwhile my understanding
is QOM classes are loaded once (the first time an instance requires
it) and never unloaded. Only instances can be unloaded, their resources
being released.
The machine life time is tied to the process one, when we are done
using a machine, it is simpler to exit() the process -- the kernel
releases the resources for us -- and create another process for a new
machine, rather than re-creating a different machine within the same
process.
If there is a need for it (like releasing class resources), it is doable
but it seems we never worried about it.

Re: [PATCH-for-6.2 v3 1/6] tests/unit/test-smp-parse: Restore MachineClass fields after modifying

[Philippe Mathieu-Daudé]

On 11/12/21 03:04, wangyanan (Y) wrote:
> On 2021/11/11 18:03, Philippe Mathieu-Daudé wrote:
>> 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.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   tests/unit/test-smp-parse.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
>> index cbe0c990494..bd11fbe91de 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);
>>   }
>>   
> Just want to have a note:
> Besides the supported min/max CPUs, mc->smp_props is dirtied
> too for test purpose in each sub-test function. But for now, it is
> not functionally necessary to also restore them at the final of each
> sub-test function. We need to do this when new specific parameters
> are tested in separate tests.

What we ought do is have an abstract TestMachineClass and have
a specific TestCaseMachineClass for each of your test cases.
This way we don't need to modify the class internal state at
runtime. I chose to not do it now because this is a more invasive
change past hard-freeze, and I just want to fix the Cirrus-CI
jobs here.

> At that time, for example, we will need
> to at least add:
> 
> /* Restore the SMP compat properties */
> mc->smp_props.dies_supported = false;
> 
> at the bottom of test_with_dies()

OK, I'll add that.

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

Re: [PATCH-for-6.2 v3 1/6] tests/unit/test-smp-parse: Restore MachineClass fields after modifying

[wangyanan (Y)]

On 2021/11/15 18:24, Philippe Mathieu-Daudé wrote:
> On 11/12/21 03:04, wangyanan (Y) wrote:
>> On 2021/11/11 18:03, Philippe Mathieu-Daudé wrote:
>>> 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.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>    tests/unit/test-smp-parse.c | 6 +++++-
>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
>>> index cbe0c990494..bd11fbe91de 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);
>>>    }
>>>    
>> Just want to have a note:
>> Besides the supported min/max CPUs, mc->smp_props is dirtied
>> too for test purpose in each sub-test function. But for now, it is
>> not functionally necessary to also restore them at the final of each
>> sub-test function. We need to do this when new specific parameters
>> are tested in separate tests.
> What we ought do is have an abstract TestMachineClass and have
> a specific TestCaseMachineClass for each of your test cases.
> This way we don't need to modify the class internal state at
> runtime. I chose to not do it now because this is a more invasive
> change past hard-freeze,
Yes, we can do that as an optimization for 7.0. I also have noticed
those for-7.0 patches submitted. I will have a look.
> and I just want to fix the Cirrus-CI
> jobs here.
And keep the fix for 6.2.
>> At that time, for example, we will need
>> to at least add:
>>
>> /* Restore the SMP compat properties */
>> mc->smp_props.dies_supported = false;
>>
>> at the bottom of test_with_dies()
> OK, I'll add that.

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

Re: [PATCH-for-6.2 v3 3/6] tests/unit/test-smp-parse: Explicit MachineClass name

[wangyanan (Y)]

On 2021/11/15 18:16, Philippe Mathieu-Daudé wrote:
> On 11/12/21 03:28, wangyanan (Y) wrote:
>> On 2021/11/11 18:03, Philippe Mathieu-Daudé wrote:
>>> 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.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>    tests/unit/test-smp-parse.c | 8 ++++++--
>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>    @@ -481,6 +483,8 @@ static void machine_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);
>> I'm not very familiar with Qom code, so it may be a stupid question.
>> The mc->name will be automatically freed elsewhere when all the
>> testing is finished and exits, right? :)
> I'll defer that to Eduardo / Markus, but meanwhile my understanding
> is QOM classes are loaded once (the first time an instance requires
> it) and never unloaded. Only instances can be unloaded, their resources
> being released.
Yes, this is also how I found about "classes" when reading the code and
playing with tests in test-smp-parse.c
> The machine life time is tied to the process one, when we are done
> using a machine, it is simpler to exit() the process -- the kernel
> releases the resources for us -- and create another process for a new
> machine, rather than re-creating a different machine within the same
> process.
> If there is a need for it (like releasing class resources), it is doable
> but it seems we never worried about it.
Ok, I see. Thank you for the explanations. :)

Thanks,
Yanan
> .