Re: [PATCH v3 2/3] qdev-monitor: Check sysbus device type before creating it

[Philippe Mathieu-Daudé]

On 10/29/21 19:34, Philippe Mathieu-Daudé wrote:
> On 10/29/21 16:22, Damien Hedde wrote:
>> Add an early check to test if the requested sysbus device type
>> is allowed by the current machine before creating the device. This
>> impacts both -device cli option and device_add qmp command.
>>
>> Before this patch, the check was done well after the device has
>> been created (in a machine init done notifier). 

Before / Until? Also you could mention "in a machine init_done
notifier, which we will remove in the next commit".

>> We can now report
>> the error right away.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> ---
>>
>> v3: update error message
>> ---
>>  softmmu/qdev-monitor.c | 11 +++++++++++
> 
> /me wonders why this file is named '-monitor'.
> 
>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
>> index 4851de51a5..e49d9773d2 100644
>> --- a/softmmu/qdev-monitor.c
>> +++ b/softmmu/qdev-monitor.c
>> @@ -42,6 +42,7 @@
>>  #include "qemu/cutils.h"
>>  #include "hw/qdev-properties.h"
>>  #include "hw/clock.h"
>> +#include "hw/boards.h"
>>  
>>  /*
>>   * Aliases were a bad idea from the start.  Let's keep them
>> @@ -254,6 +255,16 @@ static DeviceClass *qdev_get_device_class(const char 
>> **driver, Error **errp)
>>          return NULL;
>>      }
>>  
>> +    if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE)) {
>> +        /* sysbus devices need to be allowed by the machine */
>> +        MachineClass *mc = 
>> MACHINE_CLASS(object_get_class(qdev_get_machine()));
>> +        if (!device_type_is_dynamic_sysbus(mc, *driver)) {
>> +            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
> 
> Per include/qapi/qmp/qerror.h:
> 
>   /*
>    * These macros will go away, please don't use in new code, and do not
>    * add new ones!
>    */
> 
>   #define QERR_INVALID_PARAMETER_VALUE \
>       "Parameter '%s' expects %s"
> 
>> +                       "a dynamic sysbus device type for the machine");
> 
> Besides, this is easier to read:
> 
>             error_setg(errp, "Parameter 'driver' expects a dynamic"
>                              " sysbus device type for the machine");
> 
> Maybe remove QERR_INVALID_PARAMETER_VALUE from qdev_get_device_class()
> in a preliminary patch?
> 
> Otherwise:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
>> +            return NULL;
>> +        }
>> +    }
>> +
>>      return dc;
>>  }
>>  
>>
>