The logic, to check if the specified CPU type is supported in
machine_run_board_init(), is independent enough. Factor it out into
helper is_cpu_type_supported(). machine_run_board_init() looks a bit
clean with this. Since we're here, @machine_class is renamed to @mc
to avoid multiple line spanning of code. The error messages and comments
are tweaked a bit either.
No functional change intended.
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
hw/core/machine.c | 90 +++++++++++++++++++++++++++--------------------
1 file changed, 51 insertions(+), 39 deletions(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index b3ef325936..05e1922b89 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1387,13 +1387,57 @@ out:
return r;
}
+static void is_cpu_type_supported(const MachineState *machine, Error **errp)
+{
+ MachineClass *mc = MACHINE_GET_CLASS(machine);
+ ObjectClass *oc = object_class_by_name(machine->cpu_type);
+ CPUClass *cc;
+ int i;
+
+ /*
+ * Check if the user specified CPU type is supported when the valid
+ * CPU types have been determined. Note that the user specified CPU
+ * type is provided through '-cpu' option.
+ */
+ if (mc->valid_cpu_types && machine->cpu_type) {
+ for (i = 0; mc->valid_cpu_types[i]; i++) {
+ if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) {
+ break;
+ }
+ }
+
+ /* The user specified CPU type isn't valid */
+ if (!mc->valid_cpu_types[i]) {
+ error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
+ if (!mc->valid_cpu_types[1]) {
+ error_append_hint(errp, "The only valid type is: %s",
+ mc->valid_cpu_types[0]);
+ } else {
+ error_append_hint(errp, "The valid types are: %s",
+ mc->valid_cpu_types[0]);
+ }
+
+ for (i = 1; mc->valid_cpu_types[i]; i++) {
+ error_append_hint(errp, ", %s", mc->valid_cpu_types[i]);
+ }
+
+ error_append_hint(errp, "\n");
+ return;
+ }
+ }
+
+ /* Check if CPU type is deprecated and warn if so */
+ cc = CPU_CLASS(oc);
+ if (cc && cc->deprecation_note) {
+ warn_report("CPU model %s is deprecated -- %s",
+ machine->cpu_type, cc->deprecation_note);
+ }
+}
void machine_run_board_init(MachineState *machine, const char *mem_path, Error **errp)
{
ERRP_GUARD();
MachineClass *machine_class = MACHINE_GET_CLASS(machine);
- ObjectClass *oc = object_class_by_name(machine->cpu_type);
- CPUClass *cc;
Error *local_err = NULL;
/* This checkpoint is required by replay to separate prior clock
@@ -1449,43 +1493,11 @@ void machine_run_board_init(MachineState *machine, const char *mem_path, Error *
machine->ram = machine_consume_memdev(machine, machine->memdev);
}
- /* If the machine supports the valid_cpu_types check and the user
- * specified a CPU with -cpu check here that the user CPU is supported.
- */
- if (machine_class->valid_cpu_types && machine->cpu_type) {
- int i;
-
- for (i = 0; machine_class->valid_cpu_types[i]; i++) {
- if (object_class_dynamic_cast(oc,
- machine_class->valid_cpu_types[i])) {
- /* The user specified CPU is in the valid field, we are
- * good to go.
- */
- break;
- }
- }
-
- if (!machine_class->valid_cpu_types[i]) {
- /* The user specified CPU is not valid */
- error_setg(&local_err, "Invalid CPU type: %s", machine->cpu_type);
- error_append_hint(&local_err, "The valid types are: %s",
- machine_class->valid_cpu_types[0]);
- for (i = 1; machine_class->valid_cpu_types[i]; i++) {
- error_append_hint(&local_err, ", %s",
- machine_class->valid_cpu_types[i]);
- }
- error_append_hint(&local_err, "\n");
-
- error_propagate(errp, local_err);
- return;
- }
- }
-
- /* Check if CPU type is deprecated and warn if so */
- cc = CPU_CLASS(oc);
- if (cc && cc->deprecation_note) {
- warn_report("CPU model %s is deprecated -- %s", machine->cpu_type,
- cc->deprecation_note);
+ /* Check if the CPU type is supported */
+ is_cpu_type_supported(machine, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
}
if (machine->cgs) {
--
2.42.0Hi Gavin,
On 27/11/23 00:12, Gavin Shan wrote:
> The logic, to check if the specified CPU type is supported in
> machine_run_board_init(), is independent enough. Factor it out into
> helper is_cpu_type_supported(). machine_run_board_init() looks a bit
> clean with this. Since we're here, @machine_class is renamed to @mc
> to avoid multiple line spanning of code. The error messages and comments
> are tweaked a bit either.
>
> No functional change intended.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
> hw/core/machine.c | 90 +++++++++++++++++++++++++++--------------------
> 1 file changed, 51 insertions(+), 39 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index b3ef325936..05e1922b89 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1387,13 +1387,57 @@ out:
> return r;
> }
>
> +static void is_cpu_type_supported(const MachineState *machine, Error **errp)
Functions taking an Error** last argument should return a boolean value.
> +{
> + MachineClass *mc = MACHINE_GET_CLASS(machine);
> + ObjectClass *oc = object_class_by_name(machine->cpu_type);
> + CPUClass *cc;
> + int i;
> +
> + /*
> + * Check if the user specified CPU type is supported when the valid
> + * CPU types have been determined. Note that the user specified CPU
> + * type is provided through '-cpu' option.
> + */
> + if (mc->valid_cpu_types && machine->cpu_type) {
> + for (i = 0; mc->valid_cpu_types[i]; i++) {
> + if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) {
> + break;
> + }
> + }
> +
> + /* The user specified CPU type isn't valid */
> + if (!mc->valid_cpu_types[i]) {
> + error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
> + if (!mc->valid_cpu_types[1]) {
> + error_append_hint(errp, "The only valid type is: %s",
> + mc->valid_cpu_types[0]);
> + } else {
> + error_append_hint(errp, "The valid types are: %s",
> + mc->valid_cpu_types[0]);
> + }
> +
> + for (i = 1; mc->valid_cpu_types[i]; i++) {
> + error_append_hint(errp, ", %s", mc->valid_cpu_types[i]);
> + }
> +
> + error_append_hint(errp, "\n");
> + return;
> + }
> + }
> +
> + /* Check if CPU type is deprecated and warn if so */
> + cc = CPU_CLASS(oc);
> + if (cc && cc->deprecation_note) {
> + warn_report("CPU model %s is deprecated -- %s",
> + machine->cpu_type, cc->deprecation_note);
Why did you move the deprecation warning within the is_supported check?
> + }
> +}
>
> void machine_run_board_init(MachineState *machine, const char *mem_path, Error **errp)
> {
> ERRP_GUARD();
> MachineClass *machine_class = MACHINE_GET_CLASS(machine);
> - ObjectClass *oc = object_class_by_name(machine->cpu_type);
> - CPUClass *cc;
> Error *local_err = NULL;
>
> /* This checkpoint is required by replay to separate prior clock
> @@ -1449,43 +1493,11 @@ void machine_run_board_init(MachineState *machine, const char *mem_path, Error *
> machine->ram = machine_consume_memdev(machine, machine->memdev);
> }
>
> - /* If the machine supports the valid_cpu_types check and the user
> - * specified a CPU with -cpu check here that the user CPU is supported.
> - */
> - if (machine_class->valid_cpu_types && machine->cpu_type) {
> - int i;
> -
> - for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> - if (object_class_dynamic_cast(oc,
> - machine_class->valid_cpu_types[i])) {
> - /* The user specified CPU is in the valid field, we are
> - * good to go.
> - */
> - break;
> - }
> - }
> -
> - if (!machine_class->valid_cpu_types[i]) {
> - /* The user specified CPU is not valid */
> - error_setg(&local_err, "Invalid CPU type: %s", machine->cpu_type);
> - error_append_hint(&local_err, "The valid types are: %s",
> - machine_class->valid_cpu_types[0]);
> - for (i = 1; machine_class->valid_cpu_types[i]; i++) {
> - error_append_hint(&local_err, ", %s",
> - machine_class->valid_cpu_types[i]);
> - }
> - error_append_hint(&local_err, "\n");
> -
> - error_propagate(errp, local_err);
> - return;
> - }
> - }
> -
> - /* Check if CPU type is deprecated and warn if so */
> - cc = CPU_CLASS(oc);
> - if (cc && cc->deprecation_note) {
> - warn_report("CPU model %s is deprecated -- %s", machine->cpu_type,
> - cc->deprecation_note);
> + /* Check if the CPU type is supported */
> + is_cpu_type_supported(machine, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
This becomes:
if (!is_cpu_type_supported(machine, errp)) {
> + return;
> }
>
> if (machine->cgs) {
Hi Phil,
On 11/28/23 20:38, Philippe Mathieu-Daudé wrote:
> On 27/11/23 00:12, Gavin Shan wrote:
>> The logic, to check if the specified CPU type is supported in
>> machine_run_board_init(), is independent enough. Factor it out into
>> helper is_cpu_type_supported(). machine_run_board_init() looks a bit
>> clean with this. Since we're here, @machine_class is renamed to @mc
>> to avoid multiple line spanning of code. The error messages and comments
>> are tweaked a bit either.
>>
>> No functional change intended.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>> hw/core/machine.c | 90 +++++++++++++++++++++++++++--------------------
>> 1 file changed, 51 insertions(+), 39 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index b3ef325936..05e1922b89 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -1387,13 +1387,57 @@ out:
>> return r;
>> }
>> +static void is_cpu_type_supported(const MachineState *machine, Error **errp)
>
> Functions taking an Error** last argument should return a boolean value.
>
Correct, especially @errp instead of @local_err will be passed from
machine_run_board_init() to is_cpu_type_supported(). We needs an
indicator for machine_run_board_init() to bail immediately to avoid
calling mc->init() there in the failing cases.
>> +{
>> + MachineClass *mc = MACHINE_GET_CLASS(machine);
>> + ObjectClass *oc = object_class_by_name(machine->cpu_type);
>> + CPUClass *cc;
>> + int i;
>> +
>> + /*
>> + * Check if the user specified CPU type is supported when the valid
>> + * CPU types have been determined. Note that the user specified CPU
>> + * type is provided through '-cpu' option.
>> + */
>> + if (mc->valid_cpu_types && machine->cpu_type) {
>> + for (i = 0; mc->valid_cpu_types[i]; i++) {
>> + if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) {
>> + break;
>> + }
>> + }
>> +
>> + /* The user specified CPU type isn't valid */
>> + if (!mc->valid_cpu_types[i]) {
>> + error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
>> + if (!mc->valid_cpu_types[1]) {
>> + error_append_hint(errp, "The only valid type is: %s",
>> + mc->valid_cpu_types[0]);
>> + } else {
>> + error_append_hint(errp, "The valid types are: %s",
>> + mc->valid_cpu_types[0]);
>> + }
>> +
>> + for (i = 1; mc->valid_cpu_types[i]; i++) {
>> + error_append_hint(errp, ", %s", mc->valid_cpu_types[i]);
>> + }
>> +
>> + error_append_hint(errp, "\n");
>> + return;
>> + }
>> + }
>> +
>> + /* Check if CPU type is deprecated and warn if so */
>> + cc = CPU_CLASS(oc);
>> + if (cc && cc->deprecation_note) {
>> + warn_report("CPU model %s is deprecated -- %s",
>> + machine->cpu_type, cc->deprecation_note);
>
> Why did you move the deprecation warning within the is_supported check?
>
This check is more relevant to CPU type, to check if the CPU type has
been deprecated. Besides, @oc and @cc can be dropped from machine_run_board_init().
>> + }
>> +}
>> void machine_run_board_init(MachineState *machine, const char *mem_path, Error **errp)
>> {
>> ERRP_GUARD();
>> MachineClass *machine_class = MACHINE_GET_CLASS(machine);
>> - ObjectClass *oc = object_class_by_name(machine->cpu_type);
>> - CPUClass *cc;
>> Error *local_err = NULL;
>> /* This checkpoint is required by replay to separate prior clock
>> @@ -1449,43 +1493,11 @@ void machine_run_board_init(MachineState *machine, const char *mem_path, Error *
>> machine->ram = machine_consume_memdev(machine, machine->memdev);
>> }
>> - /* If the machine supports the valid_cpu_types check and the user
>> - * specified a CPU with -cpu check here that the user CPU is supported.
>> - */
>> - if (machine_class->valid_cpu_types && machine->cpu_type) {
>> - int i;
>> -
>> - for (i = 0; machine_class->valid_cpu_types[i]; i++) {
>> - if (object_class_dynamic_cast(oc,
>> - machine_class->valid_cpu_types[i])) {
>> - /* The user specified CPU is in the valid field, we are
>> - * good to go.
>> - */
>> - break;
>> - }
>> - }
>> -
>> - if (!machine_class->valid_cpu_types[i]) {
>> - /* The user specified CPU is not valid */
>> - error_setg(&local_err, "Invalid CPU type: %s", machine->cpu_type);
>> - error_append_hint(&local_err, "The valid types are: %s",
>> - machine_class->valid_cpu_types[0]);
>> - for (i = 1; machine_class->valid_cpu_types[i]; i++) {
>> - error_append_hint(&local_err, ", %s",
>> - machine_class->valid_cpu_types[i]);
>> - }
>> - error_append_hint(&local_err, "\n");
>> -
>> - error_propagate(errp, local_err);
>> - return;
>> - }
>> - }
>> -
>> - /* Check if CPU type is deprecated and warn if so */
>> - cc = CPU_CLASS(oc);
>> - if (cc && cc->deprecation_note) {
>> - warn_report("CPU model %s is deprecated -- %s", machine->cpu_type,
>> - cc->deprecation_note);
>> + /* Check if the CPU type is supported */
>> + is_cpu_type_supported(machine, &local_err);
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>
> This becomes:
>
> if (!is_cpu_type_supported(machine, errp)) {
>
Nod
>> + return;
>> }
>> if (machine->cgs) {
>
Thanks,
Gavin
© 2016 - 2024 Red Hat, Inc.