Re: [PATCH v6] hw/core/qdev: cleanup Error ** variables

[Markus Armbruster <armbru@redhat.com>]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> Rename Error ** parameter in check_only_migratable to common errp.
>
> In device_set_realized:
>
>  - Move "if (local_err != NULL)" closer to error setters.
>
>  - Drop 'Error **local_errp': it doesn't save any LoCs, but it's very
>    unusual.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>
> v6: enhance grammar in comment [Eric]
>     add r-b by Eric and Marc-André
>
>  hw/core/qdev.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index cf1ba28fe3..82d3ee590a 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -820,12 +820,12 @@ static bool device_get_realized(Object *obj, Error **errp)
>      return dev->realized;
>  }
>  
> -static bool check_only_migratable(Object *obj, Error **err)
> +static bool check_only_migratable(Object *obj, Error **errp)
>  {
>      DeviceClass *dc = DEVICE_GET_CLASS(obj);
>  
>      if (!vmstate_check_only_migratable(dc->vmsd)) {
> -        error_setg(err, "Device %s is not migratable, but "
> +        error_setg(errp, "Device %s is not migratable, but "
>                     "--only-migratable was specified",
>                     object_get_typename(obj));
>          return false;
> @@ -874,10 +874,9 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>  
>          if (dc->realize) {
>              dc->realize(dev, &local_err);
> -        }
> -
> -        if (local_err != NULL) {
> -            goto fail;
> +            if (local_err != NULL) {
> +                goto fail;
> +            }
>          }

Yes, this is the more conventional usage.

>  
>          DEVICE_LISTENER_CALL(realize, Forward, dev);
> @@ -918,27 +917,26 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>         }
>  
>      } else if (!value && dev->realized) {
> -        Error **local_errp = NULL;
> +        /* We want local_err to track only the first error */
>          QLIST_FOREACH(bus, &dev->child_bus, sibling) {
> -            local_errp = local_err ? NULL : &local_err;
>              object_property_set_bool(OBJECT(bus), false, "realized",
> -                                     local_errp);
> +                                     local_err ? NULL : &local_err);
>          }

This is a rather unusual way to keep the first error of several.
qapi/error.h advises:

 * Receive and accumulate multiple errors (first one wins):
 *     Error *err = NULL, *local_err = NULL;
 *     foo(arg, &err);
 *     bar(arg, &local_err);
 *     error_propagate(&err, local_err);
 *     if (err) {
 *         handle the error...
 *     }

If replacing this by the usual way is too troublesome now, we can do it
after the ERRP_AUTO_PROPAGATE() conversion.  Your choice.

>          if (qdev_get_vmsd(dev)) {
>              vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
>          }
>          if (dc->unrealize) {
> -            local_errp = local_err ? NULL : &local_err;
> -            dc->unrealize(dev, local_errp);
> +            dc->unrealize(dev, local_err ? NULL : &local_err);
>          }
>          dev->pending_deleted_event = true;
>          DEVICE_LISTENER_CALL(unrealize, Reverse, dev);
> -    }
>  
> -    if (local_err != NULL) {
> -        goto fail;
> +        if (local_err != NULL) {
> +            goto fail;
> +        }
>      }
>  
> +    assert(local_err == NULL);

Not sure this one's worth it's keep with the usage of local_err cleaned
up.

>      dev->realized = value;
>      return;
>  
> @@ -976,7 +974,7 @@ static bool device_get_hotpluggable(Object *obj, Error **errp)
>                                  qbus_is_hotpluggable(dev->parent_bus));
>  }
>  
> -static bool device_get_hotplugged(Object *obj, Error **err)
> +static bool device_get_hotplugged(Object *obj, Error **errp)
>  {
>      DeviceState *dev = DEVICE(obj);

In case you want to clean up afterwards rather than now:
Reviewed-by: Markus Armbruster <armbru@redhat.com>