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 <address@hidden> writes:
>
> > 13.03.2020 20:05, Markus Armbruster wrote:
> > > Signed-off-by: Markus Armbruster <address@hidden>
> > > ---
> > >    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 <address@hidden>
>
> 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 <address@hidden>
> Message-Id: <address@hidden>
> Reviewed-by: Peter Maydell <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>
> 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