| Message ID | 20231126231210.112820-4-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Unified CPU type check | expand |
Hi Gavin, On 27/11/23 00:12, Gavin Shan wrote: > The names of supported CPU models instead of CPU types should be > printed when the user specified CPU type isn't supported, to be > consistent with the output from '-cpu ?'. > > Correct the error messages to print CPU model names instead of CPU > type names. > > Signed-off-by: Gavin Shan <gshan@redhat.com> > --- > hw/core/machine.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 05e1922b89..898c25552a 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -1392,6 +1392,7 @@ 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; > + char *model; > int i; > > /* > @@ -1408,17 +1409,25 @@ static void is_cpu_type_supported(const MachineState *machine, Error **errp) > > /* The user specified CPU type isn't valid */ > if (!mc->valid_cpu_types[i]) { > - error_setg(errp, "Invalid CPU type: %s", machine->cpu_type); > + model = cpu_model_from_type(machine->cpu_type); > + g_assert(model != NULL); > + error_setg(errp, "Invalid CPU type: %s", model); > + g_free(model); g_autofree char *requested = cpu_model_from_type(machine->cpu_type); error_setg(errp, "Invalid CPU type: %s", requested); > + > + model = cpu_model_from_type(mc->valid_cpu_types[0]); > + g_assert(model != NULL); > if (!mc->valid_cpu_types[1]) { > - error_append_hint(errp, "The only valid type is: %s", > - mc->valid_cpu_types[0]); > + error_append_hint(errp, "The only valid type is: %s", model); g_autofree char *model = cpu_model_from_type(mc->valid_cpu_types[0]); error_append_hint(errp, "The only valid type is: %s\n", model); > } else { > - error_append_hint(errp, "The valid types are: %s", > - mc->valid_cpu_types[0]); > + error_append_hint(errp, "The valid types are: %s", model); Please move all the enumeration in this ladder, this makes the logic simpler to follow: error_append_hint(errp, "The valid types are: "); for (i = 0; mc->valid_cpu_types[i]; i++) { g_autofree char *model = cpu_model_from_type(mc->valid_cpu_types[i]); error_append_hint(errp, ", %s", model); } error_append_hint(errp, "\n"); > } > + g_free(model); > > for (i = 1; mc->valid_cpu_types[i]; i++) { > - error_append_hint(errp, ", %s", mc->valid_cpu_types[i]); > + model = cpu_model_from_type(mc->valid_cpu_types[i]); > + g_assert(model != NULL); > + error_append_hint(errp, ", %s", model); > + g_free(model); > } > > error_append_hint(errp, "\n");
Hi Phil, On 11/28/23 20:55, Philippe Mathieu-Daudé wrote: > On 27/11/23 00:12, Gavin Shan wrote: >> The names of supported CPU models instead of CPU types should be >> printed when the user specified CPU type isn't supported, to be >> consistent with the output from '-cpu ?'. >> >> Correct the error messages to print CPU model names instead of CPU >> type names. >> >> Signed-off-by: Gavin Shan <gshan@redhat.com> >> --- >> hw/core/machine.c | 21 +++++++++++++++------ >> 1 file changed, 15 insertions(+), 6 deletions(-) >> >> diff --git a/hw/core/machine.c b/hw/core/machine.c >> index 05e1922b89..898c25552a 100644 >> --- a/hw/core/machine.c >> +++ b/hw/core/machine.c >> @@ -1392,6 +1392,7 @@ 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; >> + char *model; >> int i; >> /* >> @@ -1408,17 +1409,25 @@ static void is_cpu_type_supported(const MachineState *machine, Error **errp) >> /* The user specified CPU type isn't valid */ >> if (!mc->valid_cpu_types[i]) { >> - error_setg(errp, "Invalid CPU type: %s", machine->cpu_type); >> + model = cpu_model_from_type(machine->cpu_type); >> + g_assert(model != NULL); >> + error_setg(errp, "Invalid CPU type: %s", model); >> + g_free(model); > > g_autofree char *requested = cpu_model_from_type(machine->cpu_type); > error_setg(errp, "Invalid CPU type: %s", requested); > Yes, g_autofree shall be used here. Besides, "Invalid CPU type" needs to be "Invalid CPU model". >> + >> + model = cpu_model_from_type(mc->valid_cpu_types[0]); >> + g_assert(model != NULL); >> if (!mc->valid_cpu_types[1]) { >> - error_append_hint(errp, "The only valid type is: %s", >> - mc->valid_cpu_types[0]); >> + error_append_hint(errp, "The only valid type is: %s", model); > > g_autofree char *model = cpu_model_from_type(mc->valid_cpu_types[0]); > error_append_hint(errp, "The only valid type is: %s\n", model); > Yes, as above. >> } else { >> - error_append_hint(errp, "The valid types are: %s", >> - mc->valid_cpu_types[0]); >> + error_append_hint(errp, "The valid types are: %s", model); > > Please move all the enumeration in this ladder, this makes the logic > simpler to follow: > > error_append_hint(errp, "The valid types are: "); > for (i = 0; mc->valid_cpu_types[i]; i++) { > g_autofree char *model = > cpu_model_from_type(mc->valid_cpu_types[i]); > error_append_hint(errp, ", %s", model); > } > error_append_hint(errp, "\n"); > Yes, but we still need to ensure mc->valid_cpu_types[0] != NULL in advance. "The valid types are: " needs to be "The valid models are: ". Besides, your proposed code needs to be adjusted a bit like below. Otherwise, we will get output "The valid types are: , aaa, bbb" } else { error_append_hint(errp, "The valid types are: "); for (i = 0; mc->valid_cpu_types[i]; i++) { error_append_hint(errp, "%s%s", mc->valid_cpu_types[i], mc->valid_cpu_types[i + 1] ? ", " : ""); } error_append_hint(errp, "\n"); } I will have separate PATCH[v8 3/9] to have the changes, together with the precise hint when only mc->valid_cpu_types[0] is valid. >> } >> + g_free(model); >> for (i = 1; mc->valid_cpu_types[i]; i++) { >> - error_append_hint(errp, ", %s", mc->valid_cpu_types[i]); >> + model = cpu_model_from_type(mc->valid_cpu_types[i]); >> + g_assert(model != NULL); >> + error_append_hint(errp, ", %s", model); >> + g_free(model); >> } >> error_append_hint(errp, "\n"); Thanks, Gavin
diff --git a/hw/core/machine.c b/hw/core/machine.c index 05e1922b89..898c25552a 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -1392,6 +1392,7 @@ 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; + char *model; int i; /* @@ -1408,17 +1409,25 @@ static void is_cpu_type_supported(const MachineState *machine, Error **errp) /* The user specified CPU type isn't valid */ if (!mc->valid_cpu_types[i]) { - error_setg(errp, "Invalid CPU type: %s", machine->cpu_type); + model = cpu_model_from_type(machine->cpu_type); + g_assert(model != NULL); + error_setg(errp, "Invalid CPU type: %s", model); + g_free(model); + + model = cpu_model_from_type(mc->valid_cpu_types[0]); + g_assert(model != NULL); if (!mc->valid_cpu_types[1]) { - error_append_hint(errp, "The only valid type is: %s", - mc->valid_cpu_types[0]); + error_append_hint(errp, "The only valid type is: %s", model); } else { - error_append_hint(errp, "The valid types are: %s", - mc->valid_cpu_types[0]); + error_append_hint(errp, "The valid types are: %s", model); } + g_free(model); for (i = 1; mc->valid_cpu_types[i]; i++) { - error_append_hint(errp, ", %s", mc->valid_cpu_types[i]); + model = cpu_model_from_type(mc->valid_cpu_types[i]); + g_assert(model != NULL); + error_append_hint(errp, ", %s", model); + g_free(model); } error_append_hint(errp, "\n");
The names of supported CPU models instead of CPU types should be printed when the user specified CPU type isn't supported, to be consistent with the output from '-cpu ?'. Correct the error messages to print CPU model names instead of CPU type names. Signed-off-by: Gavin Shan <gshan@redhat.com> --- hw/core/machine.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)