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 14:55, schrieb 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!
>
> Tweaking as follows:

I'm fine with your tweaks, both here and in 3/4.  Thanks!