[PATCH v2 0/2] tests/unit/test-smp-parse: Two fixes for test-smp-parse

[PATCH v2 0/2] tests/unit/test-smp-parse: Two fixes for test-smp-parse

[Yanan Wang]

Hi,

There are two fixes for tests/unit/test-smp-parse.c (v2).
The first one makes an unified name for the tested machine, which
will make the test more stable and resolve the CI failures in [1].
The second one fixes a check-patch complain for commit 9e8e393bb7.

[1] https://cirrus-ci.com/task/5823855357853696

History:
v1->v2:
- tweak the structure zero-initialization format from { {0} } to {}
  as Markus suggested (patch #2).

Thanks,
Yanan

Yanan Wang (2):
  tests/unit/test-smp-parse: Make an unified name for the tested machine
  tests/unit/test-smp-parse: Fix a check-patch complain

 tests/unit/test-smp-parse.c | 42 +++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 13 deletions(-)

-- 
2.19.1

[PATCH v2 1/2] tests/unit/test-smp-parse: Make an unified name for the tested machine

[Yanan Wang]

Currently, the name of the tested machine in the expected error
messages is hardcoded as "(null)" which is not good, because the
actual generated name of the machine maybe "(null)" or "(NULL)"
which will cause an unexpected test failure in some CI platforms.

So let's rename the tested machine with an unified string and
tweak the expected error messages accordingly.

Fixes: 9e8e393bb7 ("tests/unit: Add an unit test for smp parsing")
Reported-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 tests/unit/test-smp-parse.c | 38 ++++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index cbe0c99049..872512aa37 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
@@ -315,13 +317,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",
     },
 };
 
@@ -480,26 +482,41 @@ 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 Object *smp_test_machine_init(void)
 {
+    Object *obj = object_new(TYPE_MACHINE);
+    MachineClass *mc = MACHINE_GET_CLASS(obj);
+
+    g_free(mc->name);
+    mc->name = g_strdup(SMP_MACHINE_NAME);
+
     mc->min_cpus = MIN_CPUS;
     mc->max_cpus = MAX_CPUS;
 
     mc->smp_props.prefer_sockets = true;
     mc->smp_props.dies_supported = false;
+
+    return obj;
+}
+
+static void smp_test_machine_deinit(Object *obj)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(obj);
+
+    g_free(mc->name);
+    mc->name = NULL;
+
+    object_unref(obj);
 }
 
 static void test_generic(void)
 {
-    Object *obj = object_new(TYPE_MACHINE);
+    Object *obj = smp_test_machine_init();
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     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);
@@ -523,19 +540,18 @@ static void test_generic(void)
         smp_parse_test(ms, data, false);
     }
 
-    object_unref(obj);
+    smp_test_machine_deinit(obj);
 }
 
 static void test_with_dies(void)
 {
-    Object *obj = object_new(TYPE_MACHINE);
+    Object *obj = smp_test_machine_init();
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData *data = &(SMPTestData){{ }};
     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++) {
@@ -575,7 +591,7 @@ static void test_with_dies(void)
         smp_parse_test(ms, data, false);
     }
 
-    object_unref(obj);
+    smp_test_machine_deinit(obj);
 }
 
 int main(int argc, char *argv[])
-- 
2.19.1

[PATCH v2 2/2] tests/unit/test-smp-parse: Fix a check-patch complain

[Yanan Wang]

Checkpatch.pl reports errors like below for commit 9e8e393bb7.
Let's fix it with a simpler format.
ERROR: space required after that close brace '}'
+    SMPTestData *data = &(SMPTestData){{ }};

Fixes: 9e8e393bb7 ("tests/unit: Add an unit test for smp parsing")
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 tests/unit/test-smp-parse.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 872512aa37..7805a34b29 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -514,7 +514,7 @@ static void test_generic(void)
     Object *obj = smp_test_machine_init();
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
-    SMPTestData *data = &(SMPTestData){{ }};
+    SMPTestData *data = &(SMPTestData){};
     int i;
 
     for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
