[Qemu-devel] [PATCH 0/5] Error handling cleanup and fixes

[Qemu-devel] [PATCH 0/5] Error handling cleanup and fixes

[Eduardo Habkost]

This series fixes and simplifies some of the existing code that
assigns to *errp directly.

Eduardo Habkost (5):
  xilinx: Fix error handling
  block: Don't try to set *errp directly
  websock: Don't try to set *errp directly
  migration: Don't try to set *errp directly
  vnc: No need for Error** parameter at vnc_client_io_error()

 ui/vnc.h                |  2 +-
 block.c                 |  8 +++-----
 hw/dma/xilinx_axidma.c  |  4 +---
 hw/net/xilinx_axienet.c |  4 +---
 io/channel-websock.c    |  4 ++--
 migration/migration.c   |  2 +-
 ui/vnc.c                | 13 +++++--------
 7 files changed, 14 insertions(+), 23 deletions(-)

-- 
2.11.0.259.g40922b1

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

[Eduardo Habkost]

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

[Alistair Francis]

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
>
>

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

[Markus Armbruster]

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

[Eduardo Habkost]

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

[Markus Armbruster]

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!

[Qemu-devel] [PATCH 2/5] block: Don't try to set *errp directly

[Eduardo Habkost]

Assigning directly to *errp is not valid, as errp may be NULL,
&error_fatal, or &error_abort.  Use error_propagate() instead.

With this, there's no need to check if errp is NULL anymore, as
error_propagate() and error_prepend() are able to handle that.

Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 block.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index fa1d06d846..1750a1838e 100644
--- a/block.c
+++ b/block.c
@@ -4263,11 +4263,9 @@ bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
     assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
     if (!QLIST_EMPTY(&bs->op_blockers[op])) {
         blocker = QLIST_FIRST(&bs->op_blockers[op]);
-        if (errp) {
-            *errp = error_copy(blocker->reason);
-            error_prepend(errp, "Node '%s' is busy: ",
-                          bdrv_get_device_or_node_name(bs));
-        }
+        error_propagate(errp, error_copy(blocker->reason));
+        error_prepend(errp, "Node '%s' is busy: ",
+                      bdrv_get_device_or_node_name(bs));
         return true;
     }
     return false;
-- 
2.11.0.259.g40922b1

Re: [Qemu-devel] [Qemu-block] [PATCH 2/5] block: Don't try to set *errp directly

[Alberto Garcia]

On Thu 08 Jun 2017 03:39:03 PM CEST, Eduardo Habkost wrote:
> Assigning directly to *errp is not valid, as errp may be NULL,
> &error_fatal, or &error_abort.  Use error_propagate() instead.

Well, the NULL case was already handled by the code, but &error_fatal
and &error_abort certainly not.

> With this, there's no need to check if errp is NULL anymore, as
> error_propagate() and error_prepend() are able to handle that.
>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: qemu-block@nongnu.org
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH 2/5] block: Don't try to set *errp directly

[Markus Armbruster]

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.
>
> With this, there's no need to check if errp is NULL anymore, as
> error_propagate() and error_prepend() are able to handle that.
>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: qemu-block@nongnu.org
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  block.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/block.c b/block.c
> index fa1d06d846..1750a1838e 100644
> --- a/block.c
> +++ b/block.c
> @@ -4263,11 +4263,9 @@ bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
>      assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
>      if (!QLIST_EMPTY(&bs->op_blockers[op])) {
>          blocker = QLIST_FIRST(&bs->op_blockers[op]);
> -        if (errp) {
> -            *errp = error_copy(blocker->reason);
> -            error_prepend(errp, "Node '%s' is busy: ",
> -                          bdrv_get_device_or_node_name(bs));
> -        }
> +        error_propagate(errp, error_copy(blocker->reason));
> +        error_prepend(errp, "Node '%s' is busy: ",
> +                      bdrv_get_device_or_node_name(bs));
>          return true;
>      }
>      return false;

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH 3/5] websock: Don't try to set *errp directly

