Patchwork [02/18] qom: register legacy properties as new style properties

login
register
mail settings
Submitter Anthony Liguori
Date Nov. 30, 2011, 9:03 p.m.
Message ID <1322687028-29714-3-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/128587/
State New
Headers show

Comments

Anthony Liguori - Nov. 30, 2011, 9:03 p.m.
Expose all legacy properties through the new QOM property mechanism.  The qdev
property types are exposed through the 'legacy<>' namespace.  They are always
visited as strings since they do their own string parsing.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |   82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h |    7 +++++
 2 files changed, 89 insertions(+), 0 deletions(-)
Stefan Hajnoczi - Dec. 1, 2011, 8:33 a.m.
On Wed, Nov 30, 2011 at 03:03:32PM -0600, Anthony Liguori wrote:
> +static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
> +                                     const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +
> +    if (dev->state != DEV_STATE_CREATED) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +
> +    if (prop->info->parse) {
> +        Error *local_err = NULL;
> +        char *ptr = NULL;
> +
> +        visit_type_str(v, &ptr, name, &local_err);
> +        if (!local_err) {
> +            int ret;
> +            ret = prop->info->parse(dev, prop, ptr);
> +            if (ret != 0) {
> +                error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> +                          name, prop->info->name);
> +            }
> +            g_free(ptr);
> +        } else {
> +            error_propagate(errp, local_err);
> +        }

I noticed something subtle about Error** here.  Your code is correct but
I (incorrectly) wanted to eliminate local_err and use errp directly:

