[PATCH 00/11] More miscellaneous error handling fixes

[PATCH 00/11] More miscellaneous error handling fixes

[Markus Armbruster]

Markus Armbruster (11):
  nvdimm: Plug memory leak in uuid property setter
  xen: Fix and improve handling of device_add usb-host errors
  s390x/cpumodel: Fix harmless misuse of visit_check_struct()
  s390x/pci: Fix harmless mistake in zpci's property fid's setter
  tests/migration: Tighten error checking
  error: Use error_reportf_err() where appropriate
  mips/malta: Fix create_cps() error handling
  mips/boston: Fix boston_mach_init() error handling
  mips/boston: Plug memory leak in boston_mach_init()
  arm/sabrelite: Consistently use &error_fatal in sabrelite_init()
  i386: Fix x86_cpu_load_model() error API violation

 chardev/char-socket.c        |  5 +++--
 hw/arm/sabrelite.c           |  7 +------
 hw/mem/nvdimm.c              |  1 -
 hw/mips/boston.c             | 17 +++++++----------
 hw/mips/mips_malta.c         | 15 ++++++---------
 hw/s390x/s390-pci-bus.c      |  4 +++-
 hw/sd/pxa2xx_mmci.c          |  4 ++--
 hw/sd/sd.c                   |  4 ++--
 hw/usb/dev-mtp.c             |  9 +++++----
 hw/usb/xen-usb.c             | 18 ++++++++----------
 qemu-nbd.c                   |  7 +++----
 scsi/qemu-pr-helper.c        |  4 ++--
 target/i386/cpu.c            | 25 ++++++++++++++++---------
 target/s390x/cpu_models.c    |  2 +-
 tests/qtest/migration-test.c |  4 ++--
 15 files changed, 61 insertions(+), 65 deletions(-)

-- 
2.21.1

[PATCH 01/11] nvdimm: Plug memory leak in uuid property setter

[Markus Armbruster]

nvdimm_set_uuid() leaks memory on qemu_uuid_parse() failure.  Fix
that.

