Andreas Färber <afaer...@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 <arm...@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 :) [...]