if (prop->info->parse) {
    char *ptr = NULL;

    visit_type_str(v, &ptr, name, errp);
    if (!error_is_set(errp)) {
        int ret;
	ret = prop->info->parse(dev, prop, ptr);
	if (ret != 0) {
	    error_set(errp, QERR_INVALID_PARAMETER_VALUE,
	              name, prop->info->name);
	}
	g_free(ptr);
    }
} else {
...

Turns out you cannot do this because error_is_set(errp) will be false if
the caller passed in a NULL errp.  That means we wouldn't detect the
error from visit_type_str()!

So it's not okay to depend on the caller's errp.  We always need to
juggle around a local_err with propagation :(.

Just wanted to share.
Anthony Liguori - Dec. 1, 2011, 1:31 p.m.
On 12/01/2011 02:33 AM, Stefan Hajnoczi wrote:
> On Wed, Nov 30, 2011 at 03:03:32PM -0600, Anthony Liguori wrote:
>> +static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
>> +                                     const char *name, Error **errp)
>> +{
>> +    Property *prop = opaque;
>> +
>> +    if (dev->state != DEV_STATE_CREATED) {
>> +        error_set(errp, QERR_PERMISSION_DENIED);
>> +        return;
>> +    }
>> +
>> +    if (prop->info->parse) {
>> +        Error *local_err = NULL;
>> +        char *ptr = NULL;
>> +
>> +        visit_type_str(v,&ptr, name,&local_err);
>> +        if (!local_err) {
>> +            int ret;
>> +            ret = prop->info->parse(dev, prop, ptr);
>> +            if (ret != 0) {
>> +                error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> +                          name, prop->info->name);
>> +            }
>> +            g_free(ptr);
>> +        } else {
>> +            error_propagate(errp, local_err);
>> +        }
>
> I noticed something subtle about Error** here.  Your code is correct but
> I (incorrectly) wanted to eliminate local_err and use errp directly:
>
> if (prop->info->parse) {
>      char *ptr = NULL;
>
>      visit_type_str(v,&ptr, name, errp);
>      if (!error_is_set(errp)) {
>          int ret;
> 	ret = prop->info->parse(dev, prop, ptr);
> 	if (ret != 0) {
> 	    error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> 	              name, prop->info->name);
> 	}
> 	g_free(ptr);
>      }
> } else {
> ...
>
> Turns out you cannot do this because error_is_set(errp) will be false if
> the caller passed in a NULL errp.  That means we wouldn't detect the
> error from visit_type_str()!
>
> So it's not okay to depend on the caller's errp.  We always need to
> juggle around a local_err with propagation :(.
>
> Just wanted to share.

Yes, you are correct.  You see this idiom a lot in QAPI an also in glib code 
that uses GError.

Regards,

Anthony Liguori

>
Gerd Hoffmann - Dec. 1, 2011, 3:51 p.m.
Hi,

> +    for (prop = dev->info->props; prop && prop->name; prop++) {
> +        qdev_property_add_legacy(dev, prop, NULL);
> +    }

bus properties?

> +static void qdev_get_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
> +                                     const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +
> +    if (prop->info->print) {
> +        char buffer[1024];
> +        char *ptr = buffer;
> +
> +        prop->info->print(dev, prop, buffer, sizeof(buffer));
> +        visit_type_str(v, &ptr, name, errp);

I think you can look at prop->info->type here and do something more
clever at least for the bool + integer properties.

cheers,
  Gerd
Kevin Wolf - Dec. 1, 2011, 4:14 p.m.
Am 30.11.2011 22:03, schrieb Anthony Liguori:
> Expose all legacy properties through the new QOM property mechanism.  The qdev
> property types are exposed through the 'legacy<>' namespace.  They are always
> visited as strings since they do their own string parsing.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/qdev.c |   82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/qdev.h |    7 +++++
>  2 files changed, 89 insertions(+), 0 deletions(-)

Not directly related to this patch, but looking for the first time at
visitor code, visitors are clearly underdocumented.

> +static void qdev_get_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
> +                                     const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +
> +    if (prop->info->print) {
> +        char buffer[1024];
> +        char *ptr = buffer;
> +
> +        prop->info->print(dev, prop, buffer, sizeof(buffer));
> +        visit_type_str(v, &ptr, name, errp);

So a string output visitor (this is what it's called, right?) takes a
buffer on the stack that the visitor must not free...

> +    } else {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +    }
> +}
> +
> +static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
> +                                     const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +
> +    if (dev->state != DEV_STATE_CREATED) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +
> +    if (prop->info->parse) {
> +        Error *local_err = NULL;
> +        char *ptr = NULL;
> +
> +        visit_type_str(v, &ptr, name, &local_err);
> +        if (!local_err) {
> +            int ret;
> +            ret = prop->info->parse(dev, prop, ptr);
> +            if (ret != 0) {
> +                error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> +                          name, prop->info->name);
> +            }
> +            g_free(ptr);

...but a string input visitor creates a copy that must be freed.

Kind of surprising behaviour, and qapi-visit-core.h doesn't document it.

Kevin
Anthony Liguori - Dec. 2, 2011, 1:03 a.m.
On 12/01/2011 09:51 AM, Gerd Hoffmann wrote:
>    Hi,
>
>> +    for (prop = dev->info->props; prop&&  prop->name; prop++) {
>> +        qdev_property_add_legacy(dev, prop, NULL);
>> +    }
>
> bus properties?

Hrm, okay, I can fix that.  Thanks for pointing that out.

>
>> +static void qdev_get_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
>> +                                     const char *name, Error **errp)
>> +{
>> +    Property *prop = opaque;
>> +
>> +    if (prop->info->print) {
>> +        char buffer[1024];
>> +        char *ptr = buffer;
>> +
>> +        prop->info->print(dev, prop, buffer, sizeof(buffer));
>> +        visit_type_str(v,&ptr, name, errp);
>
> I think you can look at prop->info->type here and do something more
> clever at least for the bool + integer properties.

That might get a little tough because I want legacy<> types to be handled as 
strings.  I guess we could promote bool/int to non-legacy types.

Regards,

Anthony Liguori

>
> cheers,
>    Gerd
>
Anthony Liguori - Dec. 2, 2011, 1:05 a.m.
On 12/01/2011 10:14 AM, Kevin Wolf wrote:
> Am 30.11.2011 22:03, schrieb Anthony Liguori:
>> Expose all legacy properties through the new QOM property mechanism.  The qdev
>> property types are exposed through the 'legacy<>' namespace.  They are always
>> visited as strings since they do their own string parsing.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>>   hw/qdev.c |   82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   hw/qdev.h |    7 +++++
>>   2 files changed, 89 insertions(+), 0 deletions(-)
>
> Not directly related to this patch, but looking for the first time at
> visitor code, visitors are clearly underdocumented.

Yes.  I need to spend time documenting it.

>> +static void qdev_get_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
>> +                                     const char *name, Error **errp)
>> +{
>> +    Property *prop = opaque;
>> +
>> +    if (prop->info->print) {
>> +        char buffer[1024];
>> +        char *ptr = buffer;
>> +
>> +        prop->info->print(dev, prop, buffer, sizeof(buffer));
>> +        visit_type_str(v,&ptr, name, errp);
>
> So a string output visitor (this is what it's called, right?) takes a
> buffer on the stack that the visitor must not free...

An output visitor does not take ownership of the string pointer.  An input 
visitor transfers ownership of the returned string to the caller.

The semantics suck.  See my other comments about changing this behavior.  I 
think I'm convinced we need to change the visitor interface here.

>
>> +    } else {
>> +        error_set(errp, QERR_PERMISSION_DENIED);
>> +    }
>> +}
>> +
>> +static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
>> +                                     const char *name, Error **errp)
>> +{
>> +    Property *prop = opaque;
>> +
>> +    if (dev->state != DEV_STATE_CREATED) {
>> +        error_set(errp, QERR_PERMISSION_DENIED);
>> +        return;
>> +    }
>> +
>> +    if (prop->info->parse) {
>> +        Error *local_err = NULL;
>> +        char *ptr = NULL;
>> +
>> +        visit_type_str(v,&ptr, name,&local_err);
>> +        if (!local_err) {
>> +            int ret;
>> +            ret = prop->info->parse(dev, prop, ptr);
>> +            if (ret != 0) {
>> +                error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> +                          name, prop->info->name);
>> +            }
>> +            g_free(ptr);
>
> ...but a string input visitor creates a copy that must be freed.
>
> Kind of surprising behaviour, and qapi-visit-core.h doesn't document it.

Yes, I'll fix that (the lack of docs).

Regards,

Anthony Liguori

> Kevin
>
Gerd Hoffmann - Dec. 2, 2011, 12:19 p.m.
Hi,

>>> +        prop->info->print(dev, prop, buffer, sizeof(buffer));
>>> +        visit_type_str(v,&ptr, name, errp);
>>
>> I think you can look at prop->info->type here and do something more
>> clever at least for the bool + integer properties.
> 
> That might get a little tough because I want legacy<> types to be
> handled as strings.  I guess we could promote bool/int to non-legacy types.

Indeed.  For chardev and drive properties which will be some kind of
link<..> in the new world (correct?) it probably makes sense to keep
them as legacy<...>.

While being at it:  bus properties might need some more thinking here
too.  Partly they are used for physical addressing, so they will be
replaced by link<...> too I guess?  Some of them have special parsing
and will end up in legacy<...> anyway.  Some are plain integers though
(hda for example) ...

cheers,
  Gerd

Patch

diff --git a/hw/qdev.c b/hw/qdev.c
index ad2d44f..b890c9f 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -83,6 +83,7 @@  static DeviceInfo *qdev_find_info(BusInfo *bus_info, const char *name)
 static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
 {
     DeviceState *dev;
+    Property *prop;
 
     assert(bus->info == info->bus_info);
     dev = g_malloc0(info->size);
@@ -99,6 +100,11 @@  static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
     }
     dev->instance_id_alias = -1;
     dev->state = DEV_STATE_CREATED;
+
+    for (prop = dev->info->props; prop && prop->name; prop++) {
+        qdev_property_add_legacy(dev, prop, NULL);
+    }
+
     return dev;
 }
 
@@ -1061,3 +1067,79 @@  const char *qdev_property_get_type(DeviceState *dev, const char *name, Error **e
 
     return prop->type;
 }
+
+/**
+ * Legacy property handling
+ */
+
+static void qdev_get_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
+                                     const char *name, Error **errp)
+{
+    Property *prop = opaque;
+
+    if (prop->info->print) {
+        char buffer[1024];
+        char *ptr = buffer;
+
+        prop->info->print(dev, prop, buffer, sizeof(buffer));
+        visit_type_str(v, &ptr, name, errp);
+    } else {
+        error_set(errp, QERR_PERMISSION_DENIED);
+    }
+}
+
+static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
+                                     const char *name, Error **errp)
+{
+    Property *prop = opaque;
+
+    if (dev->state != DEV_STATE_CREATED) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    }
+
+    if (prop->info->parse) {
+        Error *local_err = NULL;
+        char *ptr = NULL;
+
+        visit_type_str(v, &ptr, name, &local_err);
+        if (!local_err) {
+            int ret;
+            ret = prop->info->parse(dev, prop, ptr);
+            if (ret != 0) {
+                error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                          name, prop->info->name);
+            }
+            g_free(ptr);
+        } else {
+            error_propagate(errp, local_err);
+        }
+    } else {
+        error_set(errp, QERR_PERMISSION_DENIED);
+    }
+}
+
+/**
+ * @qdev_add_legacy_property - adds a legacy property
+ *
+ * Do not use this is new code!  Properties added through this interface will
+ * be given types in the "legacy<>" type namespace.
+ *
+ * Legacy properties are always processed as strings.  The format of the string
+ * depends on the property type.
+ */
+void qdev_property_add_legacy(DeviceState *dev, Property *prop,
+                              Error **errp)
+{
+    gchar *type;
+
+    type = g_strdup_printf("legacy<%s>", prop->info->name);
+
+    qdev_property_add(dev, prop->name, type,
+                      qdev_get_legacy_property,
+                      qdev_set_legacy_property,
+                      NULL,
+                      prop, errp);
+
+    g_free(type);
+}
diff --git a/hw/qdev.h b/hw/qdev.h
index 0f23677..8ac9031 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -447,4 +447,11 @@  void qdev_property_set(DeviceState *dev, Visitor *v, const char *name,
 const char *qdev_property_get_type(DeviceState *dev, const char *name,
                                    Error **errp);
 
+/**
+ * @qdev_property_add_legacy - add a legacy @Property to a device
+ *
+ * DO NOT USE THIS IN NEW CODE!
+ */
+void qdev_property_add_legacy(DeviceState *dev, Property *prop, Error **errp);
+
 #endif