Fixes: 6c5627bb24dcd68c997857a8b671617333b1289f
Cc: Xiao Guangrong <xiaoguangrong.eric@gmail.com>
Cc: Shivaprasad G Bhat <sbhat@linux.ibm.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/mem/nvdimm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 8e426d24bb..d5752f7bf6 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -97,7 +97,6 @@ static void nvdimm_set_uuid(Object *obj, Visitor *v, const char *name,
     if (qemu_uuid_parse(value, &nvdimm->uuid) != 0) {
         error_setg(errp, "Property '%s.%s' has invalid value",
                    object_get_typename(obj), name);
-        goto out;
     }
     g_free(value);
 
-- 
2.21.1

[PATCH 02/11] xen: Fix and improve handling of device_add usb-host errors

[Markus Armbruster]

usbback_portid_add() leaks the error when qdev_device_add() fails.
Fix that.  While there, use the error to improve the error message.

The qemu_opts_from_qdict() similarly leaks on failure.  But any
failure there is a programming error.  Pass &error_abort.

Fixes: 816ac92ef769f9ffc534e49a1bb6177bddce7aa2
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Paul Durrant <paul@xen.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: xen-devel@lists.xenproject.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/usb/xen-usb.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
index 961190d0f7..42643c3390 100644
--- a/hw/usb/xen-usb.c
+++ b/hw/usb/xen-usb.c
@@ -30,6 +30,7 @@
 #include "hw/usb.h"
 #include "hw/xen/xen-legacy-backend.h"
 #include "monitor/qdev.h"
+#include "qapi/error.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qstring.h"
 
@@ -755,13 +756,15 @@ static void usbback_portid_add(struct usbback_info *usbif, unsigned port,
     qdict_put_int(qdict, "port", port);
     qdict_put_int(qdict, "hostbus", atoi(busid));
     qdict_put_str(qdict, "hostport", portname);
-    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
-    if (local_err) {
-        goto err;
-    }
+    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict,
+                                &error_abort);
     usbif->ports[port - 1].dev = USB_DEVICE(qdev_device_add(opts, &local_err));
     if (!usbif->ports[port - 1].dev) {
-        goto err;
+        qobject_unref(qdict);
+        xen_pv_printf(&usbif->xendev, 0,
+                      "device %s could not be opened: %s\n",
+                      busid, error_get_pretty(local_err));
+        error_free(local_err);
     }
     qobject_unref(qdict);
     speed = usbif->ports[port - 1].dev->speed;
@@ -793,11 +796,6 @@ static void usbback_portid_add(struct usbback_info *usbif, unsigned port,
     usbback_hotplug_enq(usbif, port);
 
     TR_BUS(&usbif->xendev, "port %d attached\n", port);
-    return;
-
-err:
-    qobject_unref(qdict);
-    xen_pv_printf(&usbif->xendev, 0, "device %s could not be opened\n", busid);
 }
 
 static void usbback_process_port(struct usbback_info *usbif, unsigned port)
-- 
2.21.1

RE: [PATCH 02/11] xen: Fix and improve handling of device_add usb-host errors

[Paul Durrant]

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: 24 April 2020 20:20
> To: qemu-devel@nongnu.org
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul
> Durrant <paul@xen.org>; Gerd Hoffmann <kraxel@redhat.com>; xen-devel@lists.xenproject.org
> Subject: [PATCH 02/11] xen: Fix and improve handling of device_add usb-host errors
> 
> usbback_portid_add() leaks the error when qdev_device_add() fails.
> Fix that.  While there, use the error to improve the error message.
> 
> The qemu_opts_from_qdict() similarly leaks on failure.  But any
> failure there is a programming error.  Pass &error_abort.
> 
> Fixes: 816ac92ef769f9ffc534e49a1bb6177bddce7aa2
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Paul Durrant <paul@xen.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: xen-devel@lists.xenproject.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/usb/xen-usb.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
> index 961190d0f7..42643c3390 100644
> --- a/hw/usb/xen-usb.c
> +++ b/hw/usb/xen-usb.c
> @@ -30,6 +30,7 @@
>  #include "hw/usb.h"
>  #include "hw/xen/xen-legacy-backend.h"
>  #include "monitor/qdev.h"
> +#include "qapi/error.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qstring.h"
> 
> @@ -755,13 +756,15 @@ static void usbback_portid_add(struct usbback_info *usbif, unsigned port,
>      qdict_put_int(qdict, "port", port);
>      qdict_put_int(qdict, "hostbus", atoi(busid));
>      qdict_put_str(qdict, "hostport", portname);
> -    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
> -    if (local_err) {
> -        goto err;
> -    }
> +    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict,
> +                                &error_abort);
>      usbif->ports[port - 1].dev = USB_DEVICE(qdev_device_add(opts, &local_err));
>      if (!usbif->ports[port - 1].dev) {
> -        goto err;
> +        qobject_unref(qdict);
> +        xen_pv_printf(&usbif->xendev, 0,
> +                      "device %s could not be opened: %s\n",
> +                      busid, error_get_pretty(local_err));
> +        error_free(local_err);

Previously the goto caused the function to bail out. Should there not be a 'return' here?

>      }
>      qobject_unref(qdict);
>      speed = usbif->ports[port - 1].dev->speed;
> @@ -793,11 +796,6 @@ static void usbback_portid_add(struct usbback_info *usbif, unsigned port,
>      usbback_hotplug_enq(usbif, port);
> 
>      TR_BUS(&usbif->xendev, "port %d attached\n", port);
> -    return;
> -
> -err:
> -    qobject_unref(qdict);
> -    xen_pv_printf(&usbif->xendev, 0, "device %s could not be opened\n", busid);
>  }
> 
>  static void usbback_process_port(struct usbback_info *usbif, unsigned port)
> --
> 2.21.1

Re: [PATCH 02/11] xen: Fix and improve handling of device_add usb-host errors

[Markus Armbruster]

Paul Durrant <xadimgnik@gmail.com> writes:

>> -----Original Message-----
>> From: Markus Armbruster <armbru@redhat.com>
>> Sent: 24 April 2020 20:20
>> To: qemu-devel@nongnu.org
>> Cc: Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul
>> Durrant <paul@xen.org>; Gerd Hoffmann <kraxel@redhat.com>; xen-devel@lists.xenproject.org
>> Subject: [PATCH 02/11] xen: Fix and improve handling of device_add usb-host errors
>> 
>> usbback_portid_add() leaks the error when qdev_device_add() fails.
>> Fix that.  While there, use the error to improve the error message.
>> 
>> The qemu_opts_from_qdict() similarly leaks on failure.  But any
>> failure there is a programming error.  Pass &error_abort.
>> 
>> Fixes: 816ac92ef769f9ffc534e49a1bb6177bddce7aa2
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Cc: Paul Durrant <paul@xen.org>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: xen-devel@lists.xenproject.org
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/usb/xen-usb.c | 18 ++++++++----------
>>  1 file changed, 8 insertions(+), 10 deletions(-)
>> 
>> diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
>> index 961190d0f7..42643c3390 100644
>> --- a/hw/usb/xen-usb.c
>> +++ b/hw/usb/xen-usb.c
>> @@ -30,6 +30,7 @@
>>  #include "hw/usb.h"
>>  #include "hw/xen/xen-legacy-backend.h"
>>  #include "monitor/qdev.h"
>> +#include "qapi/error.h"
>>  #include "qapi/qmp/qdict.h"
>>  #include "qapi/qmp/qstring.h"
>> 
>> @@ -755,13 +756,15 @@ static void usbback_portid_add(struct usbback_info *usbif, unsigned port,
>>      qdict_put_int(qdict, "port", port);
>>      qdict_put_int(qdict, "hostbus", atoi(busid));
>>      qdict_put_str(qdict, "hostport", portname);
>> -    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
>> -    if (local_err) {
>> -        goto err;
>> -    }
>> +    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict,
>> +                                &error_abort);
>>      usbif->ports[port - 1].dev = USB_DEVICE(qdev_device_add(opts, &local_err));
>>      if (!usbif->ports[port - 1].dev) {
>> -        goto err;
>> +        qobject_unref(qdict);
>> +        xen_pv_printf(&usbif->xendev, 0,
>> +                      "device %s could not be opened: %s\n",
>> +                      busid, error_get_pretty(local_err));
>> +        error_free(local_err);
>
> Previously the goto caused the function to bail out. Should there not be a 'return' here?

Owww, of course.  Thanks!

>
>>      }
>>      qobject_unref(qdict);
>>      speed = usbif->ports[port - 1].dev->speed;
>> @@ -793,11 +796,6 @@ static void usbback_portid_add(struct usbback_info *usbif, unsigned port,
>>      usbback_hotplug_enq(usbif, port);
>> 
>>      TR_BUS(&usbif->xendev, "port %d attached\n", port);
>> -    return;
>> -
>> -err:
>> -    qobject_unref(qdict);
>> -    xen_pv_printf(&usbif->xendev, 0, "device %s could not be opened\n", busid);
>>  }
>> 
>>  static void usbback_process_port(struct usbback_info *usbif, unsigned port)
>> --
>> 2.21.1

[PATCH 03/11] s390x/cpumodel: Fix harmless misuse of visit_check_struct()

[Markus Armbruster]

Commit e47970f51d "s390x/cpumodel: Fix query-cpu-model-FOO error API
violations" neglected to change visit_end_struct()'s Error ** argument
along with the others.  If visit_end_struct() failed, we'd take the
success path.  Fortunately, it can't fail here:
qobject_input_check_struct() checks we consumed the whole dictionary,
and to get here, we did.  Fix it anyway.

Cc: David Hildenbrand <david@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 target/s390x/cpu_models.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 7c32180269..87a8028ad3 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -524,7 +524,7 @@ static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
             }
         }
         if (!err) {
-            visit_check_struct(visitor, errp);
+            visit_check_struct(visitor, &err);
         }
         visit_end_struct(visitor, NULL);
         visit_free(visitor);
-- 
2.21.1

Re: [PATCH 03/11] s390x/cpumodel: Fix harmless misuse of visit_check_struct()

[David Hildenbrand]

On 24.04.20 21:20, Markus Armbruster wrote:
> Commit e47970f51d "s390x/cpumodel: Fix query-cpu-model-FOO error API
> violations" neglected to change visit_end_struct()'s Error ** argument
> along with the others.  If visit_end_struct() failed, we'd take the

s/visit_end_struct/visit_check_struct/ ?

> success path.  Fortunately, it can't fail here:
> qobject_input_check_struct() checks we consumed the whole dictionary,
> and to get here, we did.  Fix it anyway.

AFAIKs, if visit_check_struct() failed, we'd still do the memcopy, but
also report the error. Not nice, not bad.

Reviewed-by: David Hildenbrand <david@redhat.com>

> 
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  target/s390x/cpu_models.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 7c32180269..87a8028ad3 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -524,7 +524,7 @@ static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
>              }
>          }
>          if (!err) {
> -            visit_check_struct(visitor, errp);
> +            visit_check_struct(visitor, &err);
>          }
>          visit_end_struct(visitor, NULL);
>          visit_free(visitor);
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH 03/11] s390x/cpumodel: Fix harmless misuse of visit_check_struct()

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 24.04.20 21:20, Markus Armbruster wrote:
>> Commit e47970f51d "s390x/cpumodel: Fix query-cpu-model-FOO error API
>> violations" neglected to change visit_end_struct()'s Error ** argument
>> along with the others.  If visit_end_struct() failed, we'd take the
>
> s/visit_end_struct/visit_check_struct/ ?

Will fix.

>> success path.  Fortunately, it can't fail here:
>> qobject_input_check_struct() checks we consumed the whole dictionary,
>> and to get here, we did.  Fix it anyway.
>
> AFAIKs, if visit_check_struct() failed, we'd still do the memcopy, but
> also report the error. Not nice, not bad.
>
> Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks!