@@ -548,7 +548,7 @@ static void test_with_dies(void)
     Object *obj = smp_test_machine_init();
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
-    SMPTestData *data = &(SMPTestData){{ }};
+    SMPTestData *data = &(SMPTestData){};
     unsigned int num_dies = 2;
     int i;
 
-- 
2.19.1

Re: [PATCH v2 1/2] tests/unit/test-smp-parse: Make an unified name for the tested machine

[Andrew Jones]

On Thu, Nov 11, 2021 at 10:44:28AM +0800, Yanan Wang wrote:
> Currently, the name of the tested machine in the expected error
> messages is hardcoded as "(null)" which is not good, because the
> actual generated name of the machine maybe "(null)" or "(NULL)"
> which will cause an unexpected test failure in some CI platforms.
> 
> So let's rename the tested machine with an unified string and
> tweak the expected error messages accordingly.
> 
> Fixes: 9e8e393bb7 ("tests/unit: Add an unit test for smp parsing")
> Reported-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  tests/unit/test-smp-parse.c | 38 ++++++++++++++++++++++++++-----------
>  1 file changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
> index cbe0c99049..872512aa37 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
> @@ -315,13 +317,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",
>      },
>  };
>  
> @@ -480,26 +482,41 @@ 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 Object *smp_test_machine_init(void)
>  {
> +    Object *obj = object_new(TYPE_MACHINE);
> +    MachineClass *mc = MACHINE_GET_CLASS(obj);
> +
> +    g_free(mc->name);
> +    mc->name = g_strdup(SMP_MACHINE_NAME);
> +
>      mc->min_cpus = MIN_CPUS;
>      mc->max_cpus = MAX_CPUS;
>  
>      mc->smp_props.prefer_sockets = true;
>      mc->smp_props.dies_supported = false;
> +
> +    return obj;
> +}
> +
> +static void smp_test_machine_deinit(Object *obj)
> +{
> +    MachineClass *mc = MACHINE_GET_CLASS(obj);
> +
> +    g_free(mc->name);
> +    mc->name = NULL;
> +
> +    object_unref(obj);
>  }
>  
>  static void test_generic(void)
>  {
> -    Object *obj = object_new(TYPE_MACHINE);
> +    Object *obj = smp_test_machine_init();
>      MachineState *ms = MACHINE(obj);
>      MachineClass *mc = MACHINE_GET_CLASS(obj);
>      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);
> @@ -523,19 +540,18 @@ static void test_generic(void)
>          smp_parse_test(ms, data, false);
>      }
>  
> -    object_unref(obj);
> +    smp_test_machine_deinit(obj);
>  }
>  
>  static void test_with_dies(void)
>  {
> -    Object *obj = object_new(TYPE_MACHINE);
> +    Object *obj = smp_test_machine_init();
>      MachineState *ms = MACHINE(obj);
>      MachineClass *mc = MACHINE_GET_CLASS(obj);
>      SMPTestData *data = &(SMPTestData){{ }};
>      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++) {
> @@ -575,7 +591,7 @@ static void test_with_dies(void)
>          smp_parse_test(ms, data, false);
>      }
>  
> -    object_unref(obj);
> +    smp_test_machine_deinit(obj);
>  }
>  
>  int main(int argc, char *argv[])
> -- 
> 2.19.1
>

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

Re: [PATCH v2 2/2] tests/unit/test-smp-parse: Fix a check-patch complain

[Andrew Jones]

