[20/21] hw/intc/s390: Simplify error handling in kvm_s390_flic_realize()
diff mbox series

Message ID 20191130194240.10517-21-armbru@redhat.com
State New
Headers show
Series
  • Error handling fixes, may contain 4.2 material
Related show

Commit Message

Markus Armbruster Nov. 30, 2019, 7:42 p.m. UTC
Cc: Halil Pasic <pasic@linux.ibm.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/intc/s390_flic_kvm.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Comments

Cornelia Huck Dec. 2, 2019, 4:40 p.m. UTC | #1
On Sat, 30 Nov 2019 20:42:39 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Cc: Halil Pasic <pasic@linux.ibm.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/intc/s390_flic_kvm.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)

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

...someone else wants to take a look before I queue this?
Halil Pasic Dec. 3, 2019, 8:03 p.m. UTC | #2
On Mon, 2 Dec 2019 17:40:07 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Sat, 30 Nov 2019 20:42:39 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
> 
> > Cc: Halil Pasic <pasic@linux.ibm.com>
> > Cc: Cornelia Huck <cohuck@redhat.com>
> > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > ---
> >  hw/intc/s390_flic_kvm.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 
> ...someone else wants to take a look before I queue this?
> 

I guess it is a matter of taste.

Acked-by: Halil Pasic <pasic@linux.ibm.com>

The only difference I can see is if the **errp argument where
to already contain an error (*errp). I guess the old code would
just keep the first error, and discard the next one, while error_setv()
does an assert(*errp == NULL).

I assume calling with *errp != NULL is not a valid scenario. But then, I
can't say I understand the use-case behind this discard the newer error
behavior of error_propagate().

Regards,
Halil
Markus Armbruster Dec. 4, 2019, 7:28 a.m. UTC | #3
Halil Pasic <pasic@linux.ibm.com> writes:

> On Mon, 2 Dec 2019 17:40:07 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> On Sat, 30 Nov 2019 20:42:39 +0100
>> Markus Armbruster <armbru@redhat.com> wrote:
>> 
>> > Cc: Halil Pasic <pasic@linux.ibm.com>
>> > Cc: Cornelia Huck <cohuck@redhat.com>
>> > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
>> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> > ---
>> >  hw/intc/s390_flic_kvm.c | 10 ++++------
>> >  1 file changed, 4 insertions(+), 6 deletions(-)
>> 
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> 
>> ...someone else wants to take a look before I queue this?
>> 
>
> I guess it is a matter of taste.
>
> Acked-by: Halil Pasic <pasic@linux.ibm.com>

Thanks!

> The only difference I can see is if the **errp argument where
> to already contain an error (*errp). I guess the old code would
> just keep the first error, and discard the next one, while error_setv()
> does an assert(*errp == NULL).

Correct.

> I assume calling with *errp != NULL is not a valid scenario.

Correct again.  On function entry, @errp must either be null,
@error_fatal, @error_abort, or point to null.

>                                                              But then, I
> can't say I understand the use-case behind this discard the newer error
> behavior of error_propagate().

The documented[1] use case is "receive and accumulate multiple errors
(first one wins)".  See the big comment in include/qapi/error.h.

For what it's worth, GError explicitly disallows such accumulation: "The
error variable dest points to must be NULL"[2].  If you do it anyway, it
logs a warning nobody reads[3], then throws away the newer error.


[1] It's "ex post facto" documentation, like much of the Error API
documentation.

[2] https://developer.gnome.org/glib/stable/glib-Error-Reporting.html#g-propagate-error

[3] First order approximation based on the amount of crap supposedly
stable applications log.

Patch
diff mbox series

diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index 30d50c2369..dddd33ea61 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -586,16 +586,17 @@  static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
 
     KVM_S390_FLIC_GET_CLASS(dev)->parent_realize(dev, &err);
     if (err) {
-        goto fail;
+        error_propagate(errp, err);
+        return;
     }
     flic_state->fd = -1;
 
     cd.type = KVM_DEV_TYPE_FLIC;
     ret = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd);
     if (ret < 0) {
-        error_setg_errno(&err, errno, "Creating the KVM device failed");
+        error_setg_errno(errp, errno, "Creating the KVM device failed");
         trace_flic_create_device(errno);
-        goto fail;
+        return;
     }
     flic_state->fd = cd.fd;
 
@@ -603,9 +604,6 @@  static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
     test_attr.group = KVM_DEV_FLIC_CLEAR_IO_IRQ;
     flic_state->clear_io_supported = !ioctl(flic_state->fd,
                                             KVM_HAS_DEVICE_ATTR, test_attr);
-    return;
-fail:
-    error_propagate(errp, err);
 }
 
 static void kvm_s390_flic_reset(DeviceState *dev)