Re: [PATCH 03/11] s390x/cpumodel: Fix harmless misuse of visit_check_struct()

[Cornelia Huck]

On Wed, 29 Apr 2020 07:51:04 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> David Hildenbrand <david@redhat.com> writes:
> 
> > On 24.04.20 21:20, Markus Armbruster wrote:  
> >> Commit e47970f51d "s390x/cpumodel: Fix query-cpu-model-FOO error API
> >> violations" neglected to change visit_end_struct()'s Error ** argument
> >> along with the others.  If visit_end_struct() failed, we'd take the  
> >
> > s/visit_end_struct/visit_check_struct/ ?  
> 
> Will fix.
> 
> >> success path.  Fortunately, it can't fail here:
> >> qobject_input_check_struct() checks we consumed the whole dictionary,
> >> and to get here, we did.  Fix it anyway.  
> >
> > AFAIKs, if visit_check_struct() failed, we'd still do the memcopy, but
> > also report the error. Not nice, not bad.
> >
> > Reviewed-by: David Hildenbrand <david@redhat.com>  
> 
> Thanks!

Will you queue this, or shall I queue it?

Re: [PATCH 03/11] s390x/cpumodel: Fix harmless misuse of visit_check_struct()

[Markus Armbruster]

Cornelia Huck <cohuck@redhat.com> writes:

> On Wed, 29 Apr 2020 07:51:04 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> David Hildenbrand <david@redhat.com> writes:
>> 
>> > On 24.04.20 21:20, Markus Armbruster wrote:  
>> >> Commit e47970f51d "s390x/cpumodel: Fix query-cpu-model-FOO error API
>> >> violations" neglected to change visit_end_struct()'s Error ** argument
>> >> along with the others.  If visit_end_struct() failed, we'd take the  
>> >
>> > s/visit_end_struct/visit_check_struct/ ?  
>> 
>> Will fix.
>> 
>> >> success path.  Fortunately, it can't fail here:
>> >> qobject_input_check_struct() checks we consumed the whole dictionary,
>> >> and to get here, we did.  Fix it anyway.  
>> >
>> > AFAIKs, if visit_check_struct() failed, we'd still do the memcopy, but
>> > also report the error. Not nice, not bad.
>> >
>> > Reviewed-by: David Hildenbrand <david@redhat.com>  
>> 
>> Thanks!
>
> Will you queue this, or shall I queue it?

Me taking the complete series through my tree would be easiest for me.
But I can cope with other maintainers picking up bits.

Re: [PATCH 03/11] s390x/cpumodel: Fix harmless misuse of visit_check_struct()

[Cornelia Huck]

On Mon, 04 May 2020 17:24:59 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Cornelia Huck <cohuck@redhat.com> writes:
> 
> > On Wed, 29 Apr 2020 07:51:04 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >  
> >> David Hildenbrand <david@redhat.com> writes:
> >>   
> >> > On 24.04.20 21:20, Markus Armbruster wrote:    
> >> >> Commit e47970f51d "s390x/cpumodel: Fix query-cpu-model-FOO error API
> >> >> violations" neglected to change visit_end_struct()'s Error ** argument
> >> >> along with the others.  If visit_end_struct() failed, we'd take the    
> >> >
> >> > s/visit_end_struct/visit_check_struct/ ?    
> >> 
> >> Will fix.
> >>   
> >> >> success path.  Fortunately, it can't fail here:
> >> >> qobject_input_check_struct() checks we consumed the whole dictionary,
> >> >> and to get here, we did.  Fix it anyway.    
> >> >
> >> > AFAIKs, if visit_check_struct() failed, we'd still do the memcopy, but
> >> > also report the error. Not nice, not bad.
> >> >
> >> > Reviewed-by: David Hildenbrand <david@redhat.com>    
> >> 
> >> Thanks!  
> >
> > Will you queue this, or shall I queue it?  
> 
> Me taking the complete series through my tree would be easiest for me.
> But I can cope with other maintainers picking up bits.

In that case, have my

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

and feel free to pick up :)

[PATCH 04/11] s390x/pci: Fix harmless mistake in zpci's property fid's setter

[Markus Armbruster]

s390_pci_set_fid() sets zpci->fid_defined to true even when
visit_type_uint32() failed.  Reproducer: "-device zpci,fid=junk".
Harmless in practice, because qdev_device_add() then fails, throwing
away @zpci.  Fix it anyway.

Cc: Matthew Rosato <mjrosato@linux.ibm.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/s390x/s390-pci-bus.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index ed8be124da..19ee1f02bb 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -1276,7 +1276,9 @@ static void s390_pci_set_fid(Object *obj, Visitor *v, const char *name,
         return;
     }
 
-    visit_type_uint32(v, name, ptr, errp);
+    if (!visit_type_uint32(v, name, ptr, errp)) {
+        return;
+    }
     zpci->fid_defined = true;
 }
 
-- 
2.21.1

Re: [PATCH 04/11] s390x/pci: Fix harmless mistake in zpci's property fid's setter

[Matthew Rosato]

On 4/24/20 3:20 PM, Markus Armbruster wrote:
> s390_pci_set_fid() sets zpci->fid_defined to true even when
> visit_type_uint32() failed.  Reproducer: "-device zpci,fid=junk".
> Harmless in practice, because qdev_device_add() then fails, throwing
> away @zpci.  Fix it anyway.
> 
> Cc: Matthew Rosato <mjrosato@linux.ibm.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/s390x/s390-pci-bus.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index ed8be124da..19ee1f02bb 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -1276,7 +1276,9 @@ static void s390_pci_set_fid(Object *obj, Visitor *v, const char *name,
>           return;
>       }
>   
> -    visit_type_uint32(v, name, ptr, errp);
> +    if (!visit_type_uint32(v, name, ptr, errp)) {
> +        return;
> +    }

Hi Markus,

Am I missing something here (a preceding patch maybe?) -- 
visit_type_uint32 is a void function.  A quick look, no other callers 
are checking it for a return value either...

The error value might get set in visit_type_uintN though.  Taking a hint 
from other places that handle this sort of case (ex: 
cpu_max_set_sve_max_vq), maybe something like:

Error *err = NULL;
...
visit_type_uint32(v, name, ptr, &err);
if (err) {
	error_propogate(errp, err);
	return;
}
zpci->fid_defined = true;

Instead?

Re: [PATCH 04/11] s390x/pci: Fix harmless mistake in zpci's property fid's setter

[Markus Armbruster]

Matthew Rosato <mjrosato@linux.ibm.com> writes:

