| Message ID | 20211029142258.484907-3-damien.hedde@greensocs.com |
|---|---|
| State | New |
| Headers | show |
| Series | Dynamic sysbus device check error report | expand |
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). 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; > } > >
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; >> } >> >> >
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", + "a dynamic sysbus device type for the machine"); + return NULL; + } + } + return dc; }