[Eduardo Habkost]

Assigning directly to *errp is not valid, as errp may be NULL,
&error_fatal, or &error_abort.  Use error_propagate() instead.

Cc: "Daniel P. Berrange" <berrange@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 io/channel-websock.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/io/channel-websock.c b/io/channel-websock.c
index 8fabadea2f..5a3badbec2 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -856,7 +856,7 @@ static ssize_t qio_channel_websock_readv(QIOChannel *ioc,
     ssize_t ret;
 
     if (wioc->io_err) {
-        *errp = error_copy(wioc->io_err);
+        error_propagate(errp, error_copy(wioc->io_err));
         return -1;
     }
 
@@ -902,7 +902,7 @@ static ssize_t qio_channel_websock_writev(QIOChannel *ioc,
     ssize_t ret;
 
     if (wioc->io_err) {
-        *errp = error_copy(wioc->io_err);
+        error_propagate(errp, error_copy(wioc->io_err));
         return -1;
     }
 
-- 
2.11.0.259.g40922b1

Re: [Qemu-devel] [PATCH 3/5] websock: Don't try to set *errp directly

[Manos Pitsidianakis]

[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]

On Thu, Jun 08, 2017 at 10:39:04AM -0300, Eduardo Habkost wrote:
>Assigning directly to *errp is not valid, as errp may be NULL,
>&error_fatal, or &error_abort.  Use error_propagate() instead.
>
>Cc: "Daniel P. Berrange" <berrange@redhat.com>
>Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>---
> io/channel-websock.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/io/channel-websock.c b/io/channel-websock.c
>index 8fabadea2f..5a3badbec2 100644
>--- a/io/channel-websock.c
>+++ b/io/channel-websock.c
>@@ -856,7 +856,7 @@ static ssize_t qio_channel_websock_readv(QIOChannel *ioc,
>     ssize_t ret;
>
>     if (wioc->io_err) {
>-        *errp = error_copy(wioc->io_err);
>+        error_propagate(errp, error_copy(wioc->io_err));
>         return -1;
>     }
>
>@@ -902,7 +902,7 @@ static ssize_t qio_channel_websock_writev(QIOChannel *ioc,
>     ssize_t ret;
>
>     if (wioc->io_err) {
>-        *errp = error_copy(wioc->io_err);
>+        error_propagate(errp, error_copy(wioc->io_err));
>         return -1;
>     }
>
>-- 
>2.11.0.259.g40922b1
>
>
>

Reviewed-by: Manos Pitsidianakis <el13635@mail.ntua.gr>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 3/5] websock: Don't try to set *errp directly

[Markus Armbruster]

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.
>
> Cc: "Daniel P. Berrange" <berrange@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  io/channel-websock.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/io/channel-websock.c b/io/channel-websock.c
> index 8fabadea2f..5a3badbec2 100644
> --- a/io/channel-websock.c
> +++ b/io/channel-websock.c
> @@ -856,7 +856,7 @@ static ssize_t qio_channel_websock_readv(QIOChannel *ioc,
>      ssize_t ret;
>  
>      if (wioc->io_err) {
> -        *errp = error_copy(wioc->io_err);
> +        error_propagate(errp, error_copy(wioc->io_err));
>          return -1;
>      }
>  
> @@ -902,7 +902,7 @@ static ssize_t qio_channel_websock_writev(QIOChannel *ioc,
>      ssize_t ret;
>  
>      if (wioc->io_err) {
> -        *errp = error_copy(wioc->io_err);
> +        error_propagate(errp, error_copy(wioc->io_err));
>          return -1;
>      }

The error_copy() is wasted when !errp.  Good enough for an error path.
Same for PATCH 2.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH 4/5] migration: Don't try to set *errp directly

[Eduardo Habkost]

Assigning directly to *errp is not valid, as errp may be NULL,
&error_fatal, or &error_abort.  Use error_propagate() instead.

Cc: Juan Quintela <quintela@redhat.com>
Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 migration/migration.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 48c94c9ca1..5a8bef3ef5 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1188,7 +1188,7 @@ bool migration_is_blocked(Error **errp)
     }
 
     if (migration_blockers) {
-        *errp = error_copy(migration_blockers->data);
+        error_propagate(errp, error_copy(migration_blockers->data));
         return true;
     }
 
-- 
2.11.0.259.g40922b1

Re: [Qemu-devel] [PATCH 4/5] migration: Don't try to set *errp directly

[Juan Quintela]

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.
>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>


Reviewed-by: Juan Quintela <quintela@redhat.com>

queued

Thanks.

Re: [Qemu-devel] [PATCH 4/5] migration: Don't try to set *errp directly

[Dr. David Alan Gilbert]

* 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.
> 
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  migration/migration.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 48c94c9ca1..5a8bef3ef5 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1188,7 +1188,7 @@ bool migration_is_blocked(Error **errp)
>      }
>  
>      if (migration_blockers) {
> -        *errp = error_copy(migration_blockers->data);
> +        error_propagate(errp, error_copy(migration_blockers->data));
>          return true;
>      }

if errp is NULL doesn't that leak the error_copy ?

Dave

>  
> -- 
> 2.11.0.259.g40922b1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 4/5] migration: Don't try to set *errp directly

[Dr. David Alan Gilbert]

* Dr. David Alan Gilbert (dgilbert@redhat.com) wrote:
> * 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.
> > 
> > Cc: Juan Quintela <quintela@redhat.com>
> > Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  migration/migration.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 48c94c9ca1..5a8bef3ef5 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -1188,7 +1188,7 @@ bool migration_is_blocked(Error **errp)
> >      }
> >  
> >      if (migration_blockers) {
> > -        *errp = error_copy(migration_blockers->data);
> > +        error_propagate(errp, error_copy(migration_blockers->data));
> >          return true;
> >      }
> 
> if errp is NULL doesn't that leak the error_copy ?