> On 4/24/20 3:20 PM, Markus Armbruster wrote:
>> s390_pci_set_fid() sets zpci->fid_defined to true even when
>> visit_type_uint32() failed.  Reproducer: "-device zpci,fid=junk".
>> Harmless in practice, because qdev_device_add() then fails, throwing
>> away @zpci.  Fix it anyway.
>>
>> Cc: Matthew Rosato <mjrosato@linux.ibm.com>
>> Cc: Cornelia Huck <cohuck@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   hw/s390x/s390-pci-bus.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index ed8be124da..19ee1f02bb 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -1276,7 +1276,9 @@ static void s390_pci_set_fid(Object *obj, Visitor *v, const char *name,
>>           return;
>>       }
>>   -    visit_type_uint32(v, name, ptr, errp);
>> +    if (!visit_type_uint32(v, name, ptr, errp)) {
>> +        return;
>> +    }
>
> Hi Markus,
>
> Am I missing something here (a preceding patch maybe?) -- 
> visit_type_uint32 is a void function.  A quick look, no other callers
> are checking it for a return value either...
>
> The error value might get set in visit_type_uintN though.  Taking a
> hint from other places that handle this sort of case (ex:
> cpu_max_set_sve_max_vq), maybe something like:
>
> Error *err = NULL;
> ...
> visit_type_uint32(v, name, ptr, &err);
> if (err) {
> 	error_propogate(errp, err);
> 	return;
> }
> zpci->fid_defined = true;
>
> Instead?

This patch crept into this series by mistake.  It indeed depends on
other work I haven't published, yet.  Thanks, and sorry for wasting your
time!

[PATCH 05/11] tests/migration: Tighten error checking

[Markus Armbruster]

migrate_get_socket_address() neglects to check
visit_type_SocketAddressList() failure.  This smells like a leak, but
it actually will crash dereferencing @addrs.  Pass &error_abort to
remove the code smell.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qtest/migration-test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 2568c9529c..dc3490c9fa 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -13,6 +13,7 @@
 #include "qemu/osdep.h"
 
 #include "libqtest.h"
+#include "qapi/error.h"
 #include "qapi/qmp/qdict.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
@@ -301,7 +302,6 @@ static char *migrate_get_socket_address(QTestState *who, const char *parameter)
 {
     QDict *rsp;
     char *result;
-    Error *local_err = NULL;
     SocketAddressList *addrs;
     Visitor *iv = NULL;
     QObject *object;
@@ -310,7 +310,7 @@ static char *migrate_get_socket_address(QTestState *who, const char *parameter)
     object = qdict_get(rsp, parameter);
 
     iv = qobject_input_visitor_new(object);
-    visit_type_SocketAddressList(iv, NULL, &addrs, &local_err);
+    visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort);
     visit_free(iv);
 
     /* we are only using a single address */
-- 
2.21.1

[PATCH 06/11] error: Use error_reportf_err() where appropriate

[Markus Armbruster]

Replace

    error_report("...: %s", ..., error_get_pretty(err));

by

    error_reportf_err(err, "...: ", ...);

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 chardev/char-socket.c | 5 +++--
 hw/sd/pxa2xx_mmci.c   | 4 ++--
 hw/sd/sd.c            | 4 ++--
 hw/usb/dev-mtp.c      | 9 +++++----
 qemu-nbd.c            | 7 +++----
 scsi/qemu-pr-helper.c | 4 ++--
 6 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 185fe38dda..e5ee685f8c 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -138,8 +138,9 @@ static void check_report_connect_error(Chardev *chr,
     SocketChardev *s = SOCKET_CHARDEV(chr);
 
     if (!s->connect_err_reported) {
-        error_report("Unable to connect character device %s: %s",
-                     chr->label, error_get_pretty(err));
+        error_reportf_err(err,
+                          "Unable to connect character device %s: ",
+                          chr->label);
         s->connect_err_reported = true;
     }
     qemu_chr_socket_restart_timer(chr);
diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index 8f9ab0ec16..f9c50ddda5 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -497,12 +497,12 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
     carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD_CARD);
     qdev_prop_set_drive(carddev, "drive", blk, &err);
     if (err) {
-        error_report("failed to init SD card: %s", error_get_pretty(err));
+        error_reportf_err(err, "failed to init SD card: ");
         return NULL;
     }
     object_property_set_bool(OBJECT(carddev), true, "realized", &err);
     if (err) {
-        error_report("failed to init SD card: %s", error_get_pretty(err));
+        error_reportf_err(err, "failed to init SD card: ");
         return NULL;
     }
 
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 71a9af09ab..3c06a0ac6d 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -703,13 +703,13 @@ SDState *sd_init(BlockBackend *blk, bool is_spi)
     dev = DEVICE(obj);
     qdev_prop_set_drive(dev, "drive", blk, &err);
     if (err) {
-        error_report("sd_init failed: %s", error_get_pretty(err));
+        error_reportf_err(err, "sd_init failed: ");
         return NULL;
     }
     qdev_prop_set_bit(dev, "spi", is_spi);
     object_property_set_bool(obj, true, "realized", &err);
     if (err) {
-        error_report("sd_init failed: %s", error_get_pretty(err));
+        error_reportf_err(err, "sd_init failed: ");
         return NULL;
     }
 
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 20717f026b..168428156b 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -631,8 +631,9 @@ static void usb_mtp_object_readdir(MTPState *s, MTPObject *o)
         int64_t id = qemu_file_monitor_add_watch(s->file_monitor, o->path, NULL,
                                                  file_monitor_event, s, &err);
         if (id == -1) {
-            error_report("usb-mtp: failed to add watch for %s: %s", o->path,
-                         error_get_pretty(err));
+            error_reportf_err(err,
+                              "usb-mtp: failed to add watch for %s: ",
+                              o->path);
             error_free(err);
         } else {
             trace_usb_mtp_file_monitor_event(s->dev.addr, o->path,
@@ -1276,8 +1277,8 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
 
         s->file_monitor = qemu_file_monitor_new(&err);
         if (err) {
-            error_report("usb-mtp: file monitoring init failed: %s",
-                         error_get_pretty(err));
+            error_reportf_err(err,
+                              "usb-mtp: file monitoring init failed: ");
             error_free(err);
         } else {
             QTAILQ_INIT(&s->events);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 4aa005004e..30deb5d9e6 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -856,8 +856,7 @@ int main(int argc, char **argv)
         }
         tlscreds = nbd_get_tls_creds(tlscredsid, list, &local_err);
         if (local_err) {
-            error_report("Failed to get TLS creds %s",
-                         error_get_pretty(local_err));
+            error_reportf_err(local_err, "Failed to get TLS creds ");
             exit(EXIT_FAILURE);
         }
     } else {
@@ -979,8 +978,8 @@ int main(int argc, char **argv)
                                              &local_err);
             if (sioc == NULL) {
                 object_unref(OBJECT(server));
-                error_report("Failed to use socket activation: %s",
-                             error_get_pretty(local_err));
+                error_reportf_err(local_err,
+                                  "Failed to use socket activation: ");
                 exit(EXIT_FAILURE);
             }
             qio_net_listener_add(server, sioc);
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index 181ed4a186..57ad830d54 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -1030,8 +1030,8 @@ int main(int argc, char **argv)
         server_ioc = qio_channel_socket_new_fd(FIRST_SOCKET_ACTIVATION_FD,
                                                &local_err);
         if (server_ioc == NULL) {
-            error_report("Failed to use socket activation: %s",
-                         error_get_pretty(local_err));
+            error_reportf_err(local_err,
+                              "Failed to use socket activation: ");
             exit(EXIT_FAILURE);
         }
     }
