Re: [Qemu-devel] [PATCH 05/10] qdev: GlobalProperty.errp field

[Markus Armbruster]

Eduardo Habkost <address@hidden> writes:

> The new field will allow error handling to be configured by
> qdev_prop_register_global() callers: &error_fatal and
> &error_abort can be used to make QEMU exit or abort if any errors
> are reported when applying the properties.
>
> Suggested-by: Paolo Bonzini <address@hidden>
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
>  hw/core/qdev-properties.c | 8 ++++++--
>  include/hw/qdev-core.h    | 4 ++++
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index cd19603..0fe7214 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1078,10 +1078,14 @@ static void 
> qdev_prop_set_globals_for_type(DeviceState *dev,
>          prop->used = true;
>          object_property_parse(OBJECT(dev), prop->value, prop->property, 
> &err);
>          if (err != NULL) {
> -            assert(prop->user_provided);
>              error_prepend(&err, "can't apply global %s.%s=%s: ",
>                            prop->driver, prop->property, prop->value);
> -            error_reportf_err(err, "Warning: ");
> +            if (prop->errp) {
> +                error_propagate(prop->errp, err);
> +            } else {
> +                assert(prop->user_provided);
> +                error_reportf_err(err, "Warning: ");
> +            }
>          }
>      }
>  }

Now I understand.  Suggest to squash the previous patch into this one.

> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 24aa0a7..16e7cc0 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -256,6 +256,9 @@ struct PropertyInfo {
>  
>  /**
>   * GlobalProperty:
> + * @errp: If set, error_propagate() will be called on errors when applying
> + *        the property. &error_abort or &error_fatal may be used to make
> + *        errors automatically abort or exit QEMU.

"If set" is awkward, because it's not what we usually mean when we talk
about an error being set.  Suggest "If non-null".

But what makes null special isn't that errors won't be propagated.  In
fact, the code behaves as if they were (propagating to null frees the
error, which is exactly what the code does), *except* for one thing that
isn't mentioned here, but should be: we print a warning.

What about:

 * @errp: Error destination, used like a first argument of error_setg(),
 * except with null @errp, we print warnings instead of ignoring errors
 * silently.

>   * @user_provided: Set to true if property comes from user-provided config
>   * (command-line or config file).
>   * @used: Set to true if property was used when initializing a device.
> @@ -264,6 +267,7 @@ typedef struct GlobalProperty {
>      const char *driver;
>      const char *property;
>      const char *value;
> +    Error **errp;
>      bool user_provided;
>      bool used;
>  } GlobalProperty;