To answer myself, no it doesn't - error_propagate free's it in that
case.

Dave

> Dave
> 
> >  
> > -- 
> > 2.11.0.259.g40922b1
> > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 5/5] vnc: No need for Error** parameter at vnc_client_io_error()

[Eduardo Habkost]

The only callers of vnc_client_io_error() with non-NULL errp don't use
*errp after the function gets called, so there's no need to use an
Error** argument.  Change parameter to Error* and simplify the code.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 ui/vnc.h |  2 +-
 ui/vnc.c | 13 +++++--------
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/ui/vnc.h b/ui/vnc.h
index 694cf32ca9..52f65bbd86 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -526,7 +526,7 @@ uint32_t read_u32(uint8_t *data, size_t offset);
 
 /* Protocol stage functions */
 void vnc_client_error(VncState *vs);
-ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp);
+ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error *err);
 
 void start_client_init(VncState *vs);
 void start_auth_vnc(VncState *vs);
diff --git a/ui/vnc.c b/ui/vnc.c
index 47b49c7318..51f13f0c29 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1180,7 +1180,7 @@ void vnc_disconnect_finish(VncState *vs)
     g_free(vs);
 }
 
-ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
+ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error *err)
 {
     if (ret <= 0) {
         if (ret == 0) {
@@ -1188,14 +1188,11 @@ ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
             vnc_disconnect_start(vs);
         } else if (ret != QIO_CHANNEL_ERR_BLOCK) {
             VNC_DEBUG("Closing down client sock: ret %zd (%s)\n",
-                      ret, errp ? error_get_pretty(*errp) : "Unknown");
+                      ret, err ? error_get_pretty(err) : "Unknown");
             vnc_disconnect_start(vs);
         }
 
-        if (errp) {
-            error_free(*errp);
-            *errp = NULL;
-        }
+        error_free(err);
         return 0;
     }
     return ret;
@@ -1231,7 +1228,7 @@ ssize_t vnc_client_write_buf(VncState *vs, const uint8_t *data, size_t datalen)
     ret = qio_channel_write(
         vs->ioc, (const char *)data, datalen, &err);
     VNC_DEBUG("Wrote wire %p %zd -> %ld\n", data, datalen, ret);