-- 
2.21.1

Re: [PATCH 06/11] error: Use error_reportf_err() where appropriate

[Eric Blake]

On 4/24/20 2:20 PM, Markus Armbruster wrote:
> Replace
> 
>      error_report("...: %s", ..., error_get_pretty(err));
> 
> by
> 
>      error_reportf_err(err, "...: ", ...);

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

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   chardev/char-socket.c | 5 +++--
>   hw/sd/pxa2xx_mmci.c   | 4 ++--
>   hw/sd/sd.c            | 4 ++--
>   hw/usb/dev-mtp.c      | 9 +++++----
>   qemu-nbd.c            | 7 +++----
>   scsi/qemu-pr-helper.c | 4 ++--
>   6 files changed, 17 insertions(+), 16 deletions(-)

Although it touches NBD, I'm happy for this to go through your tree with 
the larger series.

> +++ b/qemu-nbd.c
> @@ -856,8 +856,7 @@ int main(int argc, char **argv)
>           }
>           tlscreds = nbd_get_tls_creds(tlscredsid, list, &local_err);
>           if (local_err) {
> -            error_report("Failed to get TLS creds %s",
> -                         error_get_pretty(local_err));
> +            error_reportf_err(local_err, "Failed to get TLS creds ");

Odd one out for not using ':' in the message, but that's independent of 
this patch.

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

Re: [PATCH 06/11] error: Use error_reportf_err() where appropriate

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 4/24/20 2:20 PM, Markus Armbruster wrote:
>> Replace
>>
>>      error_report("...: %s", ..., error_get_pretty(err));
>>
>> by
>>
>>      error_reportf_err(err, "...: ", ...);
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   chardev/char-socket.c | 5 +++--
>>   hw/sd/pxa2xx_mmci.c   | 4 ++--
>>   hw/sd/sd.c            | 4 ++--
>>   hw/usb/dev-mtp.c      | 9 +++++----
>>   qemu-nbd.c            | 7 +++----
>>   scsi/qemu-pr-helper.c | 4 ++--
>>   6 files changed, 17 insertions(+), 16 deletions(-)
>
> Although it touches NBD, I'm happy for this to go through your tree
> with the larger series.
>
>> +++ b/qemu-nbd.c
>> @@ -856,8 +856,7 @@ int main(int argc, char **argv)
>>           }
>>           tlscreds = nbd_get_tls_creds(tlscredsid, list, &local_err);
>>           if (local_err) {
>> -            error_report("Failed to get TLS creds %s",
>> -                         error_get_pretty(local_err));
>> +            error_reportf_err(local_err, "Failed to get TLS creds ");
>
> Odd one out for not using ':' in the message, but that's independent
> of this patch.

The patch is short enough to deviate from "purely mechanical" and stick
in ':' here.  Your choice.

Thanks!

Re: [PATCH 06/11] error: Use error_reportf_err() where appropriate

[Philippe Mathieu-Daudé]

On 4/27/20 10:53 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 4/24/20 2:20 PM, Markus Armbruster wrote:
>>> Replace
>>>
>>>       error_report("...: %s", ..., error_get_pretty(err));
>>>
>>> by
>>>
>>>       error_reportf_err(err, "...: ", ...);
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>    chardev/char-socket.c | 5 +++--
>>>    hw/sd/pxa2xx_mmci.c   | 4 ++--
>>>    hw/sd/sd.c            | 4 ++--
>>>    hw/usb/dev-mtp.c      | 9 +++++----
>>>    qemu-nbd.c            | 7 +++----
>>>    scsi/qemu-pr-helper.c | 4 ++--
>>>    6 files changed, 17 insertions(+), 16 deletions(-)
>>
>> Although it touches NBD, I'm happy for this to go through your tree
>> with the larger series.
>>
>>> +++ b/qemu-nbd.c
>>> @@ -856,8 +856,7 @@ int main(int argc, char **argv)
>>>            }
>>>            tlscreds = nbd_get_tls_creds(tlscredsid, list, &local_err);
>>>            if (local_err) {
>>> -            error_report("Failed to get TLS creds %s",
>>> -                         error_get_pretty(local_err));
>>> +            error_reportf_err(local_err, "Failed to get TLS creds ");
>>
>> Odd one out for not using ':' in the message, but that's independent
>> of this patch.
> 
> The patch is short enough to deviate from "purely mechanical" and stick
> in ':' here.  Your choice.

Let's deviate, else we might forget about it.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 06/11] error: Use error_reportf_err() where appropriate

[Eric Blake]

On 4/27/20 3:53 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 

>> Although it touches NBD, I'm happy for this to go through your tree
>> with the larger series.
>>
>>> +++ b/qemu-nbd.c
>>> @@ -856,8 +856,7 @@ int main(int argc, char **argv)
>>>            }
>>>            tlscreds = nbd_get_tls_creds(tlscredsid, list, &local_err);
>>>            if (local_err) {
>>> -            error_report("Failed to get TLS creds %s",
>>> -                         error_get_pretty(local_err));
>>> +            error_reportf_err(local_err, "Failed to get TLS creds ");
>>
>> Odd one out for not using ':' in the message, but that's independent
>> of this patch.
> 
> The patch is short enough to deviate from "purely mechanical" and stick
> in ':' here.  Your choice.

Adding it in now is fine by me.

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

[PATCH 07/11] mips/malta: Fix create_cps() error handling

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second

create_cps() is wrong that way.  The last calls treats an error as
fatal.  Do that for the prior ones, too.

Fixes: bff384a4fbd5d0e86939092e74e766ef0f5f592c
Cc: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/mips/mips_malta.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index e4c4de1b4e..17bf41616b 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1185,17 +1185,14 @@ static void create_cpu_without_cps(MachineState *ms,
 static void create_cps(MachineState *ms, MaltaState *s,
                        qemu_irq *cbus_irq, qemu_irq *i8259_irq)
 {
-    Error *err = NULL;
-
     sysbus_init_child_obj(OBJECT(s), "cps", OBJECT(&s->cps), sizeof(s->cps),
                           TYPE_MIPS_CPS);
-    object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type", &err);
-    object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp", &err);
-    object_property_set_bool(OBJECT(&s->cps), true, "realized", &err);
-    if (err != NULL) {
-        error_report("%s", error_get_pretty(err));
-        exit(1);
-    }
+    object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type",
+                            &error_fatal);
+    object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp",
+                            &error_fatal);
+    object_property_set_bool(OBJECT(&s->cps), true, "realized",
+                             &error_fatal);
 
     sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
 
-- 
2.21.1

Re: [PATCH 07/11] mips/malta: Fix create_cps() error handling

[Philippe Mathieu-Daudé]

On 4/24/20 9:20 PM, Markus Armbruster wrote:
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
> 
> create_cps() is wrong that way.  The last calls treats an error as
> fatal.  Do that for the prior ones, too.
> 
> Fixes: bff384a4fbd5d0e86939092e74e766ef0f5f592c
> Cc: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
> Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/mips/mips_malta.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index e4c4de1b4e..17bf41616b 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -1185,17 +1185,14 @@ static void create_cpu_without_cps(MachineState *ms,
>  static void create_cps(MachineState *ms, MaltaState *s,
>                         qemu_irq *cbus_irq, qemu_irq *i8259_irq)
>  {
> -    Error *err = NULL;
> -
>      sysbus_init_child_obj(OBJECT(s), "cps", OBJECT(&s->cps), sizeof(s->cps),
>                            TYPE_MIPS_CPS);
> -    object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type", &err);
> -    object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp", &err);
> -    object_property_set_bool(OBJECT(&s->cps), true, "realized", &err);
> -    if (err != NULL) {
> -        error_report("%s", error_get_pretty(err));

In https://www.mail-archive.com/qemu-devel@nongnu.org/msg695645.html I
also remove "qemu/error-report.h" which is not needed once you remove
this call.

> -        exit(1);
> -    }
> +    object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type",
> +                            &error_fatal);
> +    object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp",
> +                            &error_fatal);
> +    object_property_set_bool(OBJECT(&s->cps), true, "realized",
> +                             &error_fatal);
>  
>      sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
>  
> 

Re: [PATCH 07/11] mips/malta: Fix create_cps() error handling

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 4/24/20 9:20 PM, Markus Armbruster wrote:
>> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
>> pointer to a variable containing NULL.  Passing an argument of the
>> latter kind twice without clearing it in between is wrong: if the
>> first call sets an error, it no longer points to NULL for the second
>> 
>> create_cps() is wrong that way.  The last calls treats an error as
>> fatal.  Do that for the prior ones, too.
>> 
>> Fixes: bff384a4fbd5d0e86939092e74e766ef0f5f592c
>> Cc: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
>> Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>
>> Cc: Aurelien Jarno <aurelien@aurel32.net>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/mips/mips_malta.c | 15 ++++++---------
>>  1 file changed, 6 insertions(+), 9 deletions(-)
>> 
>> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
>> index e4c4de1b4e..17bf41616b 100644
>> --- a/hw/mips/mips_malta.c
>> +++ b/hw/mips/mips_malta.c
>> @@ -1185,17 +1185,14 @@ static void create_cpu_without_cps(MachineState *ms,
>>  static void create_cps(MachineState *ms, MaltaState *s,
>>                         qemu_irq *cbus_irq, qemu_irq *i8259_irq)
>>  {
>> -    Error *err = NULL;
>> -
>>      sysbus_init_child_obj(OBJECT(s), "cps", OBJECT(&s->cps), sizeof(s->cps),
>>                            TYPE_MIPS_CPS);
>> -    object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type", &err);
>> -    object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp", &err);
>> -    object_property_set_bool(OBJECT(&s->cps), true, "realized", &err);
>> -    if (err != NULL) {
>> -        error_report("%s", error_get_pretty(err));
>
> In https://www.mail-archive.com/qemu-devel@nongnu.org/msg695645.html I
> also remove "qemu/error-report.h" which is not needed once you remove
> this call.

Missed it, sorry.  I only reviewed the Coccinelle scripts [PATCH 1+3/7].

I'd replace my patch by yours to give you proper credit, but your commit
message mentions "the coccinelle script", presumably the one from PATCH
1/7, and that patch isn't quite ready in my opinion.

>> -        exit(1);
>> -    }
>> +    object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type",
>> +                            &error_fatal);
>> +    object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp",
>> +                            &error_fatal);
>> +    object_property_set_bool(OBJECT(&s->cps), true, "realized",
>> +                             &error_fatal);
>>  
>>      sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
>>  
>> 

Re: [PATCH 07/11] mips/malta: Fix create_cps() error handling

[Philippe Mathieu-Daudé]

+Peter for crediting his advice.

On 4/29/20 7:59 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
>> On 4/24/20 9:20 PM, Markus Armbruster wrote:
>>> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
>>> pointer to a variable containing NULL.  Passing an argument of the
>>> latter kind twice without clearing it in between is wrong: if the
>>> first call sets an error, it no longer points to NULL for the second
>>>
>>> create_cps() is wrong that way.  The last calls treats an error as
>>> fatal.  Do that for the prior ones, too.
>>>
>>> Fixes: bff384a4fbd5d0e86939092e74e766ef0f5f592c
>>> Cc: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
>>> Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>
>>> Cc: Aurelien Jarno <aurelien@aurel32.net>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>   hw/mips/mips_malta.c | 15 ++++++---------
>>>   1 file changed, 6 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
>>> index e4c4de1b4e..17bf41616b 100644
>>> --- a/hw/mips/mips_malta.c
>>> +++ b/hw/mips/mips_malta.c
>>> @@ -1185,17 +1185,14 @@ static void create_cpu_without_cps(MachineState *ms,
>>>   static void create_cps(MachineState *ms, MaltaState *s,
>>>                          qemu_irq *cbus_irq, qemu_irq *i8259_irq)
>>>   {
>>> -    Error *err = NULL;
>>> -
>>>       sysbus_init_child_obj(OBJECT(s), "cps", OBJECT(&s->cps), sizeof(s->cps),
>>>                             TYPE_MIPS_CPS);
>>> -    object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type", &err);
>>> -    object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp", &err);
>>> -    object_property_set_bool(OBJECT(&s->cps), true, "realized", &err);
>>> -    if (err != NULL) {
>>> -        error_report("%s", error_get_pretty(err));
>>
>> In https://www.mail-archive.com/qemu-devel@nongnu.org/msg695645.html I
>> also remove "qemu/error-report.h" which is not needed once you remove
>> this call.
> 
> Missed it, sorry.  I only reviewed the Coccinelle scripts [PATCH 1+3/7].

My bad for not Cc'ing you on the whole series, which is Error related, 
and use the default get_maintainer.pl selection.

> I'd replace my patch by yours to give you proper credit, but your commit
> message mentions "the coccinelle script", presumably the one from PATCH
> 1/7, and that patch isn't quite ready in my opinion.

I'm not worried about credit, but duplicating effort or wasting time.

Peter once warned the problem with Coccinelle scripts is finding the 
correct balance between time spent to improve QEMU codebase, and time 
spent learning Coccinelle and improving a script that is often used only 
once in a lifetime.
If the script is not provided, we ask for the script. If the script is 
embedded in various patch descriptions, we ask to add it independently 
for reuse or as example. Then the script must be almost perfect. 
Meanwhile all the following patches referencing it, while reviewed, are 
stuck.

Anyway back to your patch:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
>>> -        exit(1);
>>> -    }
>>> +    object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type",
>>> +                            &error_fatal);
>>> +    object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp",
>>> +                            &error_fatal);
>>> +    object_property_set_bool(OBJECT(&s->cps), true, "realized",
>>> +                             &error_fatal);
>>>   
>>>       sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
>>>   
>>>
> 

Re: [PATCH 07/11] mips/malta: Fix create_cps() error handling

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> +Peter for crediting his advice.
>
> On 4/29/20 7:59 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>>
>>> On 4/24/20 9:20 PM, Markus Armbruster wrote:
>>>> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
>>>> pointer to a variable containing NULL.  Passing an argument of the
>>>> latter kind twice without clearing it in between is wrong: if the
>>>> first call sets an error, it no longer points to NULL for the second
>>>>
>>>> create_cps() is wrong that way.  The last calls treats an error as
>>>> fatal.  Do that for the prior ones, too.
>>>>
>>>> Fixes: bff384a4fbd5d0e86939092e74e766ef0f5f592c
>>>> Cc: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
>>>> Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>
>>>> Cc: Aurelien Jarno <aurelien@aurel32.net>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>>   hw/mips/mips_malta.c | 15 ++++++---------
>>>>   1 file changed, 6 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
>>>> index e4c4de1b4e..17bf41616b 100644
>>>> --- a/hw/mips/mips_malta.c
>>>> +++ b/hw/mips/mips_malta.c
>>>> @@ -1185,17 +1185,14 @@ static void create_cpu_without_cps(MachineState *ms,
>>>>   static void create_cps(MachineState *ms, MaltaState *s,
>>>>                          qemu_irq *cbus_irq, qemu_irq *i8259_irq)
>>>>   {
>>>> -    Error *err = NULL;
>>>> -
>>>>       sysbus_init_child_obj(OBJECT(s), "cps", OBJECT(&s->cps), sizeof(s->cps),
>>>>                             TYPE_MIPS_CPS);
>>>> -    object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type", &err);
>>>> -    object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp", &err);
>>>> -    object_property_set_bool(OBJECT(&s->cps), true, "realized", &err);
>>>> -    if (err != NULL) {
>>>> -        error_report("%s", error_get_pretty(err));
>>>
>>> In https://www.mail-archive.com/qemu-devel@nongnu.org/msg695645.html I
>>> also remove "qemu/error-report.h" which is not needed once you remove
>>> this call.
>>
>> Missed it, sorry.  I only reviewed the Coccinelle scripts [PATCH 1+3/7].
>
> My bad for not Cc'ing you on the whole series, which is Error related,
> and use the default get_maintainer.pl selection.
>
>> I'd replace my patch by yours to give you proper credit, but your commit
>> message mentions "the coccinelle script", presumably the one from PATCH
>> 1/7, and that patch isn't quite ready in my opinion.
>
> I'm not worried about credit, but duplicating effort or wasting time.
>
> Peter once warned the problem with Coccinelle scripts is finding the
> correct balance between time spent to improve QEMU codebase, and time
> spent learning Coccinelle and improving a script that is often used
> only once in a lifetime.
> If the script is not provided, we ask for the script. If the script is
> embedded in various patch descriptions, we ask to add it independently
> for reuse or as example. Then the script must be almost
> perfect. Meanwhile all the following patches referencing it, while
> reviewed, are stuck.

True.  I try not to ask for undue perfection, but perfection eludes me
in that, too :)

For PATCH 1/7, I only asked you to explain the script's limitations in
the script and the commit message.  Her's a bit of inspiration from the
kernel's scripts/coccinelle/misc/doubleinit.cocci (picked almost at
random):

    /// Find duplicate field initializations.  This has a high rate of false
    /// positives due to #ifdefs, which Coccinelle is not aware of in a structure
    /// initialization.
    ///
    // Confidence: Low

I like the Confidence: tag.  It should come with an explanation, as it
does here.

For PATCH 3/7, I had a question.

> Anyway back to your patch:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks!

[PATCH 08/11] mips/boston: Fix boston_mach_init() error handling

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second

boston_mach_init() is wrong that way.  The last calls treats an error
as fatal.  Do that for the prior ones, too.

Fixes: df1d8a1f29f567567b9d20be685a4241282e7005
Cc: Paul Burton <pburton@wavecomp.com>
Cc: Aleksandar Rikalo <aleksandar.rikalo@rt-rk.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/mips/boston.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index 98ecd25e8e..2832dfa6ae 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -458,14 +458,11 @@ static void boston_mach_init(MachineState *machine)
     sysbus_init_child_obj(OBJECT(machine), "cps", OBJECT(&s->cps),
                           sizeof(s->cps), TYPE_MIPS_CPS);
     object_property_set_str(OBJECT(&s->cps), machine->cpu_type, "cpu-type",
-                            &err);
-    object_property_set_int(OBJECT(&s->cps), machine->smp.cpus, "num-vp", &err);
-    object_property_set_bool(OBJECT(&s->cps), true, "realized", &err);
-
-    if (err != NULL) {
-        error_report("%s", error_get_pretty(err));
-        exit(1);
-    }
+                            &error_fatal);
+    object_property_set_int(OBJECT(&s->cps), machine->smp.cpus, "num-vp",
+                            &error_fatal);
+    object_property_set_bool(OBJECT(&s->cps), true, "realized",
+                             &error_fatal);
 
     sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
 
-- 
2.21.1

Re: [PATCH 08/11] mips/boston: Fix boston_mach_init() error handling

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second

  call.

Whoops!

>
> boston_mach_init() is wrong that way.  The last calls treats an error
> as fatal.  Do that for the prior ones, too.
>
> Fixes: df1d8a1f29f567567b9d20be685a4241282e7005
> Cc: Paul Burton <pburton@wavecomp.com>
> Cc: Aleksandar Rikalo <aleksandar.rikalo@rt-rk.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/mips/boston.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/hw/mips/boston.c b/hw/mips/boston.c
> index 98ecd25e8e..2832dfa6ae 100644
> --- a/hw/mips/boston.c
> +++ b/hw/mips/boston.c
> @@ -458,14 +458,11 @@ static void boston_mach_init(MachineState *machine)
>      sysbus_init_child_obj(OBJECT(machine), "cps", OBJECT(&s->cps),
>                            sizeof(s->cps), TYPE_MIPS_CPS);
>      object_property_set_str(OBJECT(&s->cps), machine->cpu_type, "cpu-type",
> -                            &err);
> -    object_property_set_int(OBJECT(&s->cps), machine->smp.cpus, "num-vp", &err);
> -    object_property_set_bool(OBJECT(&s->cps), true, "realized", &err);
> -
> -    if (err != NULL) {
> -        error_report("%s", error_get_pretty(err));
> -        exit(1);
> -    }
> +                            &error_fatal);
> +    object_property_set_int(OBJECT(&s->cps), machine->smp.cpus, "num-vp",
> +                            &error_fatal);
> +    object_property_set_bool(OBJECT(&s->cps), true, "realized",
> +                             &error_fatal);
>  
>      sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);

