1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH 0/5] Error handling cleanup and fixes
  4. View patch

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

Eduardo Habkost posted 5 patches 8 years, 5 months ago
[Qemu-devel] [PATCH 1/5] xilinx: Fix error handling
Posted by Eduardo Habkost 8 years, 5 months ago 
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.iglesias@gmail.com>
Cc: Alistair Francis <alistair.francis@xilinx.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: qemu-arm@nongnu.org
Signed-off-by: Eduardo Habkost <ehabkost@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)
-- 
2.11.0.259.g40922b1
Re: [Qemu-devel] [PATCH 1/5] xilinx: Fix error handling
Posted by Markus Armbruster 8 years, 4 months ago 
Eduardo Habkost <ehabkost@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.iglesias@gmail.com>
> Cc: Alistair Francis <alistair.francis@xilinx.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Eduardo Habkost <ehabkost@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?

With something like that:
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Re: [Qemu-devel] [PATCH 1/5] xilinx: Fix error handling
Posted by Eduardo Habkost 8 years, 4 months ago 
On Wed, Jul 05, 2017 at 01:44:35PM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@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.iglesias@gmail.com>
> > Cc: Alistair Francis <alistair.francis@xilinx.com>
> > Cc: Jason Wang <jasowang@redhat.com>
> > Cc: qemu-arm@nongnu.org
> > Signed-off-by: Eduardo Habkost <ehabkost@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?

> 
> With something like that:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks!

-- 
Eduardo
Re: [Qemu-devel] [PATCH 1/5] xilinx: Fix error handling
Posted by Markus Armbruster 8 years, 4 months ago 
Eduardo Habkost <ehabkost@redhat.com> writes:

> On Wed, Jul 05, 2017 at 01:44:35PM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@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.iglesias@gmail.com>
>> > Cc: Alistair Francis <alistair.francis@xilinx.com>
>> > Cc: Jason Wang <jasowang@redhat.com>
>> > Cc: qemu-arm@nongnu.org
>> > Signed-off-by: Eduardo Habkost <ehabkost@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 <armbru@redhat.com>
>
> Thanks!
Re: [Qemu-devel] [PATCH 1/5] xilinx: Fix error handling
Posted by Alistair Francis 8 years, 4 months ago 
On Thu, Jun 8, 2017 at 6:39 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> 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.iglesias@gmail.com>
> Cc: Alistair Francis <alistair.francis@xilinx.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,
Alistair

> ---
>  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)
> --
> 2.11.0.259.g40922b1
>
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.