Andreas Färber <afaer...@suse.de> writes:
> Am 25.04.2014 14:55, schrieb Markus Armbruster:
>> 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!
>
> Tweaking as follows:
I'm fine with your tweaks, both here and in 3/4. Thanks!