[PATCH 09/11] mips/boston: Plug memory leak in boston_mach_init()

[Markus Armbruster]

Fixes: df1d8a1f29f567567b9d20be685a4241282e7005
Cc: Paul Burton <pburton@wavecomp.com>
Cc: Aleksandar Rikalo <aleksandar.rikalo@rt-rk.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/mips/boston.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index 2832dfa6ae..a896056be1 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -426,7 +426,6 @@ static void boston_mach_init(MachineState *machine)
 {
     DeviceState *dev;
     BostonState *s;
-    Error *err = NULL;
     MemoryRegion *flash, *ddr_low_alias, *lcd, *platreg;
     MemoryRegion *sys_mem = get_system_memory();
     XilinxPCIEHost *pcie2;
@@ -467,7 +466,8 @@ static void boston_mach_init(MachineState *machine)
     sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
 
     flash =  g_new(MemoryRegion, 1);
-    memory_region_init_rom(flash, NULL, "boston.flash", 128 * MiB, &err);
+    memory_region_init_rom(flash, NULL, "boston.flash", 128 * MiB,
+                           &error_fatal);
     memory_region_add_subregion_overlap(sys_mem, 0x18000000, flash, 0);
 
     memory_region_add_subregion_overlap(sys_mem, 0x80000000, machine->ram, 0);
-- 
2.21.1

[PATCH 10/11] arm/sabrelite: Consistently use &error_fatal in sabrelite_init()

[Markus Armbruster]

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jean-Christophe Dubois <jcd@tribudubois.net>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/sabrelite.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/hw/arm/sabrelite.c b/hw/arm/sabrelite.c
index e31694bb92..04f4b96591 100644
--- a/hw/arm/sabrelite.c
+++ b/hw/arm/sabrelite.c
@@ -41,7 +41,6 @@ static void sabrelite_reset_secondary(ARMCPU *cpu,
 static void sabrelite_init(MachineState *machine)
 {
     FslIMX6State *s;
-    Error *err = NULL;
 
     /* Check the amount of memory is compatible with the SOC */
     if (machine->ram_size > FSL_IMX6_MMDC_SIZE) {
@@ -52,11 +51,7 @@ static void sabrelite_init(MachineState *machine)
 
     s = FSL_IMX6(object_new(TYPE_FSL_IMX6));
     object_property_add_child(OBJECT(machine), "soc", OBJECT(s), &error_fatal);
-    object_property_set_bool(OBJECT(s), true, "realized", &err);
-    if (err != NULL) {
-        error_report("%s", error_get_pretty(err));
-        exit(1);
-    }
+    object_property_set_bool(OBJECT(s), true, "realized", &error_fatal);
 
     memory_region_add_subregion(get_system_memory(), FSL_IMX6_MMDC_ADDR,
                                 machine->ram);
-- 
2.21.1

Re: [PATCH 10/11] arm/sabrelite: Consistently use &error_fatal in sabrelite_init()

[Philippe Mathieu-Daudé]

On 4/24/20 9:20 PM, Markus Armbruster wrote:
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jean-Christophe Dubois <jcd@tribudubois.net>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/arm/sabrelite.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/hw/arm/sabrelite.c b/hw/arm/sabrelite.c
> index e31694bb92..04f4b96591 100644
> --- a/hw/arm/sabrelite.c
> +++ b/hw/arm/sabrelite.c
> @@ -41,7 +41,6 @@ static void sabrelite_reset_secondary(ARMCPU *cpu,
>  static void sabrelite_init(MachineState *machine)
>  {
>      FslIMX6State *s;
> -    Error *err = NULL;
>  
>      /* Check the amount of memory is compatible with the SOC */
>      if (machine->ram_size > FSL_IMX6_MMDC_SIZE) {
> @@ -52,11 +51,7 @@ static void sabrelite_init(MachineState *machine)
>  
>      s = FSL_IMX6(object_new(TYPE_FSL_IMX6));
>      object_property_add_child(OBJECT(machine), "soc", OBJECT(s), &error_fatal);
> -    object_property_set_bool(OBJECT(s), true, "realized", &err);
> -    if (err != NULL) {
> -        error_report("%s", error_get_pretty(err));
> -        exit(1);
> -    }
> +    object_property_set_bool(OBJECT(s), true, "realized", &error_fatal);
>  
>      memory_region_add_subregion(get_system_memory(), FSL_IMX6_MMDC_ADDR,
>                                  machine->ram);
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH 11/11] i386: Fix x86_cpu_load_model() error API violation

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

x86_cpu_load_model() is wrong that way.  Harmless, because its @errp
is always &error_abort.  To fix, cut out the @errp middleman.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 target/i386/cpu.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 90ffc5f3b1..3f09fd2321 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5078,7 +5078,7 @@ static void x86_cpu_apply_version_props(X86CPU *cpu, X86CPUModel *model)
 
 /* Load data from X86CPUDefinition into a X86CPU object
  */
-static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model, Error **errp)
+static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model)
 {
     X86CPUDefinition *def = model->cpudef;
     CPUX86State *env = &cpu->env;
@@ -5092,13 +5092,19 @@ static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model, Error **errp)
      */
 
     /* CPU models only set _minimum_ values for level/xlevel: */
-    object_property_set_uint(OBJECT(cpu), def->level, "min-level", errp);
-    object_property_set_uint(OBJECT(cpu), def->xlevel, "min-xlevel", errp);
+    object_property_set_uint(OBJECT(cpu), def->level, "min-level",
+                             &error_abort);
+    object_property_set_uint(OBJECT(cpu), def->xlevel, "min-xlevel",
+                             &error_abort);
 
-    object_property_set_int(OBJECT(cpu), def->family, "family", errp);
-    object_property_set_int(OBJECT(cpu), def->model, "model", errp);
-    object_property_set_int(OBJECT(cpu), def->stepping, "stepping", errp);
-    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
+    object_property_set_int(OBJECT(cpu), def->family, "family",
+                            &error_abort);
+    object_property_set_int(OBJECT(cpu), def->model, "model",
+                            &error_abort);
+    object_property_set_int(OBJECT(cpu), def->stepping, "stepping",
+                            &error_abort);
+    object_property_set_str(OBJECT(cpu), def->model_id, "model-id",
+                            &error_abort);
     for (w = 0; w < FEATURE_WORDS; w++) {
         env->features[w] = def->features[w];
     }
@@ -5135,7 +5141,8 @@ static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model, Error **errp)
         vendor = host_vendor;
     }
 
-    object_property_set_str(OBJECT(cpu), vendor, "vendor", errp);
+    object_property_set_str(OBJECT(cpu), vendor, "vendor",
+                            &error_abort);
 
     x86_cpu_apply_version_props(cpu, model);
 }
@@ -6981,7 +6988,7 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add_alias(obj, "sse4_2", obj, "sse4.2", &error_abort);
 
     if (xcc->model) {
-        x86_cpu_load_model(cpu, xcc->model, &error_abort);
+        x86_cpu_load_model(cpu, xcc->model);
     }
 }
 
-- 
2.21.1