| Message ID | 20231129042012.277831-4-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Unified CPU type check | expand |
On 29/11/23 05:20, Gavin Shan wrote: > It's no sense to check the CPU type when mc->valid_cpu_types[0] is > NULL. This case is a programming error, right? We should simply: assert(!mc->valid_cpu_types || *mc->valid_cpu_types); > So the check is skipped for this particular case. The constraint > has been taken when the error messags are appended. > > A precise hint for the error message is given when mc->valid_cpu_types[0] > is the only valid entry. Besides, enumeration on mc->valid_cpu_types[0] > when we have mutiple valid entries there is avoided to increase the code > readability, as suggested by Philippe Mathieu-Daudé. > > Signed-off-by: Gavin Shan <gshan@redhat.com> > --- > hw/core/machine.c | 18 ++++++++++++------ > 1 file changed, 12 insertions(+), 6 deletions(-)
Hi Phil On 12/1/23 20:57, Philippe Mathieu-Daudé wrote: > On 29/11/23 05:20, Gavin Shan wrote: >> It's no sense to check the CPU type when mc->valid_cpu_types[0] is >> NULL. > > This case is a programming error, right? We should simply: > > assert(!mc->valid_cpu_types || *mc->valid_cpu_types); > Yes, assert() should be used instead. I will do this like below: if (mc->valid_cpu_types && machine->cpu_type) { assert(mc->valid_cpu_types[1] != NULL); >> So the check is skipped for this particular case. The constraint >> has been taken when the error messags are appended. >> >> A precise hint for the error message is given when mc->valid_cpu_types[0] >> is the only valid entry. Besides, enumeration on mc->valid_cpu_types[0] >> when we have mutiple valid entries there is avoided to increase the code >> readability, as suggested by Philippe Mathieu-Daudé. >> >> Signed-off-by: Gavin Shan <gshan@redhat.com> >> --- >> hw/core/machine.c | 18 ++++++++++++------ >> 1 file changed, 12 insertions(+), 6 deletions(-) > Thanks, Gavin
Hi Phil, On 12/4/23 09:20, Gavin Shan wrote: > On 12/1/23 20:57, Philippe Mathieu-Daudé wrote: >> On 29/11/23 05:20, Gavin Shan wrote: >>> It's no sense to check the CPU type when mc->valid_cpu_types[0] is >>> NULL. >> >> This case is a programming error, right? We should simply: >> >> assert(!mc->valid_cpu_types || *mc->valid_cpu_types); >> > > Yes, assert() should be used instead. I will do this like below: > > if (mc->valid_cpu_types && machine->cpu_type) { > assert(mc->valid_cpu_types[1] != NULL); > I meant assert(mc->valid_cpu_types[0] != NULL) and sorry for the confusion. if (mc->valid_cpu_types && machine->cpu_type) { assert(mc->valid_cpu_types[0] != NULL); : } > >>> So the check is skipped for this particular case. The constraint >>> has been taken when the error messags are appended. >>> >>> A precise hint for the error message is given when mc->valid_cpu_types[0] >>> is the only valid entry. Besides, enumeration on mc->valid_cpu_types[0] >>> when we have mutiple valid entries there is avoided to increase the code >>> readability, as suggested by Philippe Mathieu-Daudé. >>> >>> Signed-off-by: Gavin Shan <gshan@redhat.com> >>> --- >>> hw/core/machine.c | 18 ++++++++++++------ >>> 1 file changed, 12 insertions(+), 6 deletions(-) >> Thanks, Gavin
diff --git a/hw/core/machine.c b/hw/core/machine.c index 1797e002f9..c58c84abf5 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -1399,7 +1399,7 @@ static bool is_cpu_type_supported(const MachineState *machine, Error **errp) * 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) { + if (machine->cpu_type && mc->valid_cpu_types && mc->valid_cpu_types[0]) { for (i = 0; mc->valid_cpu_types[i]; i++) { if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) { break; @@ -1409,13 +1409,19 @@ static bool 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); - 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]); + if (!mc->valid_cpu_types[1]) { + error_append_hint(errp, "The only valid type is: %s\n", + mc->valid_cpu_types[0]); + } 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"); } - error_append_hint(errp, "\n"); return false; } }
It's no sense to check the CPU type when mc->valid_cpu_types[0] is NULL. So the check is skipped for this particular case. The constraint has been taken when the error messags are appended. A precise hint for the error message is given when mc->valid_cpu_types[0] is the only valid entry. Besides, enumeration on mc->valid_cpu_types[0] when we have mutiple valid entries there is avoided to increase the code readability, as suggested by Philippe Mathieu-Daudé. Signed-off-by: Gavin Shan <gshan@redhat.com> --- hw/core/machine.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)