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

[Philippe Mathieu-Daudé]

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