On Thu, Nov 11, 2021 at 10:44:29AM +0800, Yanan Wang wrote:
> Checkpatch.pl reports errors like below for commit 9e8e393bb7.
> Let's fix it with a simpler format.
> ERROR: space required after that close brace '}'
> +    SMPTestData *data = &(SMPTestData){{ }};
> 
> Fixes: 9e8e393bb7 ("tests/unit: Add an unit test for smp parsing")
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  tests/unit/test-smp-parse.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
> index 872512aa37..7805a34b29 100644
> --- a/tests/unit/test-smp-parse.c
> +++ b/tests/unit/test-smp-parse.c
> @@ -514,7 +514,7 @@ static void test_generic(void)
>      Object *obj = smp_test_machine_init();
>      MachineState *ms = MACHINE(obj);
>      MachineClass *mc = MACHINE_GET_CLASS(obj);
> -    SMPTestData *data = &(SMPTestData){{ }};
> +    SMPTestData *data = &(SMPTestData){};
>      int i;
>  
>      for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
> @@ -548,7 +548,7 @@ static void test_with_dies(void)
>      Object *obj = smp_test_machine_init();
>      MachineState *ms = MACHINE(obj);
>      MachineClass *mc = MACHINE_GET_CLASS(obj);
> -    SMPTestData *data = &(SMPTestData){{ }};
> +    SMPTestData *data = &(SMPTestData){};
>      unsigned int num_dies = 2;
>      int i;
>  
> -- 
> 2.19.1
>

I just did some googling to refresh my memory on this, because I recall
{0} being recommended at some point. From what I can tell, {} is ok for
gcc, maybe also clang, but {0} is ANSI. The reasoning was that {} should
not be empty, and since element names are not required, '0' is enough to
assign the first element to zero. Also, as it's not required to list
each element, then that's enough for the whole struct. That said, {}
seems to be more popular, so we can probably assume the tools we support
also support it.

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

Thanks,
drew

Re: [PATCH v2 2/2] tests/unit/test-smp-parse: Fix a check-patch complain

[Thomas Huth]

On 11/11/2021 09.43, Andrew Jones wrote:
> On Thu, Nov 11, 2021 at 10:44:29AM +0800, Yanan Wang wrote:
>> Checkpatch.pl reports errors like below for commit 9e8e393bb7.
>> Let's fix it with a simpler format.
>> ERROR: space required after that close brace '}'
>> +    SMPTestData *data = &(SMPTestData){{ }};
>>
>> Fixes: 9e8e393bb7 ("tests/unit: Add an unit test for smp parsing")
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>>   tests/unit/test-smp-parse.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
>> index 872512aa37..7805a34b29 100644
>> --- a/tests/unit/test-smp-parse.c
>> +++ b/tests/unit/test-smp-parse.c
>> @@ -514,7 +514,7 @@ static void test_generic(void)
>>       Object *obj = smp_test_machine_init();
>>       MachineState *ms = MACHINE(obj);
>>       MachineClass *mc = MACHINE_GET_CLASS(obj);
>> -    SMPTestData *data = &(SMPTestData){{ }};
>> +    SMPTestData *data = &(SMPTestData){};
>>       int i;
>>   
>>       for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
>> @@ -548,7 +548,7 @@ static void test_with_dies(void)
>>       Object *obj = smp_test_machine_init();
>>       MachineState *ms = MACHINE(obj);
>>       MachineClass *mc = MACHINE_GET_CLASS(obj);
>> -    SMPTestData *data = &(SMPTestData){{ }};
>> +    SMPTestData *data = &(SMPTestData){};
>>       unsigned int num_dies = 2;
>>       int i;
>>   
>> -- 
>> 2.19.1
>>
> 
> I just did some googling to refresh my memory on this, because I recall
> {0} being recommended at some point. From what I can tell, {} is ok for
> gcc, maybe also clang, but {0} is ANSI. The reasoning was that {} should
> not be empty, and since element names are not required, '0' is enough to
> assign the first element to zero. Also, as it's not required to list
> each element, then that's enough for the whole struct. That said, {}
> seems to be more popular, so we can probably assume the tools we support
> also support it.

Some older compilers did not like {0} (see e.g. commit 555b3d67bc64), that's 
why we use {} in the QEMU code IIRC.

  Thomas