Re: [Qemu-devel] [PATCH 3/4] qom: Clean up fragile use of error_is_set() in set() 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 ObjectProperty set() 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>

Tweaking as follows:

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 5c08611..2e21eba 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -746,11 +746,11 @@ static void set_prop_arraylen(Object *obj, Visitor
*v, void *opaque,
      * array-length field in the device struct, we have to create the
      * array itself and dynamically add the corresponding properties.
      */
-    Error *local_err = NULL;
     DeviceState *dev = DEVICE(obj);
     Property *prop = opaque;
     uint32_t *alenptr = qdev_get_prop_ptr(dev, prop);
     void **arrayptr = (void *)dev + prop->arrayoffset;
+    Error *local_err = NULL;
     void *eltptr;
     const char *arrayname;
     int i;
diff --git a/hw/misc/tmp105.c b/hw/misc/tmp105.c
index e11657d..636ee97 100644
--- a/hw/misc/tmp105.c
+++ b/hw/misc/tmp105.c
@@ -67,8 +67,8 @@ static void tmp105_get_temperature(Object *obj,
Visitor *v, void *opaque,
 static void tmp105_set_temperature(Object *obj, Visitor *v, void *opaque,
                                    const char *name, Error **errp)
 {
-    Error *local_err = NULL;
     TMP105State *s = TMP105(obj);
+    Error *local_err = NULL;
     int64_t temp;

     visit_type_int(v, &temp, name, &local_err);
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 51f02eb..971a921 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -141,8 +141,8 @@ static void balloon_stats_set_poll_interval(Object
*obj, struct Visitor *v,
                                             void *opaque, const char *name,
                                             Error **errp)
 {
-    Error *local_err = NULL;
     VirtIOBalloon *s = opaque;
+    Error *local_err = NULL;
     int64_t value;

     visit_type_int(v, &value, name, &local_err);
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 73954a7..8f193a9 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1296,11 +1296,11 @@ static void x86_cpuid_version_get_family(Object
*obj, Visitor *v, void *opaque,
 static void x86_cpuid_version_set_family(Object *obj, Visitor *v, void
*opaque,
                                          const char *name, Error **errp)
 {
-    Error *local_err = NULL;
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
     const int64_t min = 0;
     const int64_t max = 0xff + 0xf;
+    Error *local_err = NULL;
     int64_t value;

     visit_type_int(v, &value, name, &local_err);
@@ -1337,11 +1337,11 @@ static void x86_cpuid_version_get_model(Object
*obj, Visitor *v, void *opaque,
 static void x86_cpuid_version_set_model(Object *obj, Visitor *v, void
*opaque,
                                         const char *name, Error **errp)
 {
-    Error *local_err = NULL;
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
     const int64_t min = 0;
     const int64_t max = 0xff;
+    Error *local_err = NULL;
     int64_t value;

     visit_type_int(v, &value, name, &local_err);
@@ -1375,11 +1375,11 @@ static void
x86_cpuid_version_set_stepping(Object *obj, Visitor *v,
                                            void *opaque, const char *name,
                                            Error **errp)
 {
-    Error *local_err = NULL;
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
     const int64_t min = 0;
     const int64_t max = 0xf;
+    Error *local_err = NULL;
     int64_t value;

     visit_type_int(v, &value, name, &local_err);
@@ -1514,10 +1514,10 @@ static void x86_cpuid_get_tsc_freq(Object *obj,
Visitor *v, void *opaque,
 static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
                                    const char *name, Error **errp)
 {
-    Error *local_err = NULL;
     X86CPU *cpu = X86_CPU(obj);
     const int64_t min = 0;
     const int64_t max = INT64_MAX;
+    Error *local_err = NULL;
     int64_t value;

     visit_type_int(v, &value, name, &local_err);

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