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.he...@greensocs.com>
>> Reviewed-by: Alistair Francis <alistair.fran...@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é <phi...@redhat.com>
>
>> + return NULL;
>> + }
>> + }
>> +
>> return dc;
>> }
>>
>>
>
