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 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?

Otherwise both patches look fine, thanks for your efforts!

I do wonder, and maybe Peter can comment as native speaker, whether it
should be "uses" (plural) or "usage" in both subjects?

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