| Message ID | 1499177279-131407-4-git-send-email-borntraeger@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 4 Jul 2017 16:07:55 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > From: Halil Pasic <pasic@linux.vnet.ibm.com> > > From the moment it was introduced by commit a2875e6f98 ("s390x/kvm: > implement floating-interrupt controller device", 2013-07-16) the kvm-flic > is not making realize fail properly in case it's impossible to create the > KVM device which basically serves as a backend and is absolutely > essential for having an operational kvm-flic. > > Let's fix this by making sure we do proper error propagation in realize. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > Fixes: a2875e6f98 "s390x/kvm: implement floating-interrupt controller device" > Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> > Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > hw/intc/s390_flic_kvm.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > (...) > cd.type = KVM_DEV_TYPE_FLIC; > ret = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd); > if (ret < 0) { > - trace_flic_create_device(errno); > - return; > + error_setg_errno(&errp_local, errno, "Creating the KVM device failed"); > + trace_flic_no_device_api(errno); Err... this should still be trace_flic_create_device(), no? > + goto fail; > } > flic_state->fd = cd.fd;
On 07/04/2017 04:31 PM, Cornelia Huck wrote: > On Tue, 4 Jul 2017 16:07:55 +0200 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> From: Halil Pasic <pasic@linux.vnet.ibm.com> >> >> From the moment it was introduced by commit a2875e6f98 ("s390x/kvm: >> implement floating-interrupt controller device", 2013-07-16) the kvm-flic >> is not making realize fail properly in case it's impossible to create the >> KVM device which basically serves as a backend and is absolutely >> essential for having an operational kvm-flic. >> >> Let's fix this by making sure we do proper error propagation in realize. >> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> Fixes: a2875e6f98 "s390x/kvm: implement floating-interrupt controller device" >> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> >> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> hw/intc/s390_flic_kvm.c | 14 +++++++++++--- >> 1 file changed, 11 insertions(+), 3 deletions(-) >> > > (...) > >> cd.type = KVM_DEV_TYPE_FLIC; >> ret = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd); >> if (ret < 0) { >> - trace_flic_create_device(errno); >> - return; >> + error_setg_errno(&errp_local, errno, "Creating the KVM device failed"); >> + trace_flic_no_device_api(errno); > > Err... this should still be trace_flic_create_device(), no? I'm afraid you are right! Probably a copy paste error :/ > >> + goto fail; >> } >> flic_state->fd = cd.fd; >
On 07/04/2017 04:46 PM, Halil Pasic wrote: > > > On 07/04/2017 04:31 PM, Cornelia Huck wrote: >> On Tue, 4 Jul 2017 16:07:55 +0200 >> Christian Borntraeger <borntraeger@de.ibm.com> wrote: >> >>> From: Halil Pasic <pasic@linux.vnet.ibm.com> >>> >>> From the moment it was introduced by commit a2875e6f98 ("s390x/kvm: >>> implement floating-interrupt controller device", 2013-07-16) the kvm-flic >>> is not making realize fail properly in case it's impossible to create the >>> KVM device which basically serves as a backend and is absolutely >>> essential for having an operational kvm-flic. >>> >>> Let's fix this by making sure we do proper error propagation in realize. >>> >>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>> Fixes: a2875e6f98 "s390x/kvm: implement floating-interrupt controller device" >>> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> >>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com> >>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>> --- >>> hw/intc/s390_flic_kvm.c | 14 +++++++++++--- >>> 1 file changed, 11 insertions(+), 3 deletions(-) >>> >> >> (...) >> >>> cd.type = KVM_DEV_TYPE_FLIC; >>> ret = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd); >>> if (ret < 0) { >>> - trace_flic_create_device(errno); >>> - return; >>> + error_setg_errno(&errp_local, errno, "Creating the KVM device failed"); >>> + trace_flic_no_device_api(errno); >> >> Err... this should still be trace_flic_create_device(), no? > > I'm afraid you are right! Probably a copy paste error :/ > Do you think the traces are still appropriate once we have proper error propagation? I did not feel comfortable removing them but thinking again, than might be the thing to do. @Christian: I think we should really fix this the one way or the other. Can you tell me what is the proper procedure? >> >>> + goto fail; >>> } >>> flic_state->fd = cd.fd; >> > >
On Tue, 4 Jul 2017 17:08:52 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > >>> cd.type = KVM_DEV_TYPE_FLIC; > >>> ret = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd); > >>> if (ret < 0) { > >>> - trace_flic_create_device(errno); > >>> - return; > >>> + error_setg_errno(&errp_local, errno, "Creating the KVM device failed"); > >>> + trace_flic_no_device_api(errno); > >> > >> Err... this should still be trace_flic_create_device(), no? > > > > I'm afraid you are right! Probably a copy paste error :/ > > > > Do you think the traces are still appropriate once we have > proper error propagation? They add less value, but I'd just keep them. > > I did not feel comfortable removing them but thinking again, > than might be the thing to do. > > @Christian: > I think we should really fix this the one way or the other. > Can you tell me what is the proper procedure? I'd vote for just fixing the trace event. Smaller change, and we can revisit this later.
On 07/04/2017 06:59 PM, Cornelia Huck wrote: > On Tue, 4 Jul 2017 17:08:52 +0200 > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > >>>>> cd.type = KVM_DEV_TYPE_FLIC; >>>>> ret = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd); >>>>> if (ret < 0) { >>>>> - trace_flic_create_device(errno); >>>>> - return; >>>>> + error_setg_errno(&errp_local, errno, "Creating the KVM device failed"); >>>>> + trace_flic_no_device_api(errno); >>>> >>>> Err... this should still be trace_flic_create_device(), no? >>> >>> I'm afraid you are right! Probably a copy paste error :/ >>> >> >> Do you think the traces are still appropriate once we have >> proper error propagation? > > They add less value, but I'd just keep them. > >> >> I did not feel comfortable removing them but thinking again, >> than might be the thing to do. >> >> @Christian: >> I think we should really fix this the one way or the other. >> Can you tell me what is the proper procedure? > > I'd vote for just fixing the trace event. Smaller change, and we can > revisit this later. > +1
diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c index b4c61d8..bea3997 100644 --- a/hw/intc/s390_flic_kvm.c +++ b/hw/intc/s390_flic_kvm.c @@ -15,6 +15,7 @@ #include "cpu.h" #include <sys/ioctl.h> #include "qemu/error-report.h" +#include "qapi/error.h" #include "hw/sysbus.h" #include "sysemu/kvm.h" #include "hw/s390x/s390_flic.h" @@ -397,18 +398,22 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error **errp) struct kvm_create_device cd = {0}; struct kvm_device_attr test_attr = {0}; int ret; + Error *errp_local = NULL; flic_state->fd = -1; if (!kvm_check_extension(kvm_state, KVM_CAP_DEVICE_CTRL)) { + error_setg_errno(&errp_local, errno, "KVM is missing capability" + " KVM_CAP_DEVICE_CTRL"); trace_flic_no_device_api(errno); - return; + goto fail; } cd.type = KVM_DEV_TYPE_FLIC; ret = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd); if (ret < 0) { - trace_flic_create_device(errno); - return; + error_setg_errno(&errp_local, errno, "Creating the KVM device failed"); + trace_flic_no_device_api(errno); + goto fail; } flic_state->fd = cd.fd; @@ -417,6 +422,9 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error **errp) flic_state->clear_io_supported = !ioctl(flic_state->fd, KVM_HAS_DEVICE_ATTR, test_attr); + return; +fail: + error_propagate(errp, errp_local); } static void kvm_s390_flic_reset(DeviceState *dev)