[v8] Unified CPU type check

[PATCH v8 3/9] machine: Improve is_cpu_type_supported()

Posted by Gavin Shan 12 months ago

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(-)

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;
         }
     }
-- 
2.42.0

Re: [PATCH v8 3/9] machine: Improve is_cpu_type_supported()

Posted by Philippe Mathieu-Daudé 12 months ago

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(-)

Re: [PATCH v8 3/9] machine: Improve is_cpu_type_supported()

Posted by Gavin Shan 11 months, 4 weeks ago

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

Re: [PATCH v8 3/9] machine: Improve is_cpu_type_supported()

Posted by Gavin Shan 11 months, 4 weeks ago

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