Eduardo Habkost <ehabk...@redhat.com> writes:
> On Wed, Jul 05, 2017 at 01:44:35PM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <ehabk...@redhat.com> 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" <edgar.igles...@gmail.com>
>> > Cc: Alistair Francis <alistair.fran...@xilinx.com>
>> > Cc: Jason Wang <jasow...@redhat.com>
>> > Cc: qemu-...@nongnu.org
>> > Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
>> > ---
>> > 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?
Can do!
>> With something like that:
>> Reviewed-by: Markus Armbruster <arm...@redhat.com>
>
> Thanks!
