Re: [Qemu-devel] [PATCH v2 4/8] qom: interpret the return value when setting legacy properties

[Paolo Bonzini <pbonzini@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 636 bytes --]

On 12/16/2011 06:00 PM, Anthony Liguori wrote:
>
> And perhaps it would make more sense to return Error * and make the
> function name be a constructor:
>
> Error *error_from_qdev_prop_err(int ret, DeviceState *dev,
>                                  Property *prop, const char *value);

That doesn't work too well when calling it from setters.  However, 
error_set_from_qdev_prop_error is definitely an improvement (no matter 
what a mouthful it is).

I need to rush now.  I placed this at git://github.com/qemu/bonzini.git 
qom-props (commit 263e8c5), so you can play on top of it or pull it.  I 
attach the interdiff from v2.

Paolo

[-- Attachment #2: interdiff.patch --]
[-- Type: text/x-patch, Size: 3619 bytes --]

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index dd41e5a..80115b3 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -644,13 +644,11 @@ static void set_generic(DeviceState *dev, Visitor *v, void *opaque,
     }
     if (!*str) {
         g_free(str);
-        qdev_prop_error(errp, EINVAL, dev, prop, str);
+        error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
         return;
     }
     ret = prop->info->parse(dev, prop, str);
-    if (ret != 0) {
-        qdev_prop_error(errp, ret, dev, prop, str);
-    }
+    error_set_from_qdev_prop_error(errp, ret, dev, prop, str);
     g_free(str);
 }
 
@@ -973,8 +972,8 @@ int qdev_prop_exists(DeviceState *dev, const char *name)
     return qdev_prop_find(dev, name) ? true : false;
 }
 
-void qdev_prop_error(Error **errp, int ret,
-                     DeviceState *dev, Property *prop, const char *value)
+void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
+                                    Property *prop, const char *value)
 {
     switch (ret) {
     case -EEXIST:
@@ -990,6 +989,10 @@ void qdev_prop_error(Error **errp, int ret,
         error_set(errp, QERR_PROPERTY_VALUE_NOT_FOUND,
                   dev->info->name, prop->name, value);
         break;
+    case 0:
+        break;
+    default:
+        abort();
     }
 }
 
@@ -1012,7 +1015,7 @@ int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
     ret = prop->info->parse(dev, prop, value);
     if (ret < 0) {
         Error *err;
-        qdev_prop_error(&err, ret, dev, prop, value);
+        error_set_from_qdev_prop_error(&err, ret, dev, prop, value);
         qerror_report_err(err);
         error_free(err);
         return -1;
diff --git a/hw/qdev.c b/hw/qdev.c
index 2f7defc..0465632 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1169,9 +1169,7 @@ static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
     }
 
     ret = prop->info->parse(dev, prop, ptr);
-    if (ret != 0) {
-        qdev_prop_error(errp, ret, dev, prop, ptr);
-    }
+    error_set_from_qdev_prop_error(errp, ret, dev, prop, ptr);
     g_free(ptr);
 }
 
@@ -1179,7 +1177,7 @@ static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
  * @qdev_add_legacy_property - adds a legacy property
  *
  * Do not use this is new code!  Properties added through this interface will
- * be given names and types in the "legacy<>" type namespace.
+ * be given names and types in the "legacy" namespace.
  *
  * Legacy properties are always processed as strings.  The format of the string
  * depends on the property type.
@@ -1189,7 +1187,7 @@ void qdev_property_add_legacy(DeviceState *dev, Property *prop,
 {
     gchar *name, *type;
 
-    name = g_strdup_printf("legacy<%s>", prop->name);
+    name = g_strdup_printf("legacy-%s", prop->name);
     type = g_strdup_printf("legacy<%s>",
                            prop->info->legacy_name ?: prop->info->name);
 
diff --git a/hw/qdev.h b/hw/qdev.h
index 3410e77..d5896be 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -375,8 +375,8 @@ void qdev_prop_set_defaults(DeviceState *dev, Property *props);
 
 void qdev_prop_register_global_list(GlobalProperty *props);
 void qdev_prop_set_globals(DeviceState *dev);
-void qdev_prop_error(Error **errp, int ret, DeviceState *name,
-                     Property *prop, const char *value);
+void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
+                                    Property *prop, const char *value);
 
 static inline const char *qdev_fw_name(DeviceState *dev)
 {