diff mbox series

[RFC,v5,010/126] hw/core/qdev: cleanup Error ** variables

Message ID 20191011160552.22907-11-vsementsov@virtuozzo.com (mailing list archive)
State New, archived
Headers show
Series error: auto propagated local_err | expand

Commit Message

Vladimir Sementsov-Ogievskiy Oct. 11, 2019, 4:03 p.m. UTC 
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>
---
 hw/core/qdev.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

Comments

Eric Blake Oct. 11, 2019, 4:52 p.m. UTC | #1 
On 10/11/19 11:03 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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>
> ---
>   hw/core/qdev.c | 28 +++++++++++++---------------
>   1 file changed, 13 insertions(+), 15 deletions(-)
> 

> @@ -894,27 +893,26 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>          }
>   
>       } else if (!value && dev->realized) {
> -        Error **local_errp = NULL;
> +        /* We want to catch in local_err only first error */

grammar:
/* 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);
>           }

Otherwise,
Reviewed-by: Eric Blake <eblake@redhat.com>
Marc-André Lureau Nov. 8, 2019, 8:55 p.m. UTC | #2 
On Fri, Oct 11, 2019 at 8:18 PM Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
>
> 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>
> ---
>  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 cbad6c1d55..e3be8cc3c4 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -796,12 +796,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;
> @@ -850,10 +850,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;
> +            }
>          }
>
>          DEVICE_LISTENER_CALL(realize, Forward, dev);
> @@ -894,27 +893,26 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>         }
>
>      } else if (!value && dev->realized) {
> -        Error **local_errp = NULL;
> +        /* We want to catch in local_err only 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);
>          }
>          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);
>      dev->realized = value;
>      return;
>
> @@ -952,7 +950,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);
>
> --
> 2.21.0
>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
diff mbox series

Patch

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cbad6c1d55..e3be8cc3c4 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -796,12 +796,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;
@@ -850,10 +850,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;
+            }
         }
 
         DEVICE_LISTENER_CALL(realize, Forward, dev);
@@ -894,27 +893,26 @@  static void device_set_realized(Object *obj, bool value, Error **errp)
        }
 
     } else if (!value && dev->realized) {
-        Error **local_errp = NULL;
+        /* We want to catch in local_err only 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);
         }
         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);
     dev->realized = value;
     return;
 
@@ -952,7 +950,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);