On 11/11/21 10:31, wangyanan (Y) wrote: > > On 2021/11/11 17:14, Philippe Mathieu-Daudé wrote: >> On 10/28/21 17:09, Philippe Mathieu-Daudé wrote: >>> From: Yanan Wang <wangyana...@huawei.com> >>> >>> Now that we have a generic parser smp_parse(), let's add an unit >>> test for the code. All possible valid/invalid SMP configurations >>> that the user can specify are covered. >>> >>> Signed-off-by: Yanan Wang <wangyana...@huawei.com> >>> Reviewed-by: Andrew Jones <drjo...@redhat.com> >>> Tested-by: Philippe Mathieu-Daudé <phi...@redhat.com> >>> Message-Id: <20211026034659.22040-3-wangyana...@huawei.com> >>> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com> >>> --- >>> tests/unit/test-smp-parse.c | 594 ++++++++++++++++++++++++++++++++++++ >>> MAINTAINERS | 1 + >>> tests/unit/meson.build | 1 + >>> 3 files changed, 596 insertions(+) >>> create mode 100644 tests/unit/test-smp-parse.c >>> +static struct SMPTestData data_generic_valid[] = { >>> + { >>> + /* config: no configuration provided >>> + * expect: cpus=1,sockets=1,cores=1,threads=1,maxcpus=1 */ >> [1] >> >>> + .config = SMP_CONFIG_GENERIC(F, 0, F, 0, F, 0, F, 0, F, 0), >>> + .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1), >>> + .expect_prefer_cores = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1), >>> + }, { >>> +static void test_generic(void) >>> +{ >>> + Object *obj = object_new(TYPE_MACHINE); >>> + MachineState *ms = MACHINE(obj); >>> + MachineClass *mc = MACHINE_GET_CLASS(obj); >> Watch out, while you create a machine instance in each >> test, there is only one machine class registered (see >> type_register_static(&smp_machine_info) below in [2]), >> ... > Yes, I noticed this. So on the top of each sub-test function, the > properties > of the single machine class is re-initialized by > smp_machine_class_init(mc). > See [*] below. >>> + SMPTestData *data = &(SMPTestData){0}; >>> + 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); >>> + >>> + 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); >>> + } >>> + >>> + /* Reset the supported min CPUs and max CPUs */ >>> + mc->min_cpus = 2; >>> + mc->max_cpus = 511; >> ... and here you are modifying the single machine class state, ... >> >>> + >>> + for (i = 0; i < ARRAY_SIZE(data_generic_invalid); i++) { >>> + *data = data_generic_invalid[i]; >>> + unsupported_params_init(mc, data); >>> + >>> + smp_parse_test(ms, data, false); >>> + } >>> + >>> + object_unref(obj); >>> +} >>> + >>> +static void test_with_dies(void) >>> +{ >>> + Object *obj = object_new(TYPE_MACHINE); >>> + MachineState *ms = MACHINE(obj); >>> + MachineClass *mc = MACHINE_GET_CLASS(obj); >> ... so here the machine class state is inconsistent, ... >> >>> + SMPTestData *data = &(SMPTestData){0}; >>> + unsigned int num_dies = 2; >>> + int i; >>> + >>> + smp_machine_class_init(mc); > And here [*]. >>> + 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); >>> + >>> + /* when dies parameter is omitted, it will be set as 1 */ >>> + data->expect_prefer_sockets.dies = 1; >>> + data->expect_prefer_cores.dies = 1; >>> + >>> + smp_parse_test(ms, data, true); >> ... in particular the first test [1] is tested with mc->min_cpus = 2. >> >> I wonder why you are not getting: >> >> Output error report: Invalid SMP CPUs 1. The min CPUs supported by >> machine '(null)' is 2 >> >> for [1]. > So as I have explained above, we won't get an output error report like > this here. :)
I see. IMHO this is bad practice example, so I'll send a cleanup patch.