-    return vnc_client_io_error(vs, ret, &err);
+    return vnc_client_io_error(vs, ret, err);
 }
 
 
@@ -1344,7 +1341,7 @@ ssize_t vnc_client_read_buf(VncState *vs, uint8_t *data, size_t datalen)
     ret = qio_channel_read(
         vs->ioc, (char *)data, datalen, &err);
     VNC_DEBUG("Read wire %p %zd -> %ld\n", data, datalen, ret);
-    return vnc_client_io_error(vs, ret, &err);
+    return vnc_client_io_error(vs, ret, err);
 }
 
 
-- 
2.11.0.259.g40922b1

Re: [Qemu-devel] [PATCH 5/5] vnc: No need for Error** parameter at vnc_client_io_error()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2150 bytes --]

On 06/08/2017 08:39 AM, Eduardo Habkost wrote:
> The only callers of vnc_client_io_error() with non-NULL errp don't use
> *errp after the function gets called, so there's no need to use an
> Error** argument.  Change parameter to Error* and simplify the code.
> 
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  ui/vnc.h |  2 +-
>  ui/vnc.c | 13 +++++--------
>  2 files changed, 6 insertions(+), 9 deletions(-)

>  
> -ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
> +ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error *err)
>  {

This is unusual.

>      if (ret <= 0) {
>          if (ret == 0) {
> @@ -1188,14 +1188,11 @@ ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
>              vnc_disconnect_start(vs);
>          } else if (ret != QIO_CHANNEL_ERR_BLOCK) {
>              VNC_DEBUG("Closing down client sock: ret %zd (%s)\n",
> -                      ret, errp ? error_get_pretty(*errp) : "Unknown");
> +                      ret, err ? error_get_pretty(err) : "Unknown");
>              vnc_disconnect_start(vs);
>          }
>  
> -        if (errp) {
> -            error_free(*errp);
> -            *errp = NULL;
> -        }
> +        error_free(err);

Worse, it's action at a distance. You are freeing memory here that was
allocated in the callers.

>          return 0;
>      }
>      return ret;
> @@ -1231,7 +1228,7 @@ ssize_t vnc_client_write_buf(VncState *vs, const uint8_t *data, size_t datalen)
>      ret = qio_channel_write(
>          vs->ioc, (const char *)data, datalen, &err);
>      VNC_DEBUG("Wrote wire %p %zd -> %ld\n", data, datalen, ret);
> -    return vnc_client_io_error(vs, ret, &err);
> +    return vnc_client_io_error(vs, ret, err);
>  }

It looks like the only reason that err gets passed is for the sake of
VNC_DEBUG messages, but I'm not sure I like this patch.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 5/5] vnc: No need for Error** parameter at vnc_client_io_error()

[Eduardo Habkost]

