diff mbox series

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

Message ID 20211111100351.2153662-4-philmd@redhat.com
State New
Headers show
Series tests/unit: Fix test-smp-parse | expand

Commit Message

Philippe Mathieu-Daudé Nov. 11, 2021, 10:03 a.m. UTC
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(-)

Comments

Andrew Jones Nov. 11, 2021, 12:56 p.m. UTC | #1
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>
Richard Henderson Nov. 11, 2021, 2:18 p.m. UTC | #2
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~
wangyanan (Y) Nov. 12, 2021, 2:28 a.m. UTC | #3
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
Philippe Mathieu-Daudé Nov. 15, 2021, 10:16 a.m. UTC | #4
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.
wangyanan (Y) Nov. 16, 2021, 11:15 a.m. UTC | #5
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
> .
diff mbox series

Patch

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)