Re: [PATCH v3 4/4] qdev: add device policy [RfC]

[Markus Armbruster <armbru@redhat.com>]

Gerd Hoffmann <kraxel@redhat.com> writes:

> Add policies for devices which are deprecated or not secure.
> There are three options: allow, warn and deny.
>
> It's implemented for devices only.  Devices will probably be the main
> user of this.  Also object_new() can't fail as of today so it's a bit
> hard to implement policy checking at object level, especially the 'deny'
> part of it.
>
> TODO: add a command line option to actually set these policies.
>
> Comments are welcome.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/core/qdev.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index f3a996f57dee..0c4e5cec743c 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -43,6 +43,15 @@
>  static bool qdev_hot_added = false;
>  bool qdev_hot_removed = false;
>  
> +enum qdev_policy {
> +    QDEV_ALLOW = 0,
> +    QDEV_WARN  = 1,
> +    QDEV_DENY  = 2,
> +};
> +
> +static enum qdev_policy qdev_deprecated_policy;
> +static enum qdev_policy qdev_not_secure_policy;
> +
>  const VMStateDescription *qdev_get_vmsd(DeviceState *dev)
>  {
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
> @@ -144,6 +153,43 @@ bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp)
>      return true;
>  }
>  
> +static bool qdev_class_check(const char *name, ObjectClass *oc)
> +{
> +    bool allow = true;
> +
> +    if (oc->deprecated) {
> +        switch (qdev_deprecated_policy) {
> +        case QDEV_WARN:
> +            warn_report("device \"%s\" is deprecated", name);
> +            break;
> +        case QDEV_DENY:
> +            error_report("device \"%s\" is deprecated", name);
> +            allow = false;
> +            break;
> +        default:
> +            /* nothing */
> +            break;
> +        }
> +    }
> +
> +    if (oc->not_secure) {
> +        switch (qdev_not_secure_policy) {
> +        case QDEV_WARN:
> +            warn_report("device \"%s\" is not secure", name);
> +            break;
> +        case QDEV_DENY:
> +            error_report("device \"%s\" is not secure", name);
> +            allow = false;
> +            break;
> +        default:
> +            /* nothing */
> +            break;
> +        }
> +    }
> +
> +    return allow;
> +}
> +
>  DeviceState *qdev_new(const char *name)
>  {
>      ObjectClass *oc = object_class_by_name(name);
> @@ -162,14 +208,26 @@ DeviceState *qdev_new(const char *name)
>          error_report("unknown type '%s'", name);
>          abort();
>      }
> +
> +    if (!qdev_class_check(name, oc)) {

Anti-pattern: use of error_report() from within a function that returns
an error via Error **errp argument.

One such call chain: QMP core -> qmp_device_add() -> qdev_device_add()
-> qdev_device_add_from_qdict() -> qdev_new().  Your error message goes
to stderr, which is wrong.

> +        exit(1);

Worse, QMP command device_add can now kill the guest instantly.

You need to lift the check up the call chains some.

> +    }
> +
>      return DEVICE(object_new(name));
>  }
>  
>  DeviceState *qdev_try_new(const char *name)
>  {
> -    if (!module_object_class_by_name(name)) {
> +    ObjectClass *oc = module_object_class_by_name(name);
> +
> +    if (!oc) {
>          return NULL;
>      }
> +
> +    if (!qdev_class_check(name, oc)) {
> +        return NULL;
> +    }
> +
>      return DEVICE(object_new(name));
>  }