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

2019-11-08 Thread Marc-André Lureau 
On Fri, Oct 11, 2019 at 8:18 PM 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 
> ---
>  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, _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, >child_bus, sibling) {
> -local_errp = local_err ? NULL : _err;
>  object_property_set_bool(OBJECT(bus), false, "realized",
> - local_errp);
> + local_err ? NULL : _err);
>  }
>  if (qdev_get_vmsd(dev)) {
>  vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
>  }
>  if (dc->unrealize) {
> -local_errp = local_err ? NULL : _err;
> -dc->unrealize(dev, local_errp);
> +dc->unrealize(dev, local_err ? NULL : _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 


-- 
Marc-André Lureau

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

2019-10-11 Thread Eric Blake 

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 
---
  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, >child_bus, sibling) {
-local_errp = local_err ? NULL : _err;
  object_property_set_bool(OBJECT(bus), false, "realized",
- local_errp);
+ local_err ? NULL : _err);
  }


Otherwise,
Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

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

2019-10-11 Thread Vladimir Sementsov-Ogievskiy 
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 
---
 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, _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, >child_bus, sibling) {
-local_errp = local_err ? NULL : _err;
 object_property_set_bool(OBJECT(bus), false, "realized",
- local_errp);
+ local_err ? NULL : _err);
 }
 if (qdev_get_vmsd(dev)) {
 vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
 }
 if (dc->unrealize) {
-local_errp = local_err ? NULL : _err;
-dc->unrealize(dev, local_errp);
+dc->unrealize(dev, local_err ? NULL : _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