| Message ID | 1398422663-13615-5-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 25.04.2014 12:44, schrieb Markus Armbruster: > Using error_is_set(ERRP) to find out whether a function failed is > either wrong, fragile, or unnecessarily opaque. It's wrong when ERRP > may be null, because errors go undetected when it is. It's fragile > when proving ERRP non-null involves a non-local argument. Else, it's > unnecessarily opaque (see commit 84d18f0). > > I guess the error_is_set(errp) in the DeviceClass realize() methods > are merely fragile right now, because I can't find a call chain that > passes a null errp argument. > > Make the code more robust and more obviously correct: receive the > error in a local variable, then propagate it through the parameter. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hw/intc/arm_gic.c | 6 ++++-- > hw/intc/arm_gic_kvm.c | 6 ++++-- > hw/intc/armv7m_nvic.c | 6 ++++-- > 3 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > index 955b8d4..f6c7144 100644 > --- a/hw/intc/arm_gic.c > +++ b/hw/intc/arm_gic.c > @@ -793,13 +793,15 @@ void gic_init_irqs_and_distributor(GICState *s, int num_irq) > static void arm_gic_realize(DeviceState *dev, Error **errp) > { > /* Device instance realize function for the GIC sysbus device */ > + Error *local_err = NULL; > int i; > GICState *s = ARM_GIC(dev); > SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > ARMGICClass *agc = ARM_GIC_GET_CLASS(s); > [snip] Here and in the previous patch I'm not so thrilled about placing this new variable first. We've usually been using the system of casting the arguments first, from base type to most specific type, then any "local" variables in the end. Here, i breaks the system already, not your fault, but in the next hunk there's an int ret as final variable or int64_t temp in TMP105. Can we (me if through my tree) move them down? Otherwise both patches look fine, thanks for your efforts! I do wonder, and maybe Peter can comment as native speaker, whether it should be "uses" (plural) or "usage" in both subjects? Regards, Andreas
On 25 April 2014 12:25, Andreas Färber <afaerber@suse.de> wrote: > I do wonder, and maybe Peter can comment as native speaker, whether it > should be "uses" (plural) or "usage" in both subjects? In this subject I would consider all of "use", "uses" and "usage" as acceptable variations. thanks -- PMM
Andreas Färber <afaerber@suse.de> writes: > Am 25.04.2014 12:44, schrieb Markus Armbruster: >> Using error_is_set(ERRP) to find out whether a function failed is >> either wrong, fragile, or unnecessarily opaque. It's wrong when ERRP >> may be null, because errors go undetected when it is. It's fragile >> when proving ERRP non-null involves a non-local argument. Else, it's >> unnecessarily opaque (see commit 84d18f0). >> >> I guess the error_is_set(errp) in the DeviceClass realize() methods >> are merely fragile right now, because I can't find a call chain that >> passes a null errp argument. >> >> Make the code more robust and more obviously correct: receive the >> error in a local variable, then propagate it through the parameter. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> hw/intc/arm_gic.c | 6 ++++-- >> hw/intc/arm_gic_kvm.c | 6 ++++-- >> hw/intc/armv7m_nvic.c | 6 ++++-- >> 3 files changed, 12 insertions(+), 6 deletions(-) >> >> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c >> index 955b8d4..f6c7144 100644 >> --- a/hw/intc/arm_gic.c >> +++ b/hw/intc/arm_gic.c >> @@ -793,13 +793,15 @@ void gic_init_irqs_and_distributor(GICState *s, int num_irq) >> static void arm_gic_realize(DeviceState *dev, Error **errp) >> { >> /* Device instance realize function for the GIC sysbus device */ >> + Error *local_err = NULL; >> int i; >> GICState *s = ARM_GIC(dev); >> SysBusDevice *sbd = SYS_BUS_DEVICE(dev); >> ARMGICClass *agc = ARM_GIC_GET_CLASS(s); >> > [snip] > > Here and in the previous patch I'm not so thrilled about placing this > new variable first. We've usually been using the system of casting the > arguments first, from base type to most specific type, then any "local" > variables in the end. Here, i breaks the system already, not your fault, > but in the next hunk there's an int ret as final variable or int64_t > temp in TMP105. Can we (me if through my tree) move them down? Sure! > Otherwise both patches look fine, thanks for your efforts! Thanks for the quick review :) [...]
On 25 April 2014 11:44, Markus Armbruster <armbru@redhat.com> wrote: > Using error_is_set(ERRP) to find out whether a function failed is > either wrong, fragile, or unnecessarily opaque. It's wrong when ERRP > may be null, because errors go undetected when it is. It's fragile > when proving ERRP non-null involves a non-local argument. Else, it's > unnecessarily opaque (see commit 84d18f0). > > I guess the error_is_set(errp) in the DeviceClass realize() methods > are merely fragile right now, because I can't find a call chain that > passes a null errp argument. > > Make the code more robust and more obviously correct: receive the > error in a local variable, then propagate it through the parameter. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> Acked-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index 955b8d4..f6c7144 100644 --- a/hw/intc/arm_gic.c +++ b/hw/intc/arm_gic.c @@ -793,13 +793,15 @@ void gic_init_irqs_and_distributor(GICState *s, int num_irq) static void arm_gic_realize(DeviceState *dev, Error **errp) { /* Device instance realize function for the GIC sysbus device */ + Error *local_err = NULL; int i; GICState *s = ARM_GIC(dev); SysBusDevice *sbd = SYS_BUS_DEVICE(dev); ARMGICClass *agc = ARM_GIC_GET_CLASS(s); - agc->parent_realize(dev, errp); - if (error_is_set(errp)) { + agc->parent_realize(dev, &local_err); + if (local_err) { + error_propagate(errp, local_err); return; } diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c index 719d227..35d79a6 100644 --- a/hw/intc/arm_gic_kvm.c +++ b/hw/intc/arm_gic_kvm.c @@ -513,14 +513,16 @@ static void kvm_arm_gic_reset(DeviceState *dev) static void kvm_arm_gic_realize(DeviceState *dev, Error **errp) { + Error *local_err = NULL; int i; GICState *s = KVM_ARM_GIC(dev); SysBusDevice *sbd = SYS_BUS_DEVICE(dev); KVMARMGICClass *kgc = KVM_ARM_GIC_GET_CLASS(s); int ret; - kgc->parent_realize(dev, errp); - if (error_is_set(errp)) { + kgc->parent_realize(dev, &local_err); + if (local_err) { + error_propagate(errp, local_err); return; } diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c index 6066fa6..0d79def 100644 --- a/hw/intc/armv7m_nvic.c +++ b/hw/intc/armv7m_nvic.c @@ -472,6 +472,7 @@ static void armv7m_nvic_reset(DeviceState *dev) static void armv7m_nvic_realize(DeviceState *dev, Error **errp) { + Error *local_err = NULL; nvic_state *s = NVIC(dev); NVICClass *nc = NVIC_GET_CLASS(s); @@ -480,8 +481,9 @@ static void armv7m_nvic_realize(DeviceState *dev, Error **errp) /* Tell the common code we're an NVIC */ s->gic.revision = 0xffffffff; s->num_irq = s->gic.num_irq; - nc->parent_realize(dev, errp); - if (error_is_set(errp)) { + nc->parent_realize(dev, &local_err); + if (local_err) { + error_propagate(errp, local_err); return; } gic_init_irqs_and_distributor(&s->gic, s->num_irq);
Using error_is_set(ERRP) to find out whether a function failed is either wrong, fragile, or unnecessarily opaque. It's wrong when ERRP may be null, because errors go undetected when it is. It's fragile when proving ERRP non-null involves a non-local argument. Else, it's unnecessarily opaque (see commit 84d18f0). I guess the error_is_set(errp) in the DeviceClass realize() methods are merely fragile right now, because I can't find a call chain that passes a null errp argument. Make the code more robust and more obviously correct: receive the error in a local variable, then propagate it through the parameter. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- hw/intc/arm_gic.c | 6 ++++-- hw/intc/arm_gic_kvm.c | 6 ++++-- hw/intc/armv7m_nvic.c | 6 ++++-- 3 files changed, 12 insertions(+), 6 deletions(-)