[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [Qemu-devel] [PATCH 1/5] xilinx: Fix error handling
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-arm] [Qemu-devel] [PATCH 1/5] xilinx: Fix error handling |
Date: |
Thu, 6 Jul 2017 11:08:37 -0300 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
On Wed, Jul 05, 2017 at 01:44:35PM +0200, Markus Armbruster wrote:
> Eduardo Habkost <address@hidden> writes:
>
> > Assigning directly to *errp is not valid, as errp may be NULL,
> > &error_fatal, or &error_abort. Use error_propagate() instead.
> >
> > error_propagate() handles non-NULL *errp correctly, so the
> > "if (!*errp)" check can be removed.
> >
> > Cc: "Edgar E. Iglesias" <address@hidden>
> > Cc: Alistair Francis <address@hidden>
> > Cc: Jason Wang <address@hidden>
> > Cc: address@hidden
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> > hw/dma/xilinx_axidma.c | 4 +---
> > hw/net/xilinx_axienet.c | 4 +---
> > 2 files changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> > index 6065689ad1..3987b5ff96 100644
> > --- a/hw/dma/xilinx_axidma.c
> > +++ b/hw/dma/xilinx_axidma.c
> > @@ -554,9 +554,7 @@ static void xilinx_axidma_realize(DeviceState *dev,
> > Error **errp)
> > return;
> >
> > xilinx_axidma_realize_fail:
> > - if (!*errp) {
> > - *errp = local_err;
> > - }
> > + error_propagate(errp, local_err);
> > }
> >
> > static void xilinx_axidma_init(Object *obj)
> > diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
> > index b6701844d3..5ffa739f68 100644
> > --- a/hw/net/xilinx_axienet.c
> > +++ b/hw/net/xilinx_axienet.c
> > @@ -981,9 +981,7 @@ static void xilinx_enet_realize(DeviceState *dev, Error
> > **errp)
> > return;
> >
> > xilinx_enet_realize_fail:
> > - if (!*errp) {
> > - *errp = local_err;
> > - }
> > + error_propagate(errp, local_err);
> > }
> >
> > static void xilinx_enet_init(Object *obj)
>
> The qdev core always passes &err. So this is "merely" a fix for a
> latent bug.
>
> If it could pass null, you'd fix a crash bug.
>
> If it could pass pointer to non-null (&error_fatal, &error_abort), you'd
> plug a memory leak.
>
> Suggest to rephrase the commit message:
>
> xilinx: Fix latent error handling bug
>
> Assigning directly to *errp is not valid, as errp may be null,
> &error_fatal, or &error_abort. The !*errp conditional protects
> against the latter two, but we then leak @local_err. Fortunately,
> the qdev core always passes pointer to null, so this is "merely" a
> latent bug.
>
> Use error_propagate() instead.
>
> What do you think?
Looks good to me. Do you want to fix it when applying?
>
> With something like that:
> Reviewed-by: Markus Armbruster <address@hidden>
Thanks!
--
Eduardo