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

["wangyanan (Y)" <wangyanan55@huawei.com>]

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