Re: [Qemu-devel] [PATCH 4/4] arm: Clean up fragile use of error_is_set() in realize() methods

[Markus Armbruster]

Andreas Färber <address@hidden> 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 <address@hidden>
>> ---
>>  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 :)

[...]