On Thu, Jun 08, 2017 at 10:17:45AM -0500, Eric Blake wrote:
> On 06/08/2017 08:39 AM, Eduardo Habkost wrote:
> > The only callers of vnc_client_io_error() with non-NULL errp don't use
> > *errp after the function gets called, so there's no need to use an
> > Error** argument.  Change parameter to Error* and simplify the code.
> > 
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Daniel P. Berrange <berrange@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  ui/vnc.h |  2 +-
> >  ui/vnc.c | 13 +++++--------
> >  2 files changed, 6 insertions(+), 9 deletions(-)
> 
> >  
> > -ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
> > +ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error *err)
> >  {
> 
> This is unusual.

Why?  I would say that using Error** for input (and not output)
is the unusual pattern.

This is the only remaining place in the whole tree where code
assigns something to *errp outside util/error.c (and no callers
of this function rely on this feature).



> 
> >      if (ret <= 0) {
> >          if (ret == 0) {
> > @@ -1188,14 +1188,11 @@ ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
> >              vnc_disconnect_start(vs);
> >          } else if (ret != QIO_CHANNEL_ERR_BLOCK) {
> >              VNC_DEBUG("Closing down client sock: ret %zd (%s)\n",
> > -                      ret, errp ? error_get_pretty(*errp) : "Unknown");
> > +                      ret, err ? error_get_pretty(err) : "Unknown");
> >              vnc_disconnect_start(vs);
> >          }
> >  
> > -        if (errp) {
> > -            error_free(*errp);
> > -            *errp = NULL;
> > -        }
> > +        error_free(err);
> 
> Worse, it's action at a distance. You are freeing memory here that was
> allocated in the callers.

Isn't this one of the purposes of this function?

The difference here is that now the function function is just
taking ownership of err, making the interface and the
implementation simpler.  If I document this more clearly at
vnc_client_io_error()'s declaration, would it make this change
acceptable?

> 
> >          return 0;
> >      }
> >      return ret;
> > @@ -1231,7 +1228,7 @@ ssize_t vnc_client_write_buf(VncState *vs, const uint8_t *data, size_t datalen)
> >      ret = qio_channel_write(
> >          vs->ioc, (const char *)data, datalen, &err);
> >      VNC_DEBUG("Wrote wire %p %zd -> %ld\n", data, datalen, ret);
> > -    return vnc_client_io_error(vs, ret, &err);
> > +    return vnc_client_io_error(vs, ret, err);
> >  }
> 
> It looks like the only reason that err gets passed is for the sake of
> VNC_DEBUG messages, but I'm not sure I like this patch.

err is passed for two purposes:
* Debug messages;
* Calling error_free().

The function keeps doing both like before, but the difference is
that now it just takes ownership of err.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 5/5] vnc: No need for Error** parameter at vnc_client_io_error()

[Eduardo Habkost]

On Thu, Jun 08, 2017 at 01:05:16PM -0300, Eduardo Habkost wrote:
> On Thu, Jun 08, 2017 at 10:17:45AM -0500, Eric Blake wrote:
> > On 06/08/2017 08:39 AM, Eduardo Habkost wrote:
> > > The only callers of vnc_client_io_error() with non-NULL errp don't use
> > > *errp after the function gets called, so there's no need to use an
> > > Error** argument.  Change parameter to Error* and simplify the code.
> > > 
> > > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > > Cc: Daniel P. Berrange <berrange@redhat.com>
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > >  ui/vnc.h |  2 +-
> > >  ui/vnc.c | 13 +++++--------
> > >  2 files changed, 6 insertions(+), 9 deletions(-)
> > 
> > >  
> > > -ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
> > > +ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error *err)
> > >  {
> > 
> > This is unusual.
> 
> Why?  I would say that using Error** for input (and not output)
> is the unusual pattern.
> 
> This is the only remaining place in the whole tree where code
> assigns something to *errp outside util/error.c (and no callers
> of this function rely on this feature).
> 
> 
> 
> > 
> > >      if (ret <= 0) {
> > >          if (ret == 0) {
> > > @@ -1188,14 +1188,11 @@ ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
> > >              vnc_disconnect_start(vs);
> > >          } else if (ret != QIO_CHANNEL_ERR_BLOCK) {
> > >              VNC_DEBUG("Closing down client sock: ret %zd (%s)\n",
> > > -                      ret, errp ? error_get_pretty(*errp) : "Unknown");
> > > +                      ret, err ? error_get_pretty(err) : "Unknown");
> > >              vnc_disconnect_start(vs);
> > >          }
> > >  
> > > -        if (errp) {
> > > -            error_free(*errp);
> > > -            *errp = NULL;
> > > -        }
> > > +        error_free(err);
> > 
> > Worse, it's action at a distance. You are freeing memory here that was
> > allocated in the callers.
> 
> Isn't this one of the purposes of this function?
> 
> The difference here is that now the function function is just
> taking ownership of err, making the interface and the
> implementation simpler.  If I document this more clearly at
> vnc_client_io_error()'s declaration, would it make this change
> acceptable?

What about the following?

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 ui/vnc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/ui/vnc.c b/ui/vnc.c
index 51f13f0c29..cb55554210 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1180,6 +1180,15 @@ void vnc_disconnect_finish(VncState *vs)
     g_free(vs);
 }
 
