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

[Andreas Färber]

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:

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index f6c7144..1532ef9 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -793,11 +793,11 @@ 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);
+    Error *local_err = NULL;

     agc->parent_realize(dev, &local_err);
     if (local_err) {
diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index 35d79a6..5038885 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -513,11 +513,11 @@ 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);
+    Error *local_err = NULL;
     int ret;

     kgc->parent_realize(dev, &local_err);
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 0d79def..ef9a352 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -472,9 +472,9 @@ 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);
+    Error *local_err = NULL;

     /* The NVIC always has only one CPU */
     s->gic.num_cpu = 1;

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg