It's no sense to check the CPU type when mc->valid_cpu_types[0] is
NULL, which is a program error. Raise an assert on this.
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é.
Besides, @cc comes from machine->cpu_type or mc->default_cpu_type. For
the later case, it can be NULL and it's also a program error. We should
use assert() in this case.
Signed-off-by: Gavin Shan <gshan@redhat.com>
v9: assert(mc->valid_cpu_types[0] != NULL) (Phil)
assert(cc != NULL); (Phil)
---
hw/core/machine.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1797e002f9..4ae9aaee8e 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1400,6 +1400,7 @@ static bool is_cpu_type_supported(const MachineState *machine, Error **errp)
* type is provided through '-cpu' option.
*/
if (mc->valid_cpu_types && machine->cpu_type) {
+ assert(mc->valid_cpu_types[0] != NULL);
for (i = 0; mc->valid_cpu_types[i]; i++) {
if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) {
break;
@@ -1409,20 +1410,27 @@ 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;
}
}
/* Check if CPU type is deprecated and warn if so */
cc = CPU_CLASS(oc);
- if (cc && cc->deprecation_note) {
+ assert(cc != NULL);
+ if (cc->deprecation_note) {
warn_report("CPU model %s is deprecated -- %s",
machine->cpu_type, cc->deprecation_note);
}
--
2.42.0
On 4/12/23 01:47, Gavin Shan wrote:
> It's no sense to check the CPU type when mc->valid_cpu_types[0] is
> NULL, which is a program error. Raise an assert on this.
>
> 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é.
>
> Besides, @cc comes from machine->cpu_type or mc->default_cpu_type. For
> the later case, it can be NULL and it's also a program error. We should
> use assert() in this case.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> v9: assert(mc->valid_cpu_types[0] != NULL) (Phil)
> assert(cc != NULL); (Phil)
> ---
> hw/core/machine.c | 20 ++++++++++++++------
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 1797e002f9..4ae9aaee8e 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1400,6 +1400,7 @@ static bool is_cpu_type_supported(const MachineState *machine, Error **errp)
> * type is provided through '-cpu' option.
> */
> if (mc->valid_cpu_types && machine->cpu_type) {
> + assert(mc->valid_cpu_types[0] != NULL);
> for (i = 0; mc->valid_cpu_types[i]; i++) {
> if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) {
> break;
> @@ -1409,20 +1410,27 @@ 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;
> }
> }
>
> /* Check if CPU type is deprecated and warn if so */
> cc = CPU_CLASS(oc);
> - if (cc && cc->deprecation_note) {
> + assert(cc != NULL);
> + if (cc->deprecation_note) {
> warn_report("CPU model %s is deprecated -- %s",
> machine->cpu_type, cc->deprecation_note);
> }
Since we were getting:
$ qemu-system-s390x -M none
QEMU 8.2.50 monitor - type 'help' for more information
qemu-system-s390x: ../../hw/core/machine.c:1444: _Bool
is_cpu_type_supported(const MachineState *, Error **): Assertion `cc !=
NULL' failed.
Aborted (core dumped)
I added a check on mc->valid_cpu_types before calling
is_cpu_type_supported() in the previous patch. See commit acbadc5a29.
On 1/6/24 08:09, Philippe Mathieu-Daudé wrote:
> On 4/12/23 01:47, Gavin Shan wrote:
>> It's no sense to check the CPU type when mc->valid_cpu_types[0] is
>> NULL, which is a program error. Raise an assert on this.
>>
>> 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é.
>>
>> Besides, @cc comes from machine->cpu_type or mc->default_cpu_type. For
>> the later case, it can be NULL and it's also a program error. We should
>> use assert() in this case.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> v9: assert(mc->valid_cpu_types[0] != NULL) (Phil)
>> assert(cc != NULL); (Phil)
>> ---
>> hw/core/machine.c | 20 ++++++++++++++------
>> 1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 1797e002f9..4ae9aaee8e 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -1400,6 +1400,7 @@ static bool is_cpu_type_supported(const MachineState *machine, Error **errp)
>> * type is provided through '-cpu' option.
>> */
>> if (mc->valid_cpu_types && machine->cpu_type) {
>> + assert(mc->valid_cpu_types[0] != NULL);
>> for (i = 0; mc->valid_cpu_types[i]; i++) {
>> if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) {
>> break;
>> @@ -1409,20 +1410,27 @@ 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;
>> }
>> }
>> /* Check if CPU type is deprecated and warn if so */
>> cc = CPU_CLASS(oc);
>> - if (cc && cc->deprecation_note) {
>> + assert(cc != NULL);
>> + if (cc->deprecation_note) {
>> warn_report("CPU model %s is deprecated -- %s",
>> machine->cpu_type, cc->deprecation_note);
>> }
>
> Since we were getting:
>
> $ qemu-system-s390x -M none
> QEMU 8.2.50 monitor - type 'help' for more information
> qemu-system-s390x: ../../hw/core/machine.c:1444: _Bool is_cpu_type_supported(const MachineState *, Error **): Assertion `cc != NULL' failed.
> Aborted (core dumped)
>
> I added a check on mc->valid_cpu_types before calling
> is_cpu_type_supported() in the previous patch. See commit acbadc5a29.
>
Phil, thanks for fixing it up and it looks good to me.
Thanks,
Gavin
© 2016 - 2025 Red Hat, Inc.