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
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~
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
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.
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.
>
> .
© 2016 - 2026 Red Hat, Inc.