Re: [Qemu-devel] [PATCH v2 2/3] globals: Allow global properties to be optional

[Cornelia Huck]

On Thu, 10 Jan 2019 00:02:58 -0200
Eduardo Habkost <address@hidden> wrote:

> Making some global properties optional will let us simplify
> compat code when a given property works on most (but not all)
> subclasses of a given type.
> 
> Device types will be able to opt out from optional compat
> properties by simply not registering those properties, or by
> making the property setter report an error.

I'm not sure whether we want to conflate "property does not exist" and
"setter returns an error". I can see the value in "apply a setting if
the property exists; if it doesn't exist, the setting is not
applicable". But if you also allow the setter to return an error, you
will not notice if you e.g. have been providing junk in the value and
it errors out everywhere.

> 
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
> Note: the "An error is fatal for non-hotplugged devices [...]"
> comment that appears in the diff scope is inaccurate, but I will
> fix that in a separate patch because I don't want this to get
> into the way of fixing the crash.
> ---
>  include/hw/qdev-core.h | 3 +++
>  qom/object.c           | 4 +++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index bc014c1c9f..463e1ddb1e 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -250,6 +250,8 @@ struct PropertyInfo {
>  /**
>   * GlobalProperty:
>   * @used: Set to true if property was used when initializing a device.
> + * @optional: If set to true, errors when applying the property won't be
> + *            reported by object_apply_global_props().
>   *
>   * An error is fatal for non-hotplugged devices, when the global is applied.
>   */
> @@ -258,6 +260,7 @@ typedef struct GlobalProperty {
>      const char *property;
>      const char *value;
>      bool used;
> +    bool optional;
>  } GlobalProperty;
>  
>  static inline void
> diff --git a/qom/object.c b/qom/object.c
> index 4e5226ca12..226ddf0189 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -387,7 +387,9 @@ void object_apply_global_props(Object *obj, const 
> GPtrArray *props, Error **errp
>          }
>          p->used = true;
>          object_property_parse(obj, p->value, p->property, &err);
> -        if (err != NULL) {
> +        if (p->optional) {
> +            error_free(err);
> +        } else if (err != NULL) {
>              error_prepend(&err, "can't apply global %s.%s=%s: ",
>                            p->driver, p->property, p->value);
>              /*