+
+/*
+ * Handle I/O error (@ret < 0) or EOF (@ret == 0) from I/O
+ * channel.  In case of errors or EOF, @err is freed using
+ * error_free().
+ *
+ * Returns 0 in case @ret <= 0 and the error was properly
+ * handled, otherwise returns @ret.
+ */
 ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error *err)
 {
     if (ret <= 0) {
-- 
2.11.0.259.g40922b1

-- 
Eduardo

Re: [Qemu-devel] [PATCH 5/5] vnc: No need for Error** parameter at vnc_client_io_error()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1883 bytes --]

On 06/08/2017 12:44 PM, Eduardo Habkost wrote:

>>>> -ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
>>>> +ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error *err)
>>>>  {
>>>
>>> This is unusual.
>>
>> Why?  I would say that using Error** for input (and not output)
>> is the unusual pattern.

Yes, and when you frame it that way, it makes sense. But with no comment
framing it that way, ...

>> Isn't this one of the purposes of this function?
>>
>> The difference here is that now the function function is just
>> taking ownership of err, making the interface and the
>> implementation simpler.  If I document this more clearly at
>> vnc_client_io_error()'s declaration, would it make this change
>> acceptable?

... why yes, you've hit on why I felt uneasy - we are missing good
documentation!

> 
> What about the following?
> 

Yes, that makes it MUCH easier to understand what's going on.

With this squashed in,
Reviewed-by: Eric Blake <eblake@redhat.com>

> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  ui/vnc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 51f13f0c29..cb55554210 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -1180,6 +1180,15 @@ void vnc_disconnect_finish(VncState *vs)
>      g_free(vs);
>  }
>  
> +
> +/*
> + * Handle I/O error (@ret < 0) or EOF (@ret == 0) from I/O
> + * channel.  In case of errors or EOF, @err is freed using
> + * error_free().
> + *
> + * Returns 0 in case @ret <= 0 and the error was properly
> + * handled, otherwise returns @ret.
> + */
>  ssize_t vnc_client_io_error(VncState *vs, ssize_t ret, Error *err)
>  {
>      if (ret <= 0) {
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 0/5] Error handling cleanup and fixes

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 961 bytes --]

On 06/08/2017 08:39 AM, Eduardo Habkost wrote:
> This series fixes and simplifies some of the existing code that
> assigns to *errp directly.
> 
> Eduardo Habkost (5):
>   xilinx: Fix error handling
>   block: Don't try to set *errp directly
>   websock: Don't try to set *errp directly
>   migration: Don't try to set *errp directly
>   vnc: No need for Error** parameter at vnc_client_io_error()

For 1-4:
Reviewed-by: Eric Blake <eblake@redhat.com>

I'm not sure about 5

> 
>  ui/vnc.h                |  2 +-
>  block.c                 |  8 +++-----
>  hw/dma/xilinx_axidma.c  |  4 +---
>  hw/net/xilinx_axienet.c |  4 +---
>  io/channel-websock.c    |  4 ++--
>  migration/migration.c   |  2 +-
>  ui/vnc.c                | 13 +++++--------
>  7 files changed, 14 insertions(+), 23 deletions(-)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 0/5] Error handling cleanup and fixes

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> This series fixes and simplifies some of the existing code that
> assigns to *errp directly.
>
> Eduardo Habkost (5):
>   xilinx: Fix error handling
>   block: Don't try to set *errp directly
>   websock: Don't try to set *errp directly
>   migration: Don't try to set *errp directly
>   vnc: No need for Error** parameter at vnc_client_io_error()

I'm willing to take PATCH 1-3 through my tree.  PATCH 4 has been merged
already.  I'd like to leave PATCH 5 to Gerd.

Re: [Qemu-devel] [PATCH 0/5] Error handling cleanup and fixes

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> This series fixes and simplifies some of the existing code that
> assigns to *errp directly.

PATCH 1-3 applied to error-next, thanks!