[v1] More miscellaneous error handling fixes

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

Markus Armbruster posted 11 patches 5 years, 6 months ago

Diff against v2
Download mbox

Maintainers: Anthony Perard <anthony.perard@citrix.com>, Laurent Vivier <lvivier@redhat.com>, Matthew Rosato <mjrosato@linux.ibm.com>, Peter Maydell <peter.maydell@linaro.org>, Andrzej Zaborowski <balrogg@gmail.com>, Juan Quintela <quintela@redhat.com>, Paul Durrant <paul@xen.org>, Richard Henderson <rth@twiddle.net>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Stefano Stabellini <sstabellini@kernel.org>, "Philippe Mathieu-Daudé" <philmd@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Cornelia Huck <cohuck@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Aurelien Jarno <aurelien@aurel32.net>, Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>, Christian Borntraeger <borntraeger@de.ibm.com>, Eric Blake <eblake@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>, Thomas Huth <thuth@redhat.com>, Halil Pasic <pasic@linux.ibm.com>, Fam Zheng <fam@euphon.net>, Xiao Guangrong <xiaoguangrong.eric@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Jean-Christophe Dubois <jcd@tribudubois.net>, Aleksandar Rikalo <aleksandar.rikalo@rt-rk.com>, David Hildenbrand <david@redhat.com>, Paul Burton <pburton@wavecomp.com>

There is a newer version of this series

Expand all Fold all

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

Posted by Markus Armbruster 5 years, 6 months ago

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

Posted by Matthew Rosato 5 years, 6 months ago

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

Posted by Markus Armbruster 5 years, 6 months ago

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!