Re: [PATCH 3/3] xen-block: Use one Error * variable instead of two

[Vladimir Sementsov-Ogievskiy]

17.03.2020 22:03, Markus Armbruster wrote:

Uh, I failed to actually send this.  It's in my pull request now.

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

13.03.2020 20:05, Markus Armbruster wrote:

Signed-off-by: Markus Armbruster <arm...@redhat.com>
---
   hw/block/xen-block.c | 5 +----
   1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 3885464513..7b3b6dee97 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -998,14 +998,13 @@ static void xen_block_device_destroy(XenBackendInstance 
*backend,
       XenBlockVdev *vdev = &blockdev->props.vdev;
       XenBlockDrive *drive = blockdev->drive;
       XenBlockIOThread *iothread = blockdev->iothread;
+    Error *local_err = NULL;
         trace_xen_block_device_destroy(vdev->number);
         object_unparent(OBJECT(xendev));
         if (iothread) {
-        Error *local_err = NULL;
-
           xen_block_iothread_destroy(iothread, &local_err);
           if (local_err) {
               error_propagate_prepend(errp, local_err,
@@ -1015,8 +1014,6 @@ static void xen_block_device_destroy(XenBackendInstance 
*backend,
       }
         if (drive) {
-        Error *local_err = NULL;
-
           xen_block_drive_destroy(drive, &local_err);
           if (local_err) {
               error_propagate_prepend(errp, local_err,

Hmm, no "return;" statement after this propagation. It's OK, as there no more code in the 
function after this "if", but I'd add it to be consistent and to avoid forgetting to add 
a return here when add more code to the function.

(and if you do this, you may also fix indentation of string paramter of 
error_propagate_prepend...)



Anyway,
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>

Like this, I guess:

xen-block: Use one Error * variable instead of two

While there, tidy up indentation, and add return just for consistency
and robustness.

Signed-off-by: Markus Armbruster <arm...@redhat.com>
Message-Id: <20200313170517.22480-4-arm...@redhat.com>
Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
Reviewed-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>

diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 3885464513..07bb32e22b 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -998,29 +998,27 @@ static void xen_block_device_destroy(XenBackendInstance 
*backend,
      XenBlockVdev *vdev = &blockdev->props.vdev;
      XenBlockDrive *drive = blockdev->drive;
      XenBlockIOThread *iothread = blockdev->iothread;
+    Error *local_err = NULL;

trace_xen_block_device_destroy(vdev->number); object_unparent(OBJECT(xendev)); if (iothread) {

-        Error *local_err = NULL;
-
          xen_block_iothread_destroy(iothread, &local_err);
          if (local_err) {
              error_propagate_prepend(errp, local_err,
-                                "failed to destroy iothread: ");
+                                    "failed to destroy iothread: ");
              return;
          }
      }

if (drive) {

-        Error *local_err = NULL;
-
          xen_block_drive_destroy(drive, &local_err);
          if (local_err) {
              error_propagate_prepend(errp, local_err,
-                                "failed to destroy drive: ");
+                                    "failed to destroy drive: ");
+            return;
          }
      }
  }

Yes, that's what I meant, thanks!

--
Best regards,
Vladimir