Re: [Qemu-devel] [PATCH 3/7] s390x: fix error propagation in kvm-flic's realize
On 07/04/2017 06:59 PM, Cornelia Huck wrote: > On Tue, 4 Jul 2017 17:08:52 +0200 > Halil Pasic 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
Re: [Qemu-devel] [PATCH 3/7] s390x: fix error propagation in kvm-flic's realize
On Tue, 4 Jul 2017 17:08:52 +0200 Halil Pasic 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.
Re: [Qemu-devel] [PATCH 3/7] s390x: fix error propagation in kvm-flic's realize
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 wrote: >> >>> From: Halil Pasic >>> >>> 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 >>> Fixes: a2875e6f98 "s390x/kvm: implement floating-interrupt controller >>> device" >>> Reviewed-by: Dong Jia Shi >>> Reviewed-by: Yi Min Zhao >>> Signed-off-by: Christian Borntraeger >>> --- >>> 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; >> > >
Re: [Qemu-devel] [PATCH 3/7] s390x: fix error propagation in kvm-flic's realize
On 07/04/2017 04:31 PM, Cornelia Huck wrote: > On Tue, 4 Jul 2017 16:07:55 +0200 > Christian Borntraeger wrote: > >> From: Halil Pasic >> >> 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 >> Fixes: a2875e6f98 "s390x/kvm: implement floating-interrupt controller device" >> Reviewed-by: Dong Jia Shi >> Reviewed-by: Yi Min Zhao >> Signed-off-by: Christian Borntraeger >> --- >> 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; >
Re: [Qemu-devel] [PATCH 3/7] s390x: fix error propagation in kvm-flic's realize
On Tue, 4 Jul 2017 16:07:55 +0200 Christian Borntraeger wrote: > From: Halil Pasic > > 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 > Fixes: a2875e6f98 "s390x/kvm: implement floating-interrupt controller device" > Reviewed-by: Dong Jia Shi > Reviewed-by: Yi Min Zhao > Signed-off-by: Christian Borntraeger > --- > 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;