Re: [Qemu-arm] [Qemu-devel] [PATCH 1/5] xilinx: Fix error handling

[Markus Armbruster]

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?

With something like that:
Reviewed-by: Markus Armbruster <address@hidden>