| Message ID | 20191130194240.10517-21-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Error handling fixes, may contain 4.2 material | expand |
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?
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
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.
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)
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(-)