[Qemu-devel] [PATCH 00/18] qom: dynamic properties and composition tree

[Qemu-devel] [PATCH 00/18] qom: dynamic properties and composition tree

[Anthony Liguori]

This is a follow up to my previous series to get us started in the QOM
direction.  A few things are different this time around.  Most notably:

 1) Devices no longer have names.  Instead, path names are always used to
    identify devices.

 2) In order to support (1), dynamic properties are now supported.

 3) The concept of a "root device" has been introduced.  The root device is
    roughly modelling the motherboard of a machine.  This type is a container
    type and it's best to think of it as something like a PCB board I guess.

To try it out, here's an example session:

Launch:

anthony@titi:~/build/qemu$ x86_64-softmmu/qemu-system-x86_64 -hda ~/images/linux.img -snapshot -device virtio-balloon-pci,id=foo -qmp unix:/tmp/server.sock,server,nowait

Explore the object model:

anthony@titi:~/git/qemu/QMP$ ./qom-list /
peripheral/
i440fx/
anthony@titi:~/git/qemu/QMP$ ./qom-list /i440fx/
piix3/
anthony@titi:~/git/qemu/QMP$ ./qom-list /i440fx/piix3
rtc/
anthony@titi:~/git/qemu/QMP$ ./qom-list /i440fx/piix3/rtc
date
base_year
anthony@titi:~/git/qemu/QMP$ ./qom-get /i440fx/piix3/rtc.date
tm_sec: 33
tm_hour: 21
tm_mday: 30
tm_year: 111
tm_mon: 10
tm_min: 2
anthony@titi:~/git/qemu/QMP$ ./qom-get rtc.date
tm_sec: 38
tm_hour: 21
tm_mday: 30
tm_year: 111
tm_mon: 10
tm_min: 2
anthony@titi:~/git/qemu/QMP$ ./qom-list /peripheral
foo/
anthony@titi:~/git/qemu/QMP$ ./qom-list /peripheral/foo
event_idx
indirect_desc

Anthony Liguori (18):
  qom: add new dynamic property infrastructure based on Visitors
  qom: register legacy properties as new style properties
  qom: introduce root device
  qdev: provide an interface to return canonical path from root
  qdev: provide a path resolution
  qom: add child properties (composition)
  qom: add link properties
  qapi: allow a 'gen' key to suppress code generation
  qmp: add qom-list command
  qom: qom_{get,set} monitor commands
  qdev: add explicitly named devices to the root complex
  dev: add an anonymous peripheral container
  rtc: make piix3 set the rtc as a child
  rtc: add a dynamic property for retrieving the date
  qom: optimize qdev_get_canonical_path using a parent link
  Make qmp.py easier to use
  Add test tools
  qdev: split out QOM functions to separate files

 Makefile.objs            |    4 +-
 QMP/qmp.py               |    6 +
 QMP/qom-get              |   26 +++
 QMP/qom-list             |   30 ++++
 QMP/qom-set              |   21 +++
 hw/container.c           |   20 +++
 hw/mc146818rtc.c         |   27 +++
 hw/pc_piix.c             |   11 ++
 hw/piix_pci.c            |    3 +
 hw/qdev-qom.h            |  200 ++++++++++++++++++++++
 hw/qdev.c                |   63 +++++++-
 hw/qdev.h                |   44 +++++
 hw/qom.c                 |  414 ++++++++++++++++++++++++++++++++++++++++++++++
 monitor.h                |    4 +
 qapi-schema.json         |  107 ++++++++++++
 qerror.c                 |    4 +
 qerror.h                 |    3 +
 qmp-commands.hx          |   18 ++
 qmp.c                    |   94 +++++++++++
 scripts/qapi-commands.py |    1 +
 scripts/qapi-types.py    |    1 +
 21 files changed, 1098 insertions(+), 3 deletions(-)
 create mode 100755 QMP/qom-get
 create mode 100755 QMP/qom-list
 create mode 100755 QMP/qom-set
 create mode 100644 hw/container.c
 create mode 100644 hw/qdev-qom.h
 create mode 100644 hw/qom.c

-- 
1.7.4.1

[Qemu-devel] [PATCH 01/18] qom: add new dynamic property infrastructure based on Visitors

[Anthony Liguori]

qdev properties are settable only during construction and static to classes.
This isn't flexible enough for QOM.

This patch introduces a property interface for qdev that provides dynamic
properties that are tied to objects, instead of classes.  These properties are
Visitor based instead of string based too.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |   99 +++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h |  118 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qerror.c  |    4 ++
 qerror.h  |    3 ++
 4 files changed, 224 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 106407f..ad2d44f 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -390,12 +390,33 @@ void qdev_init_nofail(DeviceState *dev)
     }
 }
 
+static void qdev_property_del_all(DeviceState *dev)
+{
+    while (dev->properties) {
+        GSList *i = dev->properties;
+        DeviceProperty *prop = i->data;
+
+        dev->properties = i->next;
+
+        if (prop->release) {
+            prop->release(dev, prop->name, prop->opaque);
+        }
+
+        g_free(prop->name);
+        g_free(prop->type);
+        g_free(prop);
+        g_free(i);
+    }
+}
+
 /* Unlink device from bus and free the structure.  */
 void qdev_free(DeviceState *dev)
 {
     BusState *bus;
     Property *prop;
 
+    qdev_property_del_all(dev);
+
     if (dev->state == DEV_STATE_INITIALIZED) {
         while (dev->num_child_bus) {
             bus = QLIST_FIRST(&dev->child_bus);
@@ -962,3 +983,81 @@ char* qdev_get_fw_dev_path(DeviceState *dev)
 
     return strdup(path);
 }
+
+void qdev_property_add(DeviceState *dev, const char *name, const char *type,
+                       DevicePropertyEtter *get, DevicePropertyEtter *set,
+                       DevicePropertyRelease *release, void *opaque,
+                       Error **errp)
+{
+    DeviceProperty *prop = g_malloc0(sizeof(*prop));
+
+    prop->name = g_strdup(name);
+    prop->type = g_strdup(type);
+    prop->get = get;
+    prop->set = set;
+    prop->release = release;
+    prop->opaque = opaque;
+
+    dev->properties = g_slist_append(dev->properties, prop);
+}
+
+static DeviceProperty *qdev_property_find(DeviceState *dev, const char *name)
+{
+    GSList *i;
+
+    for (i = dev->properties; i; i = i->next) {
+        DeviceProperty *prop = i->data;
+
+        if (strcmp(prop->name, name) == 0) {
+            return prop;
+        }
+    }
+
+    return NULL;
+}
+
+void qdev_property_get(DeviceState *dev, Visitor *v, const char *name,
+                       Error **errp)
+{
+    DeviceProperty *prop = qdev_property_find(dev, name);
+
+    if (prop == NULL) {
+        error_set(errp, QERR_PROPERTY_NOT_FOUND, dev->id?:"", name);
+        return;
+    }
+
+    if (!prop->get) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+    } else {
+        prop->get(dev, v, prop->opaque, name, errp);
+    }
+}
+
+void qdev_property_set(DeviceState *dev, Visitor *v, const char *name,
+                       Error **errp)
+{
+    DeviceProperty *prop = qdev_property_find(dev, name);
+
+    if (prop == NULL) {
+        error_set(errp, QERR_PROPERTY_NOT_FOUND, dev->id?:"", name);
+        return;
+    }
+
+    if (!prop->set) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+    } else {
+        prop->set(dev, prop->opaque, v, name, errp);
+    }
+}
+
+const char *qdev_property_get_type(DeviceState *dev, const char *name, Error **errp)
+{
+    DeviceProperty *prop = qdev_property_find(dev, name);
+
+    if (prop == NULL) {
+        error_set(errp, QERR_PROPERTY_NOT_FOUND, dev->id?:"", name);
+        return NULL;
+    }
+
+    return prop->type;
+}
diff --git a/hw/qdev.h b/hw/qdev.h
index 36a4198..0f23677 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -5,6 +5,7 @@
 #include "qemu-queue.h"
 #include "qemu-char.h"
 #include "qemu-option.h"
+#include "qapi/qapi-visit-core.h"
 
 typedef struct Property Property;
 
@@ -27,6 +28,42 @@ enum {
     DEV_NVECTORS_UNSPECIFIED = -1,
 };
 
+/**
+ * @DevicePropertyEtter - called when trying to get/set a property
+ *
+ * @dev the device that owns the property
+ * @v the visitor that contains the property data
+ * @opaque the opaque registered with the property
+ * @name the name of the property
+ * @errp a pointer to an Error that is filled if getting/setting fails.
+ */
+typedef void (DevicePropertyEtter)(DeviceState *dev,
+                                   Visitor *v,
+                                   void *opaque,
+                                   const char *name,
+                                   Error **errp);
+
+/**
+ * @DevicePropertyRelease - called when a property is removed from a device
+ *
+ * @dev the device that owns the property
+ * @name the name of the property
+ * @oapque the opaque registered with the property
+ */
+typedef void (DevicePropertyRelease)(DeviceState *dev,
+                                     const char *name,
+                                     void *opaque);
+
+typedef struct DeviceProperty
+{
+    gchar *name;
+    gchar *type;
+    DevicePropertyEtter *set;
+    DevicePropertyEtter *get;
+    DevicePropertyRelease *release;
+    void *opaque;
+} DeviceProperty;
+
 /* This structure should not be accessed directly.  We declare it here
    so that it can be embedded in individual device state structures.  */
 struct DeviceState {
@@ -45,6 +82,7 @@ struct DeviceState {
     QTAILQ_ENTRY(DeviceState) sibling;
     int instance_id_alias;
     int alias_required_for_version;
+    GSList *properties;
 };
 
 typedef void (*bus_dev_printfn)(Monitor *mon, DeviceState *dev, int indent);
@@ -329,4 +367,84 @@ char *qdev_get_fw_dev_path(DeviceState *dev);
 /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
 extern struct BusInfo system_bus_info;
 
+/**
+ * @qdev_property_add - add a new property to a device
+ *
+ * @dev - the device to add a property to
+ *
+ * @name - the name of the property.  This can contain any character except for
+ *         a forward slash.  In general, you should use hyphens '-' instead of
+ *         underscores '_' when naming properties.
+ *
+ * @type - the type name of the property.  This namespace is pretty loosely
+ *         defined.  Sub namespaces are constructed by using a prefix and then
+ *         to angle brackets.  For instance, the type 'virtio-net-pci' in the
+ *         'link' namespace would be 'link<virtio-net-pci>'.
+ *
+ * @get - the getter to be called to read a property.  If this is NULL, then
+ *        the property cannot be read.
+ *
+ * @set - the setter to be called to write a property.  If this is NULL, then
+ *        the property cannot be written.
+ *
+ * @release - called when the property is removed from the device.  This is
+ *            meant to allow a property to free its opaque upon device
+ *            destruction.  This may be NULL.
+ *
+ * @opaque - this is user data passed to @get, @set, and @release
+ *
+ * @errp - returns an error if this function fails
+ */
+void qdev_property_add(DeviceState *dev, const char *name, const char *type,
+                       DevicePropertyEtter *get, DevicePropertyEtter *set,
+                       DevicePropertyRelease *release, void *opaque,
+                       Error **errp);
+
+
+/**
+ * @qdev_property_get - reads a property from a device
+ *
+ * @dev - the device
+ *
+ * @v - the visitor that will receive the property value.  This should be an
+ *      Output visitor and the data will be written with @name as the name.
+ *
+ * @name - the name of the property
+ *
+ * @errp - returns an error if this function fails
+ */
+void qdev_property_get(DeviceState *dev, Visitor *v, const char *name,
+                       Error **errp);
+
+/**
+ * @qdev_property_set - writes a property to a device
+ *
+ * @dev - the device
+ *
+ * @v - the visitor that will used to write the property value.  This should be
+ *      an Input visitor and the data will be first read with @name as the name
+ *      and then written as the property value.
+ *
+ * @name - the name of the property
+ *
+ * @errp - returns an error if this function fails
+ */
+void qdev_property_set(DeviceState *dev, Visitor *v, const char *name,
+                       Error **errp);
+
+/**
+ * @qdev_property_get_type - returns the type of a property
+ *
+ * @dev - the device
+ *
+ * @name - the name of the property
+ *
+ * @errp - returns an error if this function fails
+ *
+ * Returns:
+ *   The type name of the property.
+ */
+const char *qdev_property_get_type(DeviceState *dev, const char *name,
+                                   Error **errp);
+
 #endif
diff --git a/qerror.c b/qerror.c
index fdf62b9..dd0ee76 100644
--- a/qerror.c
+++ b/qerror.c
@@ -178,6 +178,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Could not open '%(filename)'",
     },
     {
+        .error_fmt = QERR_PERMISSION_DENIED,
+        .desc      = "Insufficient permission to perform this operation",
+    },
+    {
         .error_fmt = QERR_PROPERTY_NOT_FOUND,
         .desc      = "Property '%(device).%(property)' not found",
     },
diff --git a/qerror.h b/qerror.h
index 2d3d43b..2b1d743 100644
--- a/qerror.h
+++ b/qerror.h
@@ -150,6 +150,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_OPEN_FILE_FAILED \
     "{ 'class': 'OpenFileFailed', 'data': { 'filename': %s } }"
 
+#define QERR_PERMISSION_DENIED \
+    "{ 'class': 'PermissionDenied', 'data': {} }"
+
 #define QERR_PROPERTY_NOT_FOUND \
     "{ 'class': 'PropertyNotFound', 'data': { 'device': %s, 'property': %s } }"
 
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 01/18] qom: add new dynamic property infrastructure based on Visitors

[Stefan Hajnoczi]

On Wed, Nov 30, 2011 at 03:03:31PM -0600, Anthony Liguori wrote:
> +/**
> + * @DevicePropertyEtter - called when trying to get/set a property

An established term for this is an "accessor".  I've never heard "etter"
before and it looks like a typo on first sight ;).

> +/**
> + * @qdev_property_set - writes a property to a device
> + *
> + * @dev - the device
> + *
> + * @v - the visitor that will used to write the property value.  This should be

s/will used/will be used/

Re: [Qemu-devel] [PATCH 01/18] qom: add new dynamic property infrastructure based on Visitors

[Anthony Liguori]

On 12/01/2011 02:19 AM, Stefan Hajnoczi wrote:
> On Wed, Nov 30, 2011 at 03:03:31PM -0600, Anthony Liguori wrote:
>> +/**
>> + * @DevicePropertyEtter - called when trying to get/set a property
>
> An established term for this is an "accessor".  I've never heard "etter"
> before and it looks like a typo on first sight ;).
>
>> +/**
>> + * @qdev_property_set - writes a property to a device
>> + *
>> + * @dev - the device
>> + *
>> + * @v - the visitor that will used to write the property value.  This should be
>
> s/will used/will be used/

Ack.

Regards,

Anthony Liguori

>

Re: [Qemu-devel] [PATCH 01/18] qom: add new dynamic property infrastructure based on Visitors

[Kevin Wolf]

Am 30.11.2011 22:03, schrieb Anthony Liguori:
> qdev properties are settable only during construction and static to classes.
> This isn't flexible enough for QOM.
> 
> This patch introduces a property interface for qdev that provides dynamic
> properties that are tied to objects, instead of classes.  These properties are
> Visitor based instead of string based too.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/qdev.c |   99 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/qdev.h |  118 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qerror.c  |    4 ++
>  qerror.h  |    3 ++
>  4 files changed, 224 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 106407f..ad2d44f 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -390,12 +390,33 @@ void qdev_init_nofail(DeviceState *dev)
>      }
>  }
>  
> +static void qdev_property_del_all(DeviceState *dev)
> +{
> +    while (dev->properties) {
> +        GSList *i = dev->properties;
> +        DeviceProperty *prop = i->data;
> +
> +        dev->properties = i->next;
> +
> +        if (prop->release) {
> +            prop->release(dev, prop->name, prop->opaque);
> +        }
> +
> +        g_free(prop->name);
> +        g_free(prop->type);
> +        g_free(prop);
> +        g_free(i);
> +    }
> +}
> +
>  /* Unlink device from bus and free the structure.  */
>  void qdev_free(DeviceState *dev)
>  {
>      BusState *bus;
>      Property *prop;
>  
> +    qdev_property_del_all(dev);
> +
>      if (dev->state == DEV_STATE_INITIALIZED) {
>          while (dev->num_child_bus) {
>              bus = QLIST_FIRST(&dev->child_bus);
> @@ -962,3 +983,81 @@ char* qdev_get_fw_dev_path(DeviceState *dev)
>  
>      return strdup(path);
>  }
> +
> +void qdev_property_add(DeviceState *dev, const char *name, const char *type,
> +                       DevicePropertyEtter *get, DevicePropertyEtter *set,
> +                       DevicePropertyRelease *release, void *opaque,
> +                       Error **errp)

How about letting the caller pass in a DeviceProperty for improved
readability and usability? Instead of memorizing the order of currently
eight parameters (could probably become more in the future) you can use
proper C99 initializers then.

> @@ -45,6 +82,7 @@ struct DeviceState {
>      QTAILQ_ENTRY(DeviceState) sibling;
>      int instance_id_alias;
>      int alias_required_for_version;
> +    GSList *properties;
>  };

Why GSList instead of qemu-queue.h macros that would provide type safety?

I don't think a property can belong to multiple devices, can it?
qdev_property_add only refers to a single device, and nothing else adds
elements to the list.

Kevin

Re: [Qemu-devel] [PATCH 01/18] qom: add new dynamic property infrastructure based on Visitors

[Anthony Liguori]

On 12/01/2011 09:52 AM, Kevin Wolf wrote:
> Am 30.11.2011 22:03, schrieb Anthony Liguori:
>> qdev properties are settable only during construction and static to classes.
>> This isn't flexible enough for QOM.
>>
>> This patch introduces a property interface for qdev that provides dynamic
>> properties that are tied to objects, instead of classes.  These properties are
>> Visitor based instead of string based too.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>>   hw/qdev.c |   99 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   hw/qdev.h |  118 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   qerror.c  |    4 ++
>>   qerror.h  |    3 ++
>>   4 files changed, 224 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index 106407f..ad2d44f 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -390,12 +390,33 @@ void qdev_init_nofail(DeviceState *dev)
>>       }
>>   }
>>
>> +static void qdev_property_del_all(DeviceState *dev)
>> +{
>> +    while (dev->properties) {
>> +        GSList *i = dev->properties;
>> +        DeviceProperty *prop = i->data;
>> +
>> +        dev->properties = i->next;
>> +
>> +        if (prop->release) {
>> +            prop->release(dev, prop->name, prop->opaque);
>> +        }
>> +
>> +        g_free(prop->name);
>> +        g_free(prop->type);
>> +        g_free(prop);
>> +        g_free(i);
>> +    }
>> +}
>> +
>>   /* Unlink device from bus and free the structure.  */
>>   void qdev_free(DeviceState *dev)
>>   {
>>       BusState *bus;
>>       Property *prop;
>>
>> +    qdev_property_del_all(dev);
>> +
>>       if (dev->state == DEV_STATE_INITIALIZED) {
>>           while (dev->num_child_bus) {
>>               bus = QLIST_FIRST(&dev->child_bus);
>> @@ -962,3 +983,81 @@ char* qdev_get_fw_dev_path(DeviceState *dev)
>>
>>       return strdup(path);
>>   }
>> +
>> +void qdev_property_add(DeviceState *dev, const char *name, const char *type,
>> +                       DevicePropertyEtter *get, DevicePropertyEtter *set,
>> +                       DevicePropertyRelease *release, void *opaque,
>> +                       Error **errp)
>
> How about letting the caller pass in a DeviceProperty for improved
> readability and usability? Instead of memorizing the order of currently
> eight parameters (could probably become more in the future) you can use
> proper C99 initializers then.

Yeah, instead of taking a void *opaque, it could then just take the 
DeviceProperty and use container_of adding a good bit more type safety.  I like 
it, thanks for the suggestion.

>
>> @@ -45,6 +82,7 @@ struct DeviceState {
>>       QTAILQ_ENTRY(DeviceState) sibling;
>>       int instance_id_alias;
>>       int alias_required_for_version;
>> +    GSList *properties;
>>   };
>
> Why GSList instead of qemu-queue.h macros that would provide type safety?

You're clearly thwarting my attempts at slowly introducing GSList as a 
replacement for qemu-queue ;-)

I really dislike qemu-queue.  I think it's a whole lot more difficult to use in 
practice.  The glib data structures are much more rich than qemu-queue.

> I don't think a property can belong to multiple devices, can it?
> qdev_property_add only refers to a single device, and nothing else adds
> elements to the list.

Yes, you are correct.

Regards,

Anthony Liguori

>
> Kevin
>

Re: [Qemu-devel] [PATCH 01/18] qom: add new dynamic property infrastructure based on Visitors

[Kevin Wolf]

Am 02.12.2011 02:08, schrieb Anthony Liguori:
> On 12/01/2011 09:52 AM, Kevin Wolf wrote:
>> Am 30.11.2011 22:03, schrieb Anthony Liguori:
>>> qdev properties are settable only during construction and static to classes.
>>> This isn't flexible enough for QOM.
>>>
>>> This patch introduces a property interface for qdev that provides dynamic
>>> properties that are tied to objects, instead of classes.  These properties are
>>> Visitor based instead of string based too.
>>>
>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>> ---
>>>   hw/qdev.c |   99 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   hw/qdev.h |  118 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   qerror.c  |    4 ++
>>>   qerror.h  |    3 ++
>>>   4 files changed, 224 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/qdev.c b/hw/qdev.c
>>> index 106407f..ad2d44f 100644
>>> --- a/hw/qdev.c
>>> +++ b/hw/qdev.c
>>> @@ -390,12 +390,33 @@ void qdev_init_nofail(DeviceState *dev)
>>>       }
>>>   }
>>>
>>> +static void qdev_property_del_all(DeviceState *dev)
>>> +{
>>> +    while (dev->properties) {
>>> +        GSList *i = dev->properties;
>>> +        DeviceProperty *prop = i->data;
>>> +
>>> +        dev->properties = i->next;
>>> +
>>> +        if (prop->release) {
>>> +            prop->release(dev, prop->name, prop->opaque);
>>> +        }
>>> +
>>> +        g_free(prop->name);
>>> +        g_free(prop->type);
>>> +        g_free(prop);
>>> +        g_free(i);
>>> +    }
>>> +}
>>> +
>>>   /* Unlink device from bus and free the structure.  */
>>>   void qdev_free(DeviceState *dev)
>>>   {
>>>       BusState *bus;
>>>       Property *prop;
>>>
>>> +    qdev_property_del_all(dev);
>>> +
>>>       if (dev->state == DEV_STATE_INITIALIZED) {
>>>           while (dev->num_child_bus) {
>>>               bus = QLIST_FIRST(&dev->child_bus);
>>> @@ -962,3 +983,81 @@ char* qdev_get_fw_dev_path(DeviceState *dev)
>>>
>>>       return strdup(path);
>>>   }
>>> +
>>> +void qdev_property_add(DeviceState *dev, const char *name, const char *type,
>>> +                       DevicePropertyEtter *get, DevicePropertyEtter *set,
>>> +                       DevicePropertyRelease *release, void *opaque,
>>> +                       Error **errp)
>>
>> How about letting the caller pass in a DeviceProperty for improved
>> readability and usability? Instead of memorizing the order of currently
>> eight parameters (could probably become more in the future) you can use
>> proper C99 initializers then.
> 
> Yeah, instead of taking a void *opaque, it could then just take the 
> DeviceProperty and use container_of adding a good bit more type safety.  I like 
> it, thanks for the suggestion.
> 
>>
>>> @@ -45,6 +82,7 @@ struct DeviceState {
>>>       QTAILQ_ENTRY(DeviceState) sibling;
>>>       int instance_id_alias;
>>>       int alias_required_for_version;
>>> +    GSList *properties;
>>>   };
>>
>> Why GSList instead of qemu-queue.h macros that would provide type safety?
> 
> You're clearly thwarting my attempts at slowly introducing GSList as a 
> replacement for qemu-queue ;-)
> 
> I really dislike qemu-queue.  I think it's a whole lot more difficult to use in 
> practice.  The glib data structures are much more rich than qemu-queue.

qemu-queue.h is type safe, GSList is not. IMO that's a show stopper and
I can't understand why we even need to talk about it.

If you want to convince me of the opposite, it certainly needs more than
vague "easier to use" hand waving.

Kevin

Re: [Qemu-devel] [PATCH 01/18] qom: add new dynamic property infrastructure based on Visitors

[Anthony Liguori]

On 12/01/2011 09:52 AM, Kevin Wolf wrote:
> Am 30.11.2011 22:03, schrieb Anthony Liguori:
>> +
>> +void qdev_property_add(DeviceState *dev, const char *name, const char *type,
>> +                       DevicePropertyEtter *get, DevicePropertyEtter *set,
>> +                       DevicePropertyRelease *release, void *opaque,
>> +                       Error **errp)
>
> How about letting the caller pass in a DeviceProperty for improved
> readability and usability? Instead of memorizing the order of currently
> eight parameters (could probably become more in the future) you can use
> proper C99 initializers then.

This ends up making the code much more complex for the client if you try to 
eliminate the opaque and replace it with the structure.  It becomes necessary to 
do a dynamic allocation of the structure and then you also have to add a release 
function.

We could make the structure just contain the function pointers and not the 
opaque but that doesn't seem very helpful to me.  It just adds a few extra lines 
to the client code without a lot of gain.

Regards,

Anthony Liguori

>
>> @@ -45,6 +82,7 @@ struct DeviceState {
>>       QTAILQ_ENTRY(DeviceState) sibling;
>>       int instance_id_alias;
>>       int alias_required_for_version;
>> +    GSList *properties;
>>   };
>
> Why GSList instead of qemu-queue.h macros that would provide type safety?
>
> I don't think a property can belong to multiple devices, can it?
> qdev_property_add only refers to a single device, and nothing else adds
> elements to the list.
>
> Kevin
>

Re: [Qemu-devel] [PATCH 01/18] qom: add new dynamic property infrastructure based on Visitors

[Kevin Wolf]

Am 02.12.2011 19:47, schrieb Anthony Liguori:
> On 12/01/2011 09:52 AM, Kevin Wolf wrote:
>> Am 30.11.2011 22:03, schrieb Anthony Liguori:
>>> +
>>> +void qdev_property_add(DeviceState *dev, const char *name, const char *type,
>>> +                       DevicePropertyEtter *get, DevicePropertyEtter *set,
>>> +                       DevicePropertyRelease *release, void *opaque,
>>> +                       Error **errp)
>>
>> How about letting the caller pass in a DeviceProperty for improved
>> readability and usability? Instead of memorizing the order of currently
>> eight parameters (could probably become more in the future) you can use
>> proper C99 initializers then.
> 
> This ends up making the code much more complex for the client if you try to 
> eliminate the opaque and replace it with the structure.  It becomes necessary to 
> do a dynamic allocation of the structure and then you also have to add a release 
> function.

Hm, why doesn't static allocation work with it?

> We could make the structure just contain the function pointers and not the 
> opaque but that doesn't seem very helpful to me.  It just adds a few extra lines 
> to the client code without a lot of gain.

I keep switching back and forth between mails to find out what these
parameters are supposed to mean (especially the NULL ones), so yes, I
think even passing just the name and function pointers this way would
improve readability.

Kevin

[Qemu-devel] [PATCH 02/18] qom: register legacy properties as new style properties

[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(-)

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

Re: [Qemu-devel] [PATCH 02/18] qom: register legacy properties as new style properties

[Stefan Hajnoczi]

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.

Re: [Qemu-devel] [PATCH 02/18] qom: register legacy properties as new style properties

[Anthony Liguori]

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

>

Re: [Qemu-devel] [PATCH 02/18] qom: register legacy properties as new style properties

[Gerd Hoffmann]

  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

Re: [Qemu-devel] [PATCH 02/18] qom: register legacy properties as new style properties

[Anthony Liguori]

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
>

Re: [Qemu-devel] [PATCH 02/18] qom: register legacy properties as new style properties

[Gerd Hoffmann]

  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

Re: [Qemu-devel] [PATCH 02/18] qom: register legacy properties as new style properties

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 02/18] qom: register legacy properties as new style properties

[Anthony Liguori]

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
>

[Qemu-devel] [PATCH 03/18] qom: introduce root device

[Anthony Liguori]

This is based on Jan's suggestion for how to do unique naming.  The root device
is the root of composition.  All devices are reachable via child<> links from
this device.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 Makefile.objs  |    2 +-
 hw/container.c |   20 ++++++++++++++++++++
 hw/qdev.c      |   12 ++++++++++++
 hw/qdev.h      |    8 ++++++++
 4 files changed, 41 insertions(+), 1 deletions(-)
 create mode 100644 hw/container.c

diff --git a/Makefile.objs b/Makefile.objs
index d7a6539..10e794c 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -279,7 +279,7 @@ hw-obj-$(CONFIG_LSI_SCSI_PCI) += lsi53c895a.o
 hw-obj-$(CONFIG_ESP) += esp.o
 
 hw-obj-y += dma-helpers.o sysbus.o isa-bus.o
-hw-obj-y += qdev-addr.o
+hw-obj-y += qdev-addr.o container.o
 
 # VGA
 hw-obj-$(CONFIG_VGA_PCI) += vga-pci.o
diff --git a/hw/container.c b/hw/container.c
new file mode 100644
index 0000000..9cbf399
--- /dev/null
+++ b/hw/container.c
@@ -0,0 +1,20 @@
+#include "sysbus.h"
+
+static int container_initfn(SysBusDevice *dev)
+{
+    return 0;
+}
+
+static SysBusDeviceInfo container_info = {
+    .init = container_initfn,
+    .qdev.name = "container",
+    .qdev.size = sizeof(SysBusDevice),
+    .qdev.no_user = 1,
+};
+
+static void container_init(void)
+{
+    sysbus_register_withprop(&container_info);
+}
+
+device_init(container_init);
diff --git a/hw/qdev.c b/hw/qdev.c
index b890c9f..2db2d8e 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1143,3 +1143,15 @@ void qdev_property_add_legacy(DeviceState *dev, Property *prop,
 
     g_free(type);
 }
+
+DeviceState *qdev_get_root(void)
+{
+    static DeviceState *qdev_root;
+
+    if (!qdev_root) {
+        qdev_root = qdev_create(NULL, "container");
+        qdev_init_nofail(qdev_root);
+    }
+
+    return qdev_root;
+}
diff --git a/hw/qdev.h b/hw/qdev.h
index 8ac9031..79d691f 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -454,4 +454,12 @@ const char *qdev_property_get_type(DeviceState *dev, const char *name,
  */
 void qdev_property_add_legacy(DeviceState *dev, Property *prop, Error **errp);
 
+/**
+ * @qdev_get_root - returns the root device of the composition tree
+ *
+ * Returns:
+ *   The root of the composition tree.
+ */
+DeviceState *qdev_get_root(void);
+
 #endif
-- 
1.7.4.1

[Qemu-devel] [PATCH 04/18] qdev: provide an interface to return canonical path from root

[Anthony Liguori]

The canonical path is the path in the composition tree from the root to the
device.  This is effectively the name of the device.

This is an incredibly unefficient implementation that will be optimized in
a future patch.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h |    9 +++++++++
 2 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 2db2d8e..6b2b194 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1155,3 +1155,52 @@ DeviceState *qdev_get_root(void)
 
     return qdev_root;
 }
+
+static gchar *qdev_get_path_in(DeviceState *parent, DeviceState *dev)
+{
+    GSList *i;
+
+    if (parent == dev) {
+        return g_strdup("");
+    }
+
+    for (i = parent->properties; i; i = i->next) {
+        DeviceProperty *prop = i->data;
+        gchar *subpath;
+
+        if (!strstart(prop->type, "child<", NULL)) {
+            continue;
+        }
+
+        /* Check to see if the device is one of parent's children */
+        if (prop->opaque == dev) {
+            return g_strdup(prop->name);
+        }
+
+        /* Check to see if the device is a child of our child */
+        subpath = qdev_get_path_in(prop->opaque, dev);
+        if (subpath) {
+            gchar *path;
+
+            path = g_strdup_printf("%s/%s", prop->name, subpath);
+            g_free(subpath);
+
+            return path;
+        }
+    }
+
+    return NULL;
+}
+
+gchar *qdev_get_canonical_path(DeviceState *dev)
+{
+    gchar *path, *newpath;
+
+    path = qdev_get_path_in(qdev_get_root(), dev);
+    g_assert(path != NULL);
+
+    newpath = g_strdup_printf("/%s", path);
+    g_free(path);
+
+    return newpath;
+}
diff --git a/hw/qdev.h b/hw/qdev.h
index 79d691f..82e6d95 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -462,4 +462,13 @@ void qdev_property_add_legacy(DeviceState *dev, Property *prop, Error **errp);
  */
 DeviceState *qdev_get_root(void);
 
+/**
+ * @qdev_get_canonical_path - returns the canonical path for a device.  This
+ * is the path within the composition tree starting from the root.
+ *
+ * Returns:
+ *   The canonical path in the composition tree.
+ */
+gchar *qdev_get_canonical_path(DeviceState *dev);
+
 #endif
-- 
1.7.4.1

[Qemu-devel] [PATCH 05/18] qdev: provide a path resolution

[Anthony Liguori]

There are two types of supported paths--absolute paths and partial paths.

Absolute paths are derived from the root device and can follow child<> or
link<> properties.  Since they can follow link<> properties, they can be
arbitrarily long.  Absolute paths look like absolute filenames and are prefixed
with a leading slash.

Partial paths are look like relative filenames.  They do not begin with a
prefix.  The matching rules for partial paths are subtle but designed to make
specifying devices easy.  At each level of the composition tree, the partial
path is matched as an absolute path.  The first match is not returned.  At
least two matches are searched for.  A successful result is only returned if
only one match is founded.  If more than one match is found, a flag is returned
to indicate that the match was ambiguous.

At the end of the day, partial path support means that if you create a device
called 'ide0', you can just say 'ide0' as the path name and it will Just Work.
If we internally create a device called 'i440fx', you can just say 'i440fx' and
it will Just Work and long as you don't do anything silly.

A management tool should probably always use absolute paths since then they
don't have to deal with the possibility of ambiguity.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |  100 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h |   28 +++++++++++++++++
 2 files changed, 128 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 6b2b194..5bffbb7 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1204,3 +1204,103 @@ gchar *qdev_get_canonical_path(DeviceState *dev)
 
     return newpath;
 }
+
+static DeviceState *qdev_resolve_abs_path(DeviceState *parent,
+                                          gchar **parts,
+                                          int index)
+{
+    DeviceProperty *prop;
+    DeviceState *child;
+
+    if (parts[index] == NULL) {
+        return parent;
+    }
+
+    if (strcmp(parts[index], "") == 0) {
+        return qdev_resolve_abs_path(parent, parts, index + 1);
+    }
+
+    prop = qdev_property_find(parent, parts[index]);
+    if (prop == NULL) {
+        return NULL;
+    }
+
+    child = NULL;
+    if (strstart(prop->type, "link<", NULL)) {
+        DeviceState **pchild = prop->opaque;
+        if (*pchild) {
+            child = *pchild;
+        }
+    } else if (strstart(prop->type, "child<", NULL)) {
+        child = prop->opaque;
+    }
+
+    if (!child) {
+        return NULL;
+    }
+
+    return qdev_resolve_abs_path(child, parts, index + 1);
+}
+
+static DeviceState *qdev_resolve_partial_path(DeviceState *parent,
+                                              gchar **parts,
+                                              bool *ambiguous)
+{
+    DeviceState *dev;
+    GSList *i;
+
+    dev = qdev_resolve_abs_path(parent, parts, 0);
+
+    for (i = parent->properties; i; i = i->next) {
+        DeviceProperty *prop = i->data;
+        DeviceState *found;
+
+        if (!strstart(prop->type, "child<", NULL)) {
+            continue;
+        }
+
+        found = qdev_resolve_partial_path(prop->opaque, parts, ambiguous);
+        if (found) {
+            if (dev) {
+                if (ambiguous) {
+                    *ambiguous = true;
+                }
+                return NULL;
+            }
+            dev = found;
+        }
+
+        if (ambiguous && *ambiguous) {
+            return NULL;
+        }
+    }
+
+    return dev;
+}
+
+DeviceState *qdev_resolve_path(const char *path, bool *ambiguous)
+{
+    bool partial_path = true;
+    DeviceState *dev;
+    gchar **parts;
+
+    parts = g_strsplit(path, "/", 0);
+    if (parts == NULL || parts[0] == NULL) {
+        return qdev_get_root();
+    }
+
+    if (strcmp(parts[0], "") == 0) {
+        partial_path = false;
+    }
+
+    if (partial_path) {
+        dev = qdev_resolve_partial_path(qdev_get_root(), parts, ambiguous);
+    } else {
+        dev = qdev_resolve_abs_path(qdev_get_root(), parts, 1);
+    }
+
+    g_strfreev(parts);
+
+    return dev;
+}
+
diff --git a/hw/qdev.h b/hw/qdev.h
index 82e6d95..b8b62f5 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -471,4 +471,32 @@ DeviceState *qdev_get_root(void);
  */
 gchar *qdev_get_canonical_path(DeviceState *dev);
 
+/**
+ * @qdev_resolve_path - resolves a path returning a device
+ *
+ * There are two types of supported paths--absolute paths and partial paths.
+ * 
+ * Absolute paths are derived from the root device and can follow child<> or
+ * link<> properties.  Since they can follow link<> properties, they can be
+ * arbitrarily long.  Absolute paths look like absolute filenames and are prefix
+ * with a leading slash.
+ * 
+ * Partial paths are look like relative filenames.  They do not begin with a
+ * prefix.  The matching rules for partial paths are subtle but designed to make
+ * specifying devices easy.  At each level of the composition tree, the partial
+ * path is matched as an absolute path.  The first match is not returned.  At
+ * least two matches are searched for.  A successful result is only returned if
+ * only one match is founded.  If more than one match is found, a flag is return
+ * to indicate that the match was ambiguous.
+ *
+ * @path - the path to resolve
+ *
+ * @ambiguous - returns true if the path resolution failed because of an
+ *              ambiguous match
+ *
+ * Returns:
+ *   The matched device.
+ */
+DeviceState *qdev_resolve_path(const char *path, bool *ambiguous);
+
 #endif
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 05/18] qdev: provide a path resolution

[Stefan Hajnoczi]

On Wed, Nov 30, 2011 at 03:03:35PM -0600, Anthony Liguori wrote:
> +DeviceState *qdev_resolve_path(const char *path, bool *ambiguous)
> +{
> +    bool partial_path = true;
> +    DeviceState *dev;
> +    gchar **parts;
> +
> +    parts = g_strsplit(path, "/", 0);
> +    if (parts == NULL || parts[0] == NULL) {

We must g_strfreev(parts) in the parts[0] == NULL case.

> +/**
> + * @qdev_resolve_path - resolves a path returning a device
> + *
> + * There are two types of supported paths--absolute paths and partial paths.
> + * 
> + * Absolute paths are derived from the root device and can follow child<> or
> + * link<> properties.  Since they can follow link<> properties, they can be
> + * arbitrarily long.  Absolute paths look like absolute filenames and are prefix

s/prefix/prefixed/

> + * with a leading slash.
> + * 
> + * Partial paths are look like relative filenames.  They do not begin with a

s/are//

> + * prefix.  The matching rules for partial paths are subtle but designed to make
> + * specifying devices easy.  At each level of the composition tree, the partial
> + * path is matched as an absolute path.  The first match is not returned.  At
> + * least two matches are searched for.  A successful result is only returned if
> + * only one match is founded.  If more than one match is found, a flag is return

s/return/returned/

> + * to indicate that the match was ambiguous.
> + *
> + * @path - the path to resolve
> + *
> + * @ambiguous - returns true if the path resolution failed because of an
> + *              ambiguous match

The implementation seems to depend on ambiguous being initialized to
false by the caller.  That would be worth documenting or changing so it
does not depend on the initial value.

> + *
> + * Returns:
> + *   The matched device.

The matched device or NULL on path lookup failure.

Re: [Qemu-devel] [PATCH 05/18] qdev: provide a path resolution

[Anthony Liguori]

On 12/01/2011 04:24 AM, Stefan Hajnoczi wrote:
> On Wed, Nov 30, 2011 at 03:03:35PM -0600, Anthony Liguori wrote:
>> +DeviceState *qdev_resolve_path(const char *path, bool *ambiguous)
>> +{
>> +    bool partial_path = true;
>> +    DeviceState *dev;
>> +    gchar **parts;
>> +
>> +    parts = g_strsplit(path, "/", 0);
>> +    if (parts == NULL || parts[0] == NULL) {
>
> We must g_strfreev(parts) in the parts[0] == NULL case.

Good catch!

>
>> +/**
>> + * @qdev_resolve_path - resolves a path returning a device
>> + *
>> + * There are two types of supported paths--absolute paths and partial paths.
>> + *
>> + * Absolute paths are derived from the root device and can follow child<>  or
>> + * link<>  properties.  Since they can follow link<>  properties, they can be
>> + * arbitrarily long.  Absolute paths look like absolute filenames and are prefix
>
> s/prefix/prefixed/
>
>> + * with a leading slash.
>> + *
>> + * Partial paths are look like relative filenames.  They do not begin with a
>
> s/are//
>
>> + * prefix.  The matching rules for partial paths are subtle but designed to make
>> + * specifying devices easy.  At each level of the composition tree, the partial
>> + * path is matched as an absolute path.  The first match is not returned.  At
>> + * least two matches are searched for.  A successful result is only returned if
>> + * only one match is founded.  If more than one match is found, a flag is return
>
> s/return/returned/
>
>> + * to indicate that the match was ambiguous.
>> + *
>> + * @path - the path to resolve
>> + *
>> + * @ambiguous - returns true if the path resolution failed because of an
>> + *              ambiguous match
>
> The implementation seems to depend on ambiguous being initialized to
> false by the caller.  That would be worth documenting or changing so it
> does not depend on the initial value.

That's actually a bug.  I'll fix it.  Ack on the other comments too.

Regards,

Anthony Liguori

>
>> + *
>> + * Returns:
>> + *   The matched device.
>
> The matched device or NULL on path lookup failure.
>

[Qemu-devel] [PATCH 06/18] qom: add child properties (composition)

[Anthony Liguori]

Child properties express a relationship of composition.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |   24 ++++++++++++++++++++++++
 hw/qdev.h |   20 ++++++++++++++++++++
 2 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 5bffbb7..b09d22a 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1156,6 +1156,30 @@ DeviceState *qdev_get_root(void)
     return qdev_root;
 }
 
+static void qdev_get_child_property(DeviceState *dev, Visitor *v, void *opaque,
+                                    const char *name, Error **errp)
+{
+    DeviceState *child = opaque;
+    gchar *path;
+
+    path = qdev_get_canonical_path(child);
+    visit_type_str(v, &path, name, errp);
+    g_free(path);
+}
+
+void qdev_property_add_child(DeviceState *dev, const char *name,
+                             DeviceState *child, Error **errp)
+{
+    gchar *type;
+
+    type = g_strdup_printf("child<%s>", child->info->name);
+
+    qdev_property_add(dev, name, type, qdev_get_child_property,
+                      NULL, NULL, child, errp);
+
+    g_free(type);
+}
+
 static gchar *qdev_get_path_in(DeviceState *parent, DeviceState *dev)
 {
     GSList *i;
diff --git a/hw/qdev.h b/hw/qdev.h
index b8b62f5..905a02c 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -499,4 +499,24 @@ gchar *qdev_get_canonical_path(DeviceState *dev);
  */
 DeviceState *qdev_resolve_path(const char *path, bool *ambiguous);
 
+/**
+ * @qdev_property_add_child - Add a child property to a device
+ *
+ * Child properties form the composition tree.  All devices need to be a child
+ * of another device.  Devices can only be a child of one device.
+ *
+ * There is no way for a child to determine what it's parent is.  It is not
+ * a bidirectional relationship.  This is by design.
+ *
+ * @dev - the device to add a property to
+ *
+ * @name - the name of the property
+ *
+ * @child - the child device
+ *
+ * @errp - if an error occurs, a pointer to an area to store the area
+ */
+void qdev_property_add_child(DeviceState *dev, const char *name,
+                             DeviceState *child, Error **errp);
+
 #endif
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 06/18] qom: add child properties (composition)

[Kevin Wolf]

Am 30.11.2011 22:03, schrieb Anthony Liguori:
> Child properties express a relationship of composition.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Do we have a flag or something that makes sure that a child is never
removed without its parent? The code assumes that child stays valid as
long as the parent lives.

> @@ -499,4 +499,24 @@ gchar *qdev_get_canonical_path(DeviceState *dev);
>   */
>  DeviceState *qdev_resolve_path(const char *path, bool *ambiguous);
>  
> +/**
> + * @qdev_property_add_child - Add a child property to a device
> + *
> + * Child properties form the composition tree.  All devices need to be a child
> + * of another device.  Devices can only be a child of one device.
> + *
> + * There is no way for a child to determine what it's parent is.  It is not

s/it's/its/

Kevin

Re: [Qemu-devel] [PATCH 06/18] qom: add child properties (composition)

[Anthony Liguori]

On 12/02/2011 05:54 AM, Kevin Wolf wrote:
> Am 30.11.2011 22:03, schrieb Anthony Liguori:
>> Child properties express a relationship of composition.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>
> Do we have a flag or something that makes sure that a child is never
> removed without its parent? The code assumes that child stays valid as
> long as the parent lives.

Good question.  This is an interesting issue with qdev.

The child property is not writable so you cannot detach a child.  That's because 
the child's life cycle is supposed to be tied to the parents.

qdev doesn't have the right bits to handle this right now.  I think that's 
fixable by just adding a new flag to DeviceState.

>
>> @@ -499,4 +499,24 @@ gchar *qdev_get_canonical_path(DeviceState *dev);
>>    */
>>   DeviceState *qdev_resolve_path(const char *path, bool *ambiguous);
>>
>> +/**
>> + * @qdev_property_add_child - Add a child property to a device
>> + *
>> + * Child properties form the composition tree.  All devices need to be a child
>> + * of another device.  Devices can only be a child of one device.
>> + *
>> + * There is no way for a child to determine what it's parent is.  It is not
>
> s/it's/its/

Ack.

Regards,

Anthony Liguori

> Kevin
>

[Qemu-devel] [PATCH 07/18] qom: add link properties

[Anthony Liguori]

Links represent an ephemeral relationship between devices.  They are meant to
replace the qdev concept of busses by allowing more informal relationships
between devices.

Links are fairly limited in their usefulness without implementing QOM-style
subclassing and interfaces.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h |   23 ++++++++++++++++++++
 2 files changed, 92 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index b09d22a..658ed2c 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1180,6 +1180,75 @@ void qdev_property_add_child(DeviceState *dev, const char *name,
     g_free(type);
 }
 
+static void qdev_get_link_property(DeviceState *dev, Visitor *v, void *opaque,
+                                   const char *name, Error **errp)
+{
+    DeviceState **child = opaque;
+    gchar *path;
+
+    if (*child) {
+        path = qdev_get_canonical_path(*child);
+        visit_type_str(v, &path, name, errp);
+        g_free(path);
+    } else {
+        path = (gchar *)"";
+        visit_type_str(v, &path, name, errp);
+    }
+}
+
+static void qdev_set_link_property(DeviceState *dev, Visitor *v, void *opaque,
+                                   const char *name, Error **errp)
+{
+    DeviceState **child = opaque;
+    bool ambiguous = false;
+    const char *type;
+    char *path;
+
+    type = qdev_property_get_type(dev, name, NULL);
+
+    visit_type_str(v, &path, name, errp);
+
+    if (strcmp(path, "") != 0) {
+        DeviceState *target;
+
+        target = qdev_resolve_path(path, &ambiguous);
+        if (target) {
+            gchar *target_type;
+
+            target_type = g_strdup_printf("link<%s>", target->info->name);
+            if (strcmp(target_type, type) == 0) {
+                *child = target;
+            } else {
+                error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, type);
+            }
+
+            g_free(target_type);
+        } else {
+            error_set(errp, QERR_DEVICE_NOT_FOUND, path);
+        }
+    } else {
+        *child = NULL;
+    }
+
+    g_free(path);
+}
+
+void qdev_property_add_link(DeviceState *dev, const char *name,
+                            const char *type, DeviceState **child,
+                            Error **errp)
+{
+    gchar *full_type;
+
+    full_type = g_strdup_printf("link<%s>", type);
+
+    qdev_property_add(dev, name, full_type,
+                      qdev_get_link_property,
+                      qdev_set_link_property,
+                      NULL, child, errp);
+
+    g_free(full_type);
+}
+
 static gchar *qdev_get_path_in(DeviceState *parent, DeviceState *dev)
 {
     GSList *i;
diff --git a/hw/qdev.h b/hw/qdev.h
index 905a02c..e8c9e76 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -519,4 +519,27 @@ DeviceState *qdev_resolve_path(const char *path, bool *ambiguous);
 void qdev_property_add_child(DeviceState *dev, const char *name,
                              DeviceState *child, Error **errp);
 
+/**
+ * @qdev_property_add_link - Add a link property to a device
+ *
+ * Links establish relationships between devices.  Links are unidirection
+ * although two links can be combined to form a bidirectional relationship
+ * between devices.
+ *
+ * Links form the graph in the device model.
+ *
+ * @dev - the device to add a property to
+ *
+ * @name - the name of the property
+ *
+ * @type - the qdev type of the link
+ *
+ * @child - a pointer to where the link device reference is stored
+ *
+ * @errp - if an error occurs, a pointer to an area to store the area
+ */
+void qdev_property_add_link(DeviceState *dev, const char *name,
+                            const char *type, DeviceState **child,
+                            Error **errp);
+
 #endif
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 07/18] qom: add link properties

[Stefan Hajnoczi]

On Wed, Nov 30, 2011 at 03:03:37PM -0600, Anthony Liguori wrote:
> +/**
> + * @qdev_property_add_link - Add a link property to a device
> + *
> + * Links establish relationships between devices.  Links are unidirection

s/unidirection/unidirectional/

Re: [Qemu-devel] [PATCH 07/18] qom: add link properties

[Anthony Liguori]

On 12/01/2011 04:55 AM, Stefan Hajnoczi wrote:
> On Wed, Nov 30, 2011 at 03:03:37PM -0600, Anthony Liguori wrote:
>> +/**
>> + * @qdev_property_add_link - Add a link property to a device
>> + *
>> + * Links establish relationships between devices.  Links are unidirection
>
> s/unidirection/unidirectional/

Ack.

Regards,

Anthony Liguori

>

Re: [Qemu-devel] [PATCH 07/18] qom: add link properties

[Avi Kivity]

On 11/30/2011 11:03 PM, Anthony Liguori wrote:
> Links represent an ephemeral relationship between devices.  They are meant to
> replace the qdev concept of busses by allowing more informal relationships
> between devices.

So, links are equivalent to pointers?

> Links are fairly limited in their usefulness without implementing QOM-style
> subclassing and interfaces.
>
>  
> +static void qdev_get_link_property(DeviceState *dev, Visitor *v, void *opaque,
> +                                   const char *name, Error **errp)
> +{
> +    DeviceState **child = opaque;
> +    gchar *path;
> +
> +    if (*child) {
> +        path = qdev_get_canonical_path(*child);
> +        visit_type_str(v, &path, name, errp);
> +        g_free(path);
> +    } else {
> +        path = (gchar *)"";

If gchar != char, this is wrong.  Also, you're converting a const
pointer into a non-const pointer, discarding type safety.

> +        visit_type_str(v, &path, name, errp);
> +    }

Shouldn't this be visit_type_link()?


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 07/18] qom: add link properties

[Stefan Hajnoczi]

On Thu, Dec 1, 2011 at 11:21 AM, Avi Kivity <avi@redhat.com> wrote:
> On 11/30/2011 11:03 PM, Anthony Liguori wrote:
>> Links represent an ephemeral relationship between devices.  They are meant to
>> replace the qdev concept of busses by allowing more informal relationships
>> between devices.
>
> So, links are equivalent to pointers?
>
>> Links are fairly limited in their usefulness without implementing QOM-style
>> subclassing and interfaces.
>>
>>
>> +static void qdev_get_link_property(DeviceState *dev, Visitor *v, void *opaque,
>> +                                   const char *name, Error **errp)
>> +{
>> +    DeviceState **child = opaque;
>> +    gchar *path;
>> +
>> +    if (*child) {
>> +        path = qdev_get_canonical_path(*child);
>> +        visit_type_str(v, &path, name, errp);
>> +        g_free(path);
>> +    } else {
>> +        path = (gchar *)"";
>
> If gchar != char, this is wrong.  Also, you're converting a const
> pointer into a non-const pointer, discarding type safety.

This looked weird to me too but the cast has to do with the fact that
the visitor function works both for input and output visitors.  The
output visitor needs to write to gchar** while the input visitor does
not.

When this function is called with the correct visitor type we are
guaranteed that path will not be modified.

Stefan

Re: [Qemu-devel] [PATCH 07/18] qom: add link properties

[Avi Kivity]

On 12/01/2011 01:35 PM, Stefan Hajnoczi wrote:
> >>
> >> +static void qdev_get_link_property(DeviceState *dev, Visitor *v, void *opaque,
> >> +                                   const char *name, Error **errp)
> >> +{
> >> +    DeviceState **child = opaque;
> >> +    gchar *path;
> >> +
> >> +    if (*child) {
> >> +        path = qdev_get_canonical_path(*child);
> >> +        visit_type_str(v, &path, name, errp);
> >> +        g_free(path);
> >> +    } else {
> >> +        path = (gchar *)"";
> >
> > If gchar != char, this is wrong.  Also, you're converting a const
> > pointer into a non-const pointer, discarding type safety.
>
> This looked weird to me too but the cast has to do with the fact that
> the visitor function works both for input and output visitors.  The
> output visitor needs to write to gchar** while the input visitor does
> not.

So you need to pass a non-const pointer to an array of const char, or
const gchar **.  You don't modify the string in place, you allocate a
new string and free the old one.


> When this function is called with the correct visitor type we are
> guaranteed that path will not be modified.

What if it's called with the output visitor? (warning: confusing
convention).

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 07/18] qom: add link properties

[Anthony Liguori]

On 12/01/2011 06:34 AM, Avi Kivity wrote:
> On 12/01/2011 01:35 PM, Stefan Hajnoczi wrote:
>>>>
>>>> +static void qdev_get_link_property(DeviceState *dev, Visitor *v, void *opaque,
>>>> +                                   const char *name, Error **errp)
>>>> +{
>>>> +    DeviceState **child = opaque;
>>>> +    gchar *path;
>>>> +
>>>> +    if (*child) {
>>>> +        path = qdev_get_canonical_path(*child);
>>>> +        visit_type_str(v,&path, name, errp);
>>>> +        g_free(path);
>>>> +    } else {
>>>> +        path = (gchar *)"";
>>>
>>> If gchar != char, this is wrong.  Also, you're converting a const
>>> pointer into a non-const pointer, discarding type safety.
>>
>> This looked weird to me too but the cast has to do with the fact that
>> the visitor function works both for input and output visitors.  The
>> output visitor needs to write to gchar** while the input visitor does
>> not.
>
> So you need to pass a non-const pointer to an array of const char, or
> const gchar **.  You don't modify the string in place, you allocate a
> new string and free the old one.
>
>
>> When this function is called with the correct visitor type we are
>> guaranteed that path will not be modified.
>
> What if it's called with the output visitor? (warning: confusing
> convention).

The reason there's a single Visitor type that works for both input and output 
visitors is to make it so you can write a single visit function that works for 
input and output.  This works very well for most cases (in fact, QAPI makes 
heavy use of it).

That said, I'm starting to feel like there should be a separate input and output 
visitor interface.  It would make a number of things (like visiting lists) 
significantly simpler.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 07/18] qom: add link properties

[Avi Kivity]

On 12/01/2011 03:47 PM, Anthony Liguori wrote:
> On 12/01/2011 06:34 AM, Avi Kivity wrote:
>> On 12/01/2011 01:35 PM, Stefan Hajnoczi wrote:
>>>>>
>>>>> +static void qdev_get_link_property(DeviceState *dev, Visitor *v,
>>>>> void *opaque,
>>>>> +                                   const char *name, Error **errp)
>>>>> +{
>>>>> +    DeviceState **child = opaque;
>>>>> +    gchar *path;
>>>>> +
>>>>> +    if (*child) {
>>>>> +        path = qdev_get_canonical_path(*child);
>>>>> +        visit_type_str(v,&path, name, errp);
>>>>> +        g_free(path);
>>>>> +    } else {
>>>>> +        path = (gchar *)"";
>>>>
>>>> If gchar != char, this is wrong.  Also, you're converting a const
>>>> pointer into a non-const pointer, discarding type safety.
>>>
>>> This looked weird to me too but the cast has to do with the fact that
>>> the visitor function works both for input and output visitors.  The
>>> output visitor needs to write to gchar** while the input visitor does
>>> not.
>>
>> So you need to pass a non-const pointer to an array of const char, or
>> const gchar **.  You don't modify the string in place, you allocate a
>> new string and free the old one.
>>
>>
>>> When this function is called with the correct visitor type we are
>>> guaranteed that path will not be modified.
>>
>> What if it's called with the output visitor? (warning: confusing
>> convention).
>
> The reason there's a single Visitor type that works for both input and
> output visitors is to make it so you can write a single visit function
> that works for input and output.  This works very well for most cases
> (in fact, QAPI makes heavy use of it).
>
> That said, I'm starting to feel like there should be a separate input
> and output visitor interface.  It would make a number of things (like
> visiting lists) significantly simpler.

Perhaps.  But that's independent of the issue here.  No matter how many
visitors you have, you never change a gchar in a string in place, you
always allocate a new string (or you can't change the string length). 
So the type needs to be const gchar **, not gchar **.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 07/18] qom: add link properties

[Anthony Liguori]

On 12/01/2011 07:50 AM, Avi Kivity wrote:
> On 12/01/2011 03:47 PM, Anthony Liguori wrote:
>>> What if it's called with the output visitor? (warning: confusing
>>> convention).
>>
>> The reason there's a single Visitor type that works for both input and
>> output visitors is to make it so you can write a single visit function
>> that works for input and output.  This works very well for most cases
>> (in fact, QAPI makes heavy use of it).
>>
>> That said, I'm starting to feel like there should be a separate input
>> and output visitor interface.  It would make a number of things (like
>> visiting lists) significantly simpler.
>
> Perhaps.  But that's independent of the issue here.  No matter how many
> visitors you have, you never change a gchar in a string in place, you
> always allocate a new string (or you can't change the string length).
> So the type needs to be const gchar **, not gchar **.

While you are correct, I tend to use 'gchar *' and 'const gchar *' to indicate 
ownership.  If you have a function that takes a 'const gchar **', the 
expectation is that you wouldn't have to free the return value since g_free() 
takes a non-const pointer.  You would, in fact, have to cast away the const in 
order to even call free.

IOW, if you call visit_type_str() and it returns a newly allocated string to 
you, it's your responsibility to free it.

Regards,

Anthony Liguori

>

Re: [Qemu-devel] [PATCH 07/18] qom: add link properties

[Anthony Liguori]

On 12/01/2011 05:21 AM, Avi Kivity wrote:
> On 11/30/2011 11:03 PM, Anthony Liguori wrote:
>> Links represent an ephemeral relationship between devices.  They are meant to
>> replace the qdev concept of busses by allowing more informal relationships
>> between devices.
>
> So, links are equivalent to pointers?

Yup.  Once we have qom inheritance (next stage), we can have a link<PCIDevice> 
property and you'll be able to set it to an E1000State with the appropriate 
casting and error checking taking place.

>> Links are fairly limited in their usefulness without implementing QOM-style
>> subclassing and interfaces.
>>
>>
>> +static void qdev_get_link_property(DeviceState *dev, Visitor *v, void *opaque,
>> +                                   const char *name, Error **errp)
>> +{
>> +    DeviceState **child = opaque;
>> +    gchar *path;
>> +
>> +    if (*child) {
>> +        path = qdev_get_canonical_path(*child);
>> +        visit_type_str(v,&path, name, errp);
>> +        g_free(path);
>> +    } else {
>> +        path = (gchar *)"";
>
> If gchar != char, this is wrong.  Also, you're converting a const
> pointer into a non-const pointer, discarding type safety.

I'll address this later in the thread.

>> +        visit_type_str(v,&path, name, errp);
>> +    }
>
> Shouldn't this be visit_type_link()?

link isn't a primitive type for visitors.   visit_type_link() would just call 
visit_type_str() so I don't think there's a ton of value in introducing the 
extra layer.

The accessors are the only places that should be marshaling links.

Regards,

Anthony Liguori

>

Re: [Qemu-devel] [PATCH 07/18] qom: add link properties

[Avi Kivity]

On 12/01/2011 03:44 PM, Anthony Liguori wrote:
>> So, links are equivalent to pointers?
>
>
> Yup.  Once we have qom inheritance (next stage), we can have a
> link<PCIDevice> property and you'll be able to set it to an E1000State
> with the appropriate casting and error checking taking place.

I really like this goal but can't help feeling that we're stretching C
beyond its limits here, so that the client code ends up
boilerplate-heavy.  Kind of like the issue with local_err elsewhere in
this thread, where you juggle things instead of a "throw
Exception(...)".  What does the client code looks like for link<PCIDevice>?

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 07/18] qom: add link properties

[Anthony Liguori]

On 12/01/2011 08:03 AM, Avi Kivity wrote:
> On 12/01/2011 03:44 PM, Anthony Liguori wrote:
>>> So, links are equivalent to pointers?
>>
>>
>> Yup.  Once we have qom inheritance (next stage), we can have a
>> link<PCIDevice>  property and you'll be able to set it to an E1000State
>> with the appropriate casting and error checking taking place.
>
> I really like this goal but can't help feeling that we're stretching C
> beyond its limits here, so that the client code ends up
> boilerplate-heavy.  Kind of like the issue with local_err elsewhere in
> this thread, where you juggle things instead of a "throw
> Exception(...)".

I understand and have been down every possible road here.  It's tempting to look 
at C++ or another language and view it as a simplifying assumption that makes 
the whole effort tremendously easier.

But that's not been my experience.  We just have to stretch C++ in different 
ways and you end up with that same icky feeling at the end of the day.

>  What does the client code looks like for link<PCIDevice>?

I'm not sure what you mean by client code, but consider a device called 
UsbController that looks like:

struct UsbController
{
     DeviceState parent;

     UsbDevice *slave; // link property
};

To add this as a link, somewhere in the init function you would do:


static void usb_controller_initfn(UsbController *dev)
{
    ...
    qdev_property_add_link(DEVICE(dev), "slave", "UsbDevice",
                           (DeviceState **)&dev->slave, NULL);
}

If you want to set the property explicitly, you would just do:

   dev->slave = some_other_device

You don't need to use any special function to manipulate the link. 
Stylistically, I'd prefer that all devices exposed accessor functions and that 
you did these things through accessors so that we had clear rules about what's 
public and private.

In terms of QMP client code, you just do:

  qom-set /path/to/usb-controller.slave /some/other/device

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 07/18] qom: add link properties

[Avi Kivity]

On 12/01/2011 04:53 PM, Anthony Liguori wrote:
>
>>  What does the client code looks like for link<PCIDevice>?
>
> I'm not sure what you mean by client code,

This:

> but consider a device called UsbController that looks like:
>
> struct UsbController
> {
>     DeviceState parent;
>
>     UsbDevice *slave; // link property
> };
>
> To add this as a link, somewhere in the init function you would do:
>
>
> static void usb_controller_initfn(UsbController *dev)
> {
>    ...
>    qdev_property_add_link(DEVICE(dev), "slave", "UsbDevice",
>                           (DeviceState **)&dev->slave, NULL);
> }

Issues:
- this is an object property, not a class property, so to get a list of
properties we need to instantiate an object.
- "UsbDevice" as the type is not type safe at compile time
- ditto for the cast

>
> If you want to set the property explicitly, you would just do:
>
>   dev->slave = some_other_device
>
> You don't need to use any special function to manipulate the link.
> Stylistically, I'd prefer that all devices exposed accessor functions
> and that you did these things through accessors so that we had clear
> rules about what's public and private.

Also, so we can have observers that trigger on changes.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 07/18] qom: add link properties

[Anthony Liguori]

On 12/01/2011 09:00 AM, Avi Kivity wrote:
> On 12/01/2011 04:53 PM, Anthony Liguori wrote:
>>
>>>   What does the client code looks like for link<PCIDevice>?
>>
>> I'm not sure what you mean by client code,
>
> This:
>
>> but consider a device called UsbController that looks like:
>>
>> struct UsbController
>> {
>>      DeviceState parent;
>>
>>      UsbDevice *slave; // link property
>> };
>>
>> To add this as a link, somewhere in the init function you would do:
>>
>>
>> static void usb_controller_initfn(UsbController *dev)
>> {
>>     ...
>>     qdev_property_add_link(DEVICE(dev), "slave", "UsbDevice",
>>                            (DeviceState **)&dev->slave, NULL);
>> }
>
> Issues:

I have thought about all of these and have solutions but we have to take it one 
step at a time.

> - this is an object property, not a class property, so to get a list of
> properties we need to instantiate an object.

Yes, but it will be safe to instantiate an object as object instantiation is 
side-effect free.  Recall, QOM does not have constructor properties and 
construction is delayed to realize.

In addition, we'll be moving all structures into header files and I imagine 
we'll have a doc syntax so that we can place documentation about the properties 
in the headers and exact it appropriately.

> - "UsbDevice" as the type is not type safe at compile time

It will become TYPE_USB_DEVICE once we get the next stage in.  See some of my 
earlier QOM posts for examples.

> - ditto for the cast

The cast is unfortunate.  I can't really figure a good solution to this.  I 
could implement a macro to eliminate the cast but since this may be a NULL 
pointer when this function is called, I can't find a way to make this safer.

>
>>
>> If you want to set the property explicitly, you would just do:
>>
>>    dev->slave = some_other_device
>>
>> You don't need to use any special function to manipulate the link.
>> Stylistically, I'd prefer that all devices exposed accessor functions
>> and that you did these things through accessors so that we had clear
>> rules about what's public and private.
>
> Also, so we can have observers that trigger on changes.

Exactly.  I don't think it's a hard requirement right now for any type of 
property.  I think avoiding boiler plate is more important at the moment.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 07/18] qom: add link properties

[Gerd Hoffmann]

  Hi,

> In terms of QMP client code, you just do:
> 
>  qom-set /path/to/usb-controller.slave /some/other/device

Lacks notification.  usb-controller doesn't notice that you have plugged
in some usb device and thus can't raise an IRQ to notify the guest ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 07/18] qom: add link properties

[Avi Kivity]

On 12/01/2011 05:03 PM, Gerd Hoffmann wrote:
>   Hi,
>
> > In terms of QMP client code, you just do:
> > 
> >  qom-set /path/to/usb-controller.slave /some/other/device
>
> Lacks notification.  usb-controller doesn't notice that you have plugged
> in some usb device and thus can't raise an IRQ to notify the guest ...
>

I was looking for an example of why we need an observer, thanks.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 07/18] qom: add link properties

[Anthony Liguori]

On 12/01/2011 09:03 AM, Gerd Hoffmann wrote:
>    Hi,
>
>> In terms of QMP client code, you just do:
>>
>>   qom-set /path/to/usb-controller.slave /some/other/device
>
> Lacks notification.  usb-controller doesn't notice that you have plugged
> in some usb device and thus can't raise an IRQ to notify the guest ...

Properties will have flags.  One of those flags makes the property mutable only 
while the device isn't realized.

This is a case where the property is mutable during realize.  You wouldn't use a 
normal link here.  You would use a "hotpluggable link" which would have a notify 
hook.

This works because all properties are get/set through an accessor interface and 
there's a rich error interface.

The original QOM series I posted had this logic in it.

Regards,

Anthony Liguori

>
> cheers,
>    Gerd
>
>

Re: [Qemu-devel] [PATCH 07/18] qom: add link properties

[Kevin Wolf]

Am 30.11.2011 22:03, schrieb Anthony Liguori:
> Links represent an ephemeral relationship between devices.  They are meant to
> replace the qdev concept of busses by allowing more informal relationships
> between devices.
> 
> Links are fairly limited in their usefulness without implementing QOM-style
> subclassing and interfaces.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Same thing as in the previous patch: The code doesn't seem to be
prepared for the case that the "child" (this is not a tree, so there are
no children...) is removed. While you could say that devices used for
composition just shouldn't be unpluggable, I don't think you can require
this for links.

Kevin

Re: [Qemu-devel] [PATCH 07/18] qom: add link properties

[Anthony Liguori]

On 12/02/2011 06:15 AM, Kevin Wolf wrote:
> Am 30.11.2011 22:03, schrieb Anthony Liguori:
>> Links represent an ephemeral relationship between devices.  They are meant to
>> replace the qdev concept of busses by allowing more informal relationships
>> between devices.
>>
>> Links are fairly limited in their usefulness without implementing QOM-style
>> subclassing and interfaces.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>
> Same thing as in the previous patch: The code doesn't seem to be
> prepared for the case that the "child" (this is not a tree, so there are
> no children...) is removed. While you could say that devices used for
> composition just shouldn't be unpluggable, I don't think you can require
> this for links.

I think we need reference counts to handle this appropriately.

Regards,

Anthony Liguori

>
> Kevin
>

[Qemu-devel] [PATCH 08/18] qapi: allow a 'gen' key to suppress code generation

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 scripts/qapi-commands.py |    1 +
 scripts/qapi-types.py    |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index f7def16..54d1f5d 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -405,6 +405,7 @@ except os.error, e:
 
 exprs = parse_schema(sys.stdin)
 commands = filter(lambda expr: expr.has_key('command'), exprs)
+commands = filter(lambda expr: not expr.has_key('gen'), commands)
 
 if dispatch_type == "sync":
     fdecl = open(h_file, 'w')
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index f64d84c..267cb49 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -238,6 +238,7 @@ fdecl.write(mcgen('''
                   guard=guardname(h_file)))
 
 exprs = parse_schema(sys.stdin)
+exprs = filter(lambda expr: not expr.has_key('gen'), exprs)
 
 for expr in exprs:
     ret = "\n"
-- 
1.7.4.1

[Qemu-devel] [PATCH 09/18] qmp: add qom-list command

[Anthony Liguori]

This can be used to list properties in the device model.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 qapi-schema.json |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  |    6 ++++++
 qmp.c            |   29 +++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+), 0 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index cb1ba77..276b7c3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -887,3 +887,51 @@
 # Notes: Do not use this command.
 ##
 { 'command': 'cpu', 'data': {'index': 'int'} }
+
+##
+# @DevicePropertyInfo:
+#
+# @name: the name of the property
+#
+# @type: the type of the property.  This will typically come in one of four
+#        forms:
+#
+#        1) A primitive type such as 'u8', 'u16', 'bool', 'str', or 'double'.
+#           These types are mapped to the appropriate JSON type.
+#
+#        2) A legacy type in the form 'legacy<subtype>' where subtype is the
+#           legacy qdev typename.  These types are always treated as strings.
+#
+#        3) A child type in the form 'child<subtype>' where subtype is a qdev
+#           device type name.  Child properties create the composition tree.
+#
+#        4) A link type in the form 'link<subtype>' where subtype is a qdev
+#           device type name.  Link properties form the device model graph.
+#
+# Since: 1.1
+#
+# Notes: This type is experimental.  Its syntax may change in future releases.
+##
+{ 'type': 'DevicePropertyInfo',
+  'data': { 'name': 'str', 'type': 'str' } }
+
+##
+# @qom-list:
+#
+# This command will list any properties of a device given a path in the device
+# model.
+#
+# @path: the path within the device model.  See @qom-get for a description of
+#        this parameter.
+#
+# Returns: a list of @DevicePropertyInfo that describe the properties of the
+#          device.
+#
+# Since: 1.1
+#
+# Notes: This command is experimental.  It's syntax may change in future
+#        releases.
+##
+{ 'command': 'qom-list',
+  'data': { 'path': 'str' },
+  'returns': [ 'DevicePropertyInfo' ] }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 97975a5..d6ff466 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2001,3 +2001,9 @@ EQMP
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_input_query_balloon,
     },
+
+    {
+        .name       = "qom-list",
+        .args_type  = "path:s",
+        .mhandler.cmd_new = qmp_marshal_input_qom_list,
+    },
diff --git a/qmp.c b/qmp.c
index 511dd62..b9f0572 100644
--- a/qmp.c
+++ b/qmp.c
@@ -16,6 +16,7 @@
 #include "qmp-commands.h"
 #include "kvm.h"
 #include "arch_init.h"
+#include "hw/qdev.h"
 
 NameInfo *qmp_query_name(Error **errp)
 {
@@ -117,3 +118,31 @@ SpiceInfo *qmp_query_spice(Error **errp)
     return NULL;
 };
 #endif
+
+DevicePropertyInfoList *qmp_qom_list(const char *path, Error **errp)
+{
+    DeviceState *dev;
+    bool ambiguous = false;
+    DevicePropertyInfoList *props = NULL;
+    GSList *i;
+
+    dev = qdev_resolve_path(path, &ambiguous);
+    if (dev == NULL) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, path);
+        return NULL;
+    }
+
+    for (i = dev->properties; i; i = i->next) {
+        DevicePropertyInfoList *entry = g_malloc0(sizeof(*entry));
+        DeviceProperty *prop = i->data;
+
+        entry->value = g_malloc0(sizeof(DevicePropertyInfo));
+        entry->next = props;
+        props = entry;
+
+        entry->value->name = g_strdup(prop->name);
+        entry->value->type = g_strdup(prop->type);
+    }
+
+    return props;
+}
-- 
1.7.4.1

[Qemu-devel] [PATCH 10/18] qom: qom_{get,set} monitor commands

[Anthony Liguori]

This allows clients to read and write device model properties through QMP.  QAPI
doesn't support Visitor types yet and these commands are special in that they
don't work with fixed types.

I've added a documentation stub to qapi-schema.json so we can keep consistency
there.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 monitor.h        |    4 +++
 qapi-schema.json |   59 +++++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  |   12 ++++++++++
 qmp.c            |   65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 140 insertions(+), 0 deletions(-)

diff --git a/monitor.h b/monitor.h
index e76795f..efc76c7 100644
--- a/monitor.h
+++ b/monitor.h
@@ -64,4 +64,8 @@ typedef void (MonitorCompletion)(void *opaque, QObject *ret_data);
 
 void monitor_set_error(Monitor *mon, QError *qerror);
 
+int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret);
+
+int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret);
+
 #endif /* !MONITOR_H */
diff --git a/qapi-schema.json b/qapi-schema.json
index 276b7c3..df99e94 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -935,3 +935,62 @@
 { 'command': 'qom-list',
   'data': { 'path': 'str' },
   'returns': [ 'DevicePropertyInfo' ] }
+
+##
+# @qom-get:
+#
+# This command will get a property from a device model path and return the
+# value.
+#
+# @path: The path within the device model.  There are two forms of supported
+#        paths--absolute and partial paths.
+#
+#        Absolute paths are derived from the root device and can follow child<>
+#        or link<> properties.  Since they can follow link<> properties, they
+#        can be arbitrarily long.  Absolute paths look like absolute filenames
+#        and are prefixed  with a leading slash.
+#
+#        Partial paths are look like relative filenames.  They do not begin
+#        with a prefix.  The matching rules for partial paths are subtle but
+#        designed to make specifying devices easy.  At each level of the
+#        composition tree, the partial path is matched as an absolute path.
+#        The first match is not returned.  At least two matches are searched
+#        for.  A successful result is only returned if only one match is
+#        founded.  If more than one match is found, a flag is return to
+#        indicate that the match was ambiguous.
+#
+# @property: The property name to read
+#
+# Returns: The property value.  The type depends on the property type.  legacy<>
+#          properties are returned as #str.  child<> and link<> properties are
+#          returns as #str pathnames.  All integer property types (u8, u16, etc)
+#          are returned as #int.
+#
+# Since: 1.1
+#
+# Notes: This command is experimental and may change syntax in future releases.
+##
+{ 'command': 'qom-get',
+  'data': { 'path': 'str', 'property': 'str' },
+  'returns': 'visitor',
+  'gen': 'no' }
+
+##
+# @qom-set:
+#
+# This command will set a property from a device model path.
+#
+# @path: see @qom-get for a description of this parameter
+#
+# @property: the property name to set
+#
+# @value: a value who's type is appropriate for the property type.  See @qom-get
+#         for a description of type mapping.
+#
+# Since: 1.1
+#
+# Notes: This command is experimental and may change syntax in future releases.
+##
+{ 'command': 'qom-set',
+  'data': { 'path': 'str', 'property': 'str', 'value': 'visitor' },
+  'gen': 'no' }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d6ff466..027189a 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2007,3 +2007,15 @@ EQMP
         .args_type  = "path:s",
         .mhandler.cmd_new = qmp_marshal_input_qom_list,
     },
+
+    {
+        .name       = "qom-set",
+	.args_type  = "path:s,property:s,opts:O",
+	.mhandler.cmd_new = qmp_qom_set,
+    },
+
+    {
+        .name       = "qom-get",
+	.args_type  = "path:s,property:s",
+	.mhandler.cmd_new = qmp_qom_get,
+    },
diff --git a/qmp.c b/qmp.c
index b9f0572..e917db4 100644
--- a/qmp.c
+++ b/qmp.c
@@ -17,6 +17,8 @@
 #include "kvm.h"
 #include "arch_init.h"
 #include "hw/qdev.h"
+#include "qapi/qmp-input-visitor.h"
+#include "qapi/qmp-output-visitor.h"
 
 NameInfo *qmp_query_name(Error **errp)
 {
@@ -146,3 +148,66 @@ DevicePropertyInfoList *qmp_qom_list(const char *path, Error **errp)
 
     return props;
 }
+
+/* FIXME: teach qapi about how to pass through Visitors */
+int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret)
+{
+    const char *path = qdict_get_str(qdict, "path");
+    const char *property = qdict_get_str(qdict, "property");
+    QObject *value = qdict_get(qdict, "value");
+    Error *local_err = NULL;
+    QmpInputVisitor *mi;
+    DeviceState *dev;
+
+    dev = qdev_resolve_path(path, NULL);
+    if (!dev) {
+        error_set(&local_err, QERR_DEVICE_NOT_FOUND, path);
+        goto out;
+    }
+
+    mi = qmp_input_visitor_new(value);
+    qdev_property_set(dev, qmp_input_get_visitor(mi), property, &local_err);
+
+    qmp_input_visitor_cleanup(mi);
+
+out:
+    if (local_err) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        return -1;
+    }
+
+    return 0;
+}
+
+int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret)
+{
+    const char *path = qdict_get_str(qdict, "path");
+    const char *property = qdict_get_str(qdict, "property");
+    Error *local_err = NULL;
+    QmpOutputVisitor *mo;
+    DeviceState *dev;
+
+    dev = qdev_resolve_path(path, NULL);
+    if (!dev) {
+        error_set(&local_err, QERR_DEVICE_NOT_FOUND, path);
+        goto out;
+    }
+
+    mo = qmp_output_visitor_new();
+    qdev_property_get(dev, qmp_output_get_visitor(mo), property, &local_err);
+    if (!local_err) {
+        *ret = qmp_output_get_qobject(mo);
+    }
+
+    qmp_output_visitor_cleanup(mo);
+
+out:
+    if (local_err) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        return -1;
+    }
+
+    return 0;
+}
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 10/18] qom: qom_{get,set} monitor commands

[Stefan Hajnoczi]

On Wed, Nov 30, 2011 at 03:03:40PM -0600, Anthony Liguori wrote:
> +##
> +# @qom-get:
> +#
> +# This command will get a property from a device model path and return the
> +# value.
> +#
> +# @path: The path within the device model.  There are two forms of supported
> +#        paths--absolute and partial paths.
> +#
> +#        Absolute paths are derived from the root device and can follow child<>
> +#        or link<> properties.  Since they can follow link<> properties, they
> +#        can be arbitrarily long.  Absolute paths look like absolute filenames
> +#        and are prefixed  with a leading slash.
> +#
> +#        Partial paths are look like relative filenames.  They do not begin

s/are//

> +#        with a prefix.  The matching rules for partial paths are subtle but
> +#        designed to make specifying devices easy.  At each level of the
> +#        composition tree, the partial path is matched as an absolute path.
> +#        The first match is not returned.  At least two matches are searched
> +#        for.  A successful result is only returned if only one match is
> +#        founded.  If more than one match is found, a flag is return to

s/founded/found/

Re: [Qemu-devel] [PATCH 10/18] qom: qom_{get,set} monitor commands

[Anthony Liguori]

On 12/01/2011 05:04 AM, Stefan Hajnoczi wrote:
> On Wed, Nov 30, 2011 at 03:03:40PM -0600, Anthony Liguori wrote:
>> +##
>> +# @qom-get:
>> +#
>> +# This command will get a property from a device model path and return the
>> +# value.
>> +#
>> +# @path: The path within the device model.  There are two forms of supported
>> +#        paths--absolute and partial paths.
>> +#
>> +#        Absolute paths are derived from the root device and can follow child<>
>> +#        or link<>  properties.  Since they can follow link<>  properties, they
>> +#        can be arbitrarily long.  Absolute paths look like absolute filenames
>> +#        and are prefixed  with a leading slash.
>> +#
>> +#        Partial paths are look like relative filenames.  They do not begin
>
> s/are//
>
>> +#        with a prefix.  The matching rules for partial paths are subtle but
>> +#        designed to make specifying devices easy.  At each level of the
>> +#        composition tree, the partial path is matched as an absolute path.
>> +#        The first match is not returned.  At least two matches are searched
>> +#        for.  A successful result is only returned if only one match is
>> +#        founded.  If more than one match is found, a flag is return to
>
> s/founded/found/

Ack.

Regards,

Anthony Liguori

>

[Qemu-devel] [PATCH 11/18] qdev: add explicitly named devices to the root complex

[Anthony Liguori]

We first add a 'peripheral' container to the root device that we add user
created devices to.  This provides all user created devices with a unique and
isolated namespace.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 658ed2c..8b8d53e 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -222,6 +222,19 @@ int qdev_device_help(QemuOpts *opts)
     return 1;
 }
 
+static DeviceState *qdev_get_peripheral(void)
+{
+    static DeviceState *dev;
+
+    if (dev == NULL) {
+        dev = qdev_create(NULL, "container");
+        qdev_property_add_child(qdev_get_root(), "peripheral", dev, NULL);
+        qdev_init_nofail(dev);
+    }
+
+    return dev;
+}
+
 DeviceState *qdev_device_add(QemuOpts *opts)
 {
     const char *driver, *path, *id;
@@ -273,6 +286,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     id = qemu_opts_id(opts);
     if (id) {
         qdev->id = id;
+        qdev_property_add_child(qdev_get_peripheral(), qdev->id, qdev, NULL);
     }
     if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
         qdev_free(qdev);
-- 
1.7.4.1

[Qemu-devel] [PATCH 12/18] dev: add an anonymous peripheral container

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 8b8d53e..b944108 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -235,6 +235,19 @@ static DeviceState *qdev_get_peripheral(void)
     return dev;
 }
 
+static DeviceState *qdev_get_peripheral_anon(void)
+{
+    static DeviceState *dev;
+
+    if (dev == NULL) {
+        dev = qdev_create(NULL, "container");
+        qdev_property_add_child(qdev_get_root(), "peripheral-anon", dev, NULL);
+        qdev_init_nofail(dev);
+    }
+
+    return dev;
+}
+
 DeviceState *qdev_device_add(QemuOpts *opts)
 {
     const char *driver, *path, *id;
@@ -287,7 +300,13 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     if (id) {
         qdev->id = id;
         qdev_property_add_child(qdev_get_peripheral(), qdev->id, qdev, NULL);
-    }
+    } else {
+        static int anon_count;
+        gchar *name = g_strdup_printf("device[%d]", anon_count++);
+        qdev_property_add_child(qdev_get_peripheral_anon(), name,
+                                qdev, NULL);
+        g_free(name);
+    }        
     if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
         qdev_free(qdev);
         return NULL;
-- 
1.7.4.1

[Qemu-devel] [PATCH 13/18] rtc: make piix3 set the rtc as a child

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/pc_piix.c  |   11 +++++++++++
 hw/piix_pci.c |    3 +++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 970f43c..81d65a9 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -205,6 +205,17 @@ static void pc_init1(MemoryRegion *system_memory,
         }
     }
 
+    /* FIXME there's some major spaghetti here.  Somehow we create the devices
+     * on the PIIX before we actually create it.  We create the PIIX3 deep in
+     * the recess of the i440fx creation too and then loose the DeviceState.
+     *
+     * For now, let's "fix" this by making judicious use of paths.  This is not
+     * generally the right way to do this.
+     */
+
+    qdev_property_add_child(qdev_resolve_path("/i440fx/piix3", NULL),
+                            "rtc", (DeviceState *)rtc_state, NULL);
+
     audio_init(gsi, pci_enabled ? pci_bus : NULL);
 
     pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index d183443..d785d4b 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -288,6 +288,7 @@ static PCIBus *i440fx_common_init(const char *device_name,
                     address_space_io, 0);
     s->bus = b;
     qdev_init_nofail(dev);
+    qdev_property_add_child(qdev_get_root(), "i440fx", dev, NULL);
 
     d = pci_create_simple(b, 0, device_name);
     *pi440fx_state = DO_UPCAST(PCII440FXState, dev, d);
@@ -323,6 +324,8 @@ static PCIBus *i440fx_common_init(const char *device_name,
                 pci_create_simple_multifunction(b, -1, true, "PIIX3"));
         pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3,
                 PIIX_NUM_PIRQS);
+
+        qdev_property_add_child(dev, "piix3", &piix3->dev.qdev, NULL);
     }
     piix3->pic = pic;
 
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 13/18] rtc: make piix3 set the rtc as a child

[Stefan Hajnoczi]

On Wed, Nov 30, 2011 at 03:03:43PM -0600, Anthony Liguori wrote:
> +    /* FIXME there's some major spaghetti here.  Somehow we create the devices
> +     * on the PIIX before we actually create it.  We create the PIIX3 deep in
> +     * the recess of the i440fx creation too and then loose the DeviceState.

s/loose/lose/

Re: [Qemu-devel] [PATCH 13/18] rtc: make piix3 set the rtc as a child

[Anthony Liguori]

On 12/01/2011 05:07 AM, Stefan Hajnoczi wrote:
> On Wed, Nov 30, 2011 at 03:03:43PM -0600, Anthony Liguori wrote:
>> +    /* FIXME there's some major spaghetti here.  Somehow we create the devices
>> +     * on the PIIX before we actually create it.  We create the PIIX3 deep in
>> +     * the recess of the i440fx creation too and then loose the DeviceState.
>
> s/loose/lose/

Ack.

Regards,

Anthony Liguori

[Qemu-devel] [PATCH 14/18] rtc: add a dynamic property for retrieving the date

[Anthony Liguori]

This really shows the power of dynamic object properties compared to qdev
static properties.

This property represents a complex structure who's format is preserved over the
wire.  This is enabled by visitors.

It also shows an entirely synthetic property that is not tied to device state.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/mc146818rtc.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 2aaca2f..0c23cb0 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -614,6 +614,29 @@ static const MemoryRegionOps cmos_ops = {
     .old_portio = cmos_portio
 };
 
+// FIXME add int32 visitor
+static void visit_type_int32(Visitor *v, int *value, const char *name, Error **errp)
+{
+    int64_t val = *value;
+    visit_type_int(v, &val, name, errp);
+}
+
+static void rtc_get_date(DeviceState *dev, Visitor *v, void *opaque,
+                         const char *name, Error **errp)
+{
+    ISADevice *isa = DO_UPCAST(ISADevice, qdev, dev);
+    RTCState *s = DO_UPCAST(RTCState, dev, isa);
+
+    visit_start_struct(v, NULL, "struct tm", name, 0, errp);
+    visit_type_int32(v, &s->current_tm.tm_year, "tm_year", errp);
+    visit_type_int32(v, &s->current_tm.tm_mon, "tm_mon", errp);
+    visit_type_int32(v, &s->current_tm.tm_mday, "tm_mday", errp);
+    visit_type_int32(v, &s->current_tm.tm_hour, "tm_hour", errp);
+    visit_type_int32(v, &s->current_tm.tm_min, "tm_min", errp);
+    visit_type_int32(v, &s->current_tm.tm_sec, "tm_sec", errp);
+    visit_end_struct(v, errp);
+}
+
 static int rtc_initfn(ISADevice *dev)
 {
     RTCState *s = DO_UPCAST(RTCState, dev, dev);
@@ -647,6 +670,10 @@ static int rtc_initfn(ISADevice *dev)
 
     qdev_set_legacy_instance_id(&dev->qdev, base, 2);
     qemu_register_reset(rtc_reset, s);
+
+    qdev_property_add(&s->dev.qdev, "date", "struct tm",
+                      rtc_get_date, NULL, NULL, s, NULL);
+
     return 0;
 }
 
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 14/18] rtc: add a dynamic property for retrieving the date

[Gerd Hoffmann]

  Hi,

> +static void rtc_get_date(DeviceState *dev, Visitor *v, void *opaque,
> +                         const char *name, Error **errp)
> +{
> +    ISADevice *isa = DO_UPCAST(ISADevice, qdev, dev);
> +    RTCState *s = DO_UPCAST(RTCState, dev, isa);
> +
> +    visit_start_struct(v, NULL, "struct tm", name, 0, errp);
> +    visit_type_int32(v, &s->current_tm.tm_year, "tm_year", errp);
> +    visit_type_int32(v, &s->current_tm.tm_mon, "tm_mon", errp);
> +    visit_type_int32(v, &s->current_tm.tm_mday, "tm_mday", errp);
> +    visit_type_int32(v, &s->current_tm.tm_hour, "tm_hour", errp);
> +    visit_type_int32(v, &s->current_tm.tm_min, "tm_min", errp);
> +    visit_type_int32(v, &s->current_tm.tm_sec, "tm_sec", errp);
> +    visit_end_struct(v, errp);
> +}

Ok, what is the long term plan here?  I don't think we want open-code
everything here, do we?  Especially once visitors become more widespread
used.  And I can see that they are useful for a bunch of stuff beside
device properties and relationships.  vmstate for example.  Or to list
device state (say a register dump) for debugging purposes.

Today we have code to generate structs and visitors from scratch.
I think it would be useful to also generate visitors for existing
structs, with some kind of annotation, like this ...

struct SomeDev {
      DeviceState dev;
      Chardev     *chr;
      uint32_t    reg1   __v(vmstate);
      uint32_t    reg2   __v(vmstate);
      [ ... ]
);

... instead of the vmstate structs we create manually today.  Likewise
for properties.  And probably we can even generate different visitors
for different "views" at the same struct.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 14/18] rtc: add a dynamic property for retrieving the date

[Anthony Liguori]

On 12/01/2011 09:46 AM, Gerd Hoffmann wrote:
>    Hi,
>
>> +static void rtc_get_date(DeviceState *dev, Visitor *v, void *opaque,
>> +                         const char *name, Error **errp)
>> +{
>> +    ISADevice *isa = DO_UPCAST(ISADevice, qdev, dev);
>> +    RTCState *s = DO_UPCAST(RTCState, dev, isa);
>> +
>> +    visit_start_struct(v, NULL, "struct tm", name, 0, errp);
>> +    visit_type_int32(v,&s->current_tm.tm_year, "tm_year", errp);
>> +    visit_type_int32(v,&s->current_tm.tm_mon, "tm_mon", errp);
>> +    visit_type_int32(v,&s->current_tm.tm_mday, "tm_mday", errp);
>> +    visit_type_int32(v,&s->current_tm.tm_hour, "tm_hour", errp);
>> +    visit_type_int32(v,&s->current_tm.tm_min, "tm_min", errp);
>> +    visit_type_int32(v,&s->current_tm.tm_sec, "tm_sec", errp);
>> +    visit_end_struct(v, errp);
>> +}
>
> Ok, what is the long term plan here?  I don't think we want open-code
> everything here, do we?  Especially once visitors become more widespread
> used.  And I can see that they are useful for a bunch of stuff beside
> device properties and relationships.  vmstate for example.  Or to list
> device state (say a register dump) for debugging purposes.
>
> Today we have code to generate structs and visitors from scratch.
> I think it would be useful to also generate visitors for existing
> structs, with some kind of annotation, like this ...
>
> struct SomeDev {
>        DeviceState dev;
>        Chardev     *chr;
>        uint32_t    reg1   __v(vmstate);
>        uint32_t    reg2   __v(vmstate);
>        [ ... ]
> );

https://github.com/aliguori/qidl/

In your example:

struct SomeDev {
        DeviceState _immutable dev;
        Chardev _immutable     *chr;
        uint32_t               reg1;
        uint32_t               reg2;
        [ ... ]
};

>
> ... instead of the vmstate structs we create manually today.  Likewise
> for properties.  And probably we can even generate different visitors
> for different "views" at the same struct.

qc generates a json description of the struct.  You can then use a generate to 
take that json and generate code for VMState, QAPI, etc.

The readme has quite a lot of detail about the syntax.  The parser is pretty 
complete already.

https://github.com/aliguori/qidl/blob/master/qc.md

But I want to get us moving on QOM first before I go any further with this.  We 
can always go back and remove the manually written visit functions.

Regards,

Anthony Liguori

>
> cheers,
>    Gerd
>

Re: [Qemu-devel] [PATCH 14/18] rtc: add a dynamic property for retrieving the date

[Gerd Hoffmann]

  Hi,

> https://github.com/aliguori/qidl/
> 
> In your example:
> 
> struct SomeDev {
>        DeviceState _immutable dev;
>        Chardev _immutable     *chr;
>        uint32_t               reg1;
>        uint32_t               reg2;
>        [ ... ]
> };
> 
>>
>> ... instead of the vmstate structs we create manually today.  Likewise
>> for properties.  And probably we can even generate different visitors
>> for different "views" at the same struct.
> 
> qc generates a json description of the struct.  You can then use a
> generate to take that json and generate code for VMState, QAPI, etc.
> 
> The readme has quite a lot of detail about the syntax.  The parser is
> pretty complete already.
> 
> https://github.com/aliguori/qidl/blob/master/qc.md

Ah, nice.  Any plans to support lists there, so it is possible to save
the state of (multiple) in-flight transactions?

> But I want to get us moving on QOM first before I go any further with
> this.  We can always go back and remove the manually written visit
> functions.

Sure, one step at a time.  It helps when reviewing to have a rough idea
of the big pixture though ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 14/18] rtc: add a dynamic property for retrieving the date

[Anthony Liguori]

On 12/02/2011 06:35 AM, Gerd Hoffmann wrote:
>> The readme has quite a lot of detail about the syntax.  The parser is
>> pretty complete already.
>>
>> https://github.com/aliguori/qidl/blob/master/qc.md
>
> Ah, nice.  Any plans to support lists there, so it is possible to save
> the state of (multiple) in-flight transactions?

I had support for lists locally and just pushed it.  It is based on GSList and 
uses a marker to indicate the type.  So:

struct MyDevice {
       int32_t reg1;
       int32_t reg2;

       GSList _type_is(MyRequest) *pending_requests;
};

This is nice and works and extends to any list type but the syntax is awkward. 
So I also swizzled things so that we run through the preprocessor first such 
that we can do:

#define QSList(a) GSList _type_is(MyRequest)

struct MyDevice {
       int32_t reg1;
       int32_t reg2;

       QSList(MyRequest) *pending_requests;
};

Regards,

Anthony Liguori

>> But I want to get us moving on QOM first before I go any further with
>> this.  We can always go back and remove the manually written visit
>> functions.
>
> Sure, one step at a time.  It helps when reviewing to have a rough idea
> of the big pixture though ...
>
> cheers,
>    Gerd
>
>

Re: [Qemu-devel] [PATCH 14/18] rtc: add a dynamic property for retrieving the date

[Gerd Hoffmann]

On 12/02/11 14:20, Anthony Liguori wrote:
> On 12/02/2011 06:35 AM, Gerd Hoffmann wrote:
>>> The readme has quite a lot of detail about the syntax.  The parser is
>>> pretty complete already.
>>>
>>> https://github.com/aliguori/qidl/blob/master/qc.md
>>
>> Ah, nice.  Any plans to support lists there, so it is possible to save
>> the state of (multiple) in-flight transactions?
> 
> I had support for lists locally and just pushed it.  It is based on
> GSList and uses a marker to indicate the type.  So:

We have qemu-queue.h, which unlike GSList is typesafe ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 14/18] rtc: add a dynamic property for retrieving the date

[Anthony Liguori]

On 12/02/2011 07:34 AM, Gerd Hoffmann wrote:
> On 12/02/11 14:20, Anthony Liguori wrote:
>> On 12/02/2011 06:35 AM, Gerd Hoffmann wrote:
>>>> The readme has quite a lot of detail about the syntax.  The parser is
>>>> pretty complete already.
>>>>
>>>> https://github.com/aliguori/qidl/blob/master/qc.md
>>>
>>> Ah, nice.  Any plans to support lists there, so it is possible to save
>>> the state of (multiple) in-flight transactions?
>>
>> I had support for lists locally and just pushed it.  It is based on
>> GSList and uses a marker to indicate the type.  So:
>
> We have qemu-queue.h, which unlike GSList is typesafe ...

qemu-queue would be extremely difficult to use because you need to encode 
information about how to walk the structure in the type.

Otherwise, you can't generate code to walk the structure.

Regards,

Anthony Liguori

>
> cheers,
>    Gerd
>

[Qemu-devel] [PATCH 15/18] qom: optimize qdev_get_canonical_path using a parent link

[Anthony Liguori]

The full tree search was a bit unreasonable.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |   60 +++++++++++++++++++++++++++---------------------------------
 hw/qdev.h |    4 ++++
 2 files changed, 31 insertions(+), 33 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index b944108..7da7196 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1210,6 +1210,9 @@ void qdev_property_add_child(DeviceState *dev, const char *name,
     qdev_property_add(dev, name, type, qdev_get_child_property,
                       NULL, NULL, child, errp);
 
+    g_assert(child->parent == NULL);
+    child->parent = dev;
+
     g_free(type);
 }
 
@@ -1282,48 +1285,39 @@ void qdev_property_add_link(DeviceState *dev, const char *name,
     g_free(full_type);
 }
 
-static gchar *qdev_get_path_in(DeviceState *parent, DeviceState *dev)
+gchar *qdev_get_canonical_path(DeviceState *dev)
 {
-    GSList *i;
+    DeviceState *root = qdev_get_root();
+    char *newpath = NULL, *path = NULL;
 
-    if (parent == dev) {
-        return g_strdup("");
-    }
-
-    for (i = parent->properties; i; i = i->next) {
-        DeviceProperty *prop = i->data;
-        gchar *subpath;
-
-        if (!strstart(prop->type, "child<", NULL)) {
-            continue;
-        }
+    while (dev != root) {
+        GSList *i;
 
-        /* Check to see if the device is one of parent's children */
-        if (prop->opaque == dev) {
-            return g_strdup(prop->name);
-        }
+        g_assert(dev->parent != NULL);
 
-        /* Check to see if the device is a child of our child */
-        subpath = qdev_get_path_in(prop->opaque, dev);
-        if (subpath) {
-            gchar *path;
+        for (i = dev->parent->properties; i; i = i->next) {
+            DeviceProperty *prop = i->data;
 
-            path = g_strdup_printf("%s/%s", prop->name, subpath);
-            g_free(subpath);
+            if (!strstart(prop->type, "child<", NULL)) {
+                continue;
+            }
 
-            return path;
+            if (prop->opaque == dev) {
+                if (path) {
+                    newpath = g_strdup_printf("%s/%s", prop->name, path);
+                    g_free(path);
+                    path = newpath;
+                } else {
+                    path = g_strdup(prop->name);
+                }
+                break;
+            }
         }
-    }
 
-    return NULL;
-}
+        g_assert(i != NULL);
 
-gchar *qdev_get_canonical_path(DeviceState *dev)
-{
-    gchar *path, *newpath;
-
-    path = qdev_get_path_in(qdev_get_root(), dev);
-    g_assert(path != NULL);
+        dev = dev->parent;
+    }
 
     newpath = g_strdup_printf("/%s", path);
     g_free(path);
diff --git a/hw/qdev.h b/hw/qdev.h
index e8c9e76..8eb7ce1 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -83,6 +83,10 @@ struct DeviceState {
     int instance_id_alias;
     int alias_required_for_version;
     GSList *properties;
+
+    /* Do not, under any circumstance, use this parent link below anywhere
+     * outside of qdev.c.  You have been warned. */
+    DeviceState *parent;
 };
 
 typedef void (*bus_dev_printfn)(Monitor *mon, DeviceState *dev, int indent);
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 15/18] qom: optimize qdev_get_canonical_path using a parent link

[Stefan Hajnoczi]

On Wed, Nov 30, 2011 at 03:03:45PM -0600, Anthony Liguori wrote:
> @@ -1210,6 +1210,9 @@ void qdev_property_add_child(DeviceState *dev, const char *name,
>      qdev_property_add(dev, name, type, qdev_get_child_property,
>                        NULL, NULL, child, errp);
>  
> +    g_assert(child->parent == NULL);
> +    child->parent = dev;

The implications are:

1. A DeviceState must be a child or the root.  It is not okay to create
   a DeviceState and inquire its canonical path before making it a child in
   the graph.

2. A DeviceState can only be the child of one parent.  Since
   user-created devices are added to /peripheral or /peripheral-anon this
   means that the /i440fx only has links to them, never a parent-child
   relationship.

Is this right?

> +    /* Do not, under any circumstance, use this parent link below anywhere
> +     * outside of qdev.c.  You have been warned. */
> +    DeviceState *parent;

It would be nice to explain why parent is private to qdev.c.

Re: [Qemu-devel] [PATCH 15/18] qom: optimize qdev_get_canonical_path using a parent link

[Anthony Liguori]

On 12/01/2011 05:21 AM, Stefan Hajnoczi wrote:
> On Wed, Nov 30, 2011 at 03:03:45PM -0600, Anthony Liguori wrote:
>> @@ -1210,6 +1210,9 @@ void qdev_property_add_child(DeviceState *dev, const char *name,
>>       qdev_property_add(dev, name, type, qdev_get_child_property,
>>                         NULL, NULL, child, errp);
>>
>> +    g_assert(child->parent == NULL);
>> +    child->parent = dev;
>
> The implications are:
>
> 1. A DeviceState must be a child or the root.  It is not okay to create
>     a DeviceState and inquire its canonical path before making it a child in
>     the graph.

Correct.

>
> 2. A DeviceState can only be the child of one parent.  Since
>     user-created devices are added to /peripheral or /peripheral-anon this
>     means that the /i440fx only has links to them, never a parent-child
>     relationship.

Correct.

/peripheral[-anon] are where user created devices live.  Machine created devices 
live directly off of /

Each "directory" ends up being a unique namespace.  This ends up being a nice 
way to support compatibly since we have so many namespace today.

I imagine, for instance, that we will end up with a /block directory too and 
that is where blockdev objects will live.

>
> Is this right?
>
>> +    /* Do not, under any circumstance, use this parent link below anywhere
>> +     * outside of qdev.c.  You have been warned. */
>> +    DeviceState *parent;
>
> It would be nice to explain why parent is private to qdev.c.

Composition is a unidirectional relationship.  The parent pointer is there 
simply as an optimization.  I'll make the comment a big scarier :-)

Regards,

Anthony Liguori

[Qemu-devel] [PATCH 16/18] Make qmp.py easier to use

[Anthony Liguori]

---
 QMP/qmp.py |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/QMP/qmp.py b/QMP/qmp.py
index c7dbea0..36ecc1d 100644
--- a/QMP/qmp.py
+++ b/QMP/qmp.py
@@ -128,6 +128,12 @@ class QEMUMonitorProtocol:
             qmp_cmd['id'] = id
         return self.cmd_obj(qmp_cmd)
 
+    def command(self, cmd, **kwds):
+        ret = self.cmd(cmd, kwds)
+        if ret.has_key('error'):
+            raise Exception(ret['error']['desc'])
+        return ret['return']
+
     def get_events(self, wait=False):
         """
         Get a list of available QMP events.
-- 
1.7.4.1

[Qemu-devel] [PATCH 17/18] Add test tools

[Anthony Liguori]

---
 QMP/qom-get  |   26 ++++++++++++++++++++++++++
 QMP/qom-list |   30 ++++++++++++++++++++++++++++++
 QMP/qom-set  |   21 +++++++++++++++++++++
 3 files changed, 77 insertions(+), 0 deletions(-)
 create mode 100755 QMP/qom-get
 create mode 100755 QMP/qom-list
 create mode 100755 QMP/qom-set

diff --git a/QMP/qom-get b/QMP/qom-get
new file mode 100755
index 0000000..b086bc5
--- /dev/null
+++ b/QMP/qom-get
@@ -0,0 +1,26 @@
+#!/usr/bin/python
+##
+# Virtio Support
+#
+# Copyright IBM, Corp. 2011
+#
+# Authors:
+#  Anthony Liguori   <aliguori@us.ibm.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or later.  See
+# the COPYING file in the top-level directory.
+##
+
+import sys
+from qmp import QEMUMonitorProtocol
+
+srv = QEMUMonitorProtocol('/tmp/server.sock')
+srv.connect()
+
+path, prop = sys.argv[1].rsplit('.', 1)
+rsp = srv.command('qom-get', path=path, property=prop)
+if type(rsp) == dict:
+    for i in rsp.keys():
+        print '%s: %s' % (i, rsp[i])
+else:
+    print rsp
diff --git a/QMP/qom-list b/QMP/qom-list
new file mode 100755
index 0000000..5dbb07b
--- /dev/null
+++ b/QMP/qom-list
@@ -0,0 +1,30 @@
+#!/usr/bin/python
+##
+# Virtio Support
+#
+# Copyright IBM, Corp. 2011
+#
+# Authors:
+#  Anthony Liguori   <aliguori@us.ibm.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or later.  See
+# the COPYING file in the top-level directory.
+##
+
+import sys
+from qmp import QEMUMonitorProtocol
+
+srv = QEMUMonitorProtocol('/tmp/server.sock')
+srv.connect()
+
+if len(sys.argv) == 1:
+    print '/'
+    sys.exit(0)
+
+for item in srv.command('qom-list', path=sys.argv[1]):
+    if item['type'].startswith('child<'):
+        print '%s/' % item['name']
+    elif item['type'].startswith('link<'):
+        print '@%s/' % item['name']
+    else:
+        print '%s' % item['name']
diff --git a/QMP/qom-set b/QMP/qom-set
new file mode 100755
index 0000000..83a4394
--- /dev/null
+++ b/QMP/qom-set
@@ -0,0 +1,21 @@
+#!/usr/bin/python
+##
+# Virtio Support
+#
+# Copyright IBM, Corp. 2011
+#
+# Authors:
+#  Anthony Liguori   <aliguori@us.ibm.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or later.  See
+# the COPYING file in the top-level directory.
+##
+
+import sys
+from qmp import QEMUMonitorProtocol
+
+srv = QEMUMonitorProtocol('/tmp/server.sock')
+srv.connect()
+
+path, prop = sys.argv[1].rsplit('.', 1)
+print srv.command('qom-set', path=path, property=prop, value=sys.argv[2])
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 17/18] Add test tools

[Stefan Hajnoczi]

On Wed, Nov 30, 2011 at 03:03:47PM -0600, Anthony Liguori wrote:
> diff --git a/QMP/qom-get b/QMP/qom-get
> new file mode 100755
> index 0000000..b086bc5
> --- /dev/null
> +++ b/QMP/qom-get
> @@ -0,0 +1,26 @@
> +#!/usr/bin/python
> +##
> +# Virtio Support

QEMU Object Model property getter utility

> +#
> +# Copyright IBM, Corp. 2011
> +#
> +# Authors:
> +#  Anthony Liguori   <aliguori@us.ibm.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or later.  See
> +# the COPYING file in the top-level directory.
> +##
> +
> +import sys
> +from qmp import QEMUMonitorProtocol
> +
> +srv = QEMUMonitorProtocol('/tmp/server.sock')

I think it would be worth doing it like QMP/qmp-shell.py and using a
command-line argument.  We might also like to support a
QEMU_MONITOR environment variable.

Hardcoding to /tmp/server.sock makes this script less useful.

> diff --git a/QMP/qom-list b/QMP/qom-list
> new file mode 100755
> index 0000000..5dbb07b
> --- /dev/null
> +++ b/QMP/qom-list
> @@ -0,0 +1,30 @@
> +#!/usr/bin/python
> +##
> +# Virtio Support

QEMU Object Model property listing utility

> diff --git a/QMP/qom-set b/QMP/qom-set
> new file mode 100755
> index 0000000..83a4394
> --- /dev/null
> +++ b/QMP/qom-set
> @@ -0,0 +1,21 @@
> +#!/usr/bin/python
> +##
> +# Virtio Support

QEMU Object Model property setter utility

Re: [Qemu-devel] [PATCH 17/18] Add test tools

[Anthony Liguori]

On 12/01/2011 05:26 AM, Stefan Hajnoczi wrote:
> On Wed, Nov 30, 2011 at 03:03:47PM -0600, Anthony Liguori wrote:
>> diff --git a/QMP/qom-get b/QMP/qom-get
>> new file mode 100755
>> index 0000000..b086bc5
>> --- /dev/null
>> +++ b/QMP/qom-get
>> @@ -0,0 +1,26 @@
>> +#!/usr/bin/python
>> +##
>> +# Virtio Support
>
> QEMU Object Model property getter utility
>
>> +#
>> +# Copyright IBM, Corp. 2011
>> +#
>> +# Authors:
>> +#  Anthony Liguori<aliguori@us.ibm.com>
>> +#
>> +# This work is licensed under the terms of the GNU GPL, version 2 or later.  See
>> +# the COPYING file in the top-level directory.
>> +##
>> +
>> +import sys
>> +from qmp import QEMUMonitorProtocol
>> +
>> +srv = QEMUMonitorProtocol('/tmp/server.sock')
>
> I think it would be worth doing it like QMP/qmp-shell.py and using a
> command-line argument.  We might also like to support a
> QEMU_MONITOR environment variable.
>
> Hardcoding to /tmp/server.sock makes this script less useful.
>
>> diff --git a/QMP/qom-list b/QMP/qom-list
>> new file mode 100755
>> index 0000000..5dbb07b
>> --- /dev/null
>> +++ b/QMP/qom-list
>> @@ -0,0 +1,30 @@
>> +#!/usr/bin/python
>> +##
>> +# Virtio Support
>
> QEMU Object Model property listing utility
>
>> diff --git a/QMP/qom-set b/QMP/qom-set
>> new file mode 100755
>> index 0000000..83a4394
>> --- /dev/null
>> +++ b/QMP/qom-set
>> @@ -0,0 +1,21 @@
>> +#!/usr/bin/python
>> +##
>> +# Virtio Support
>
> QEMU Object Model property setter utility


Ack.  I made these changes actually but something got lost when I sent the 
patches out.  I actually used git-publish for the first time and apparently 
screwed something up when setting up the branch :-/

Regards,

Anthony Liguori
>

Re: [Qemu-devel] [PATCH 17/18] Add test tools

[Stefan Hajnoczi]

On Thu, Dec 1, 2011 at 1:39 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 12/01/2011 05:26 AM, Stefan Hajnoczi wrote:
>>
>> On Wed, Nov 30, 2011 at 03:03:47PM -0600, Anthony Liguori wrote:
> Ack.  I made these changes actually but something got lost when I sent the
> patches out.  I actually used git-publish for the first time and apparently
> screwed something up when setting up the branch :-/

Let me know if git-publish acts in a way you don't expect.  The latest
version is 0.2 at https://github.com/stefanha/git-publish.

Stefan

[Qemu-devel] [PATCH 18/18] qdev: split out QOM functions to separate files

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 Makefile.objs |    2 +-
 hw/qdev-qom.h |  200 +++++++++++++++++++++++++++
 hw/qdev.c     |  401 -------------------------------------------------------
 hw/qdev.h     |  175 +------------------------
 hw/qom.c      |  414 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 616 insertions(+), 576 deletions(-)
 create mode 100644 hw/qdev-qom.h
 create mode 100644 hw/qom.c

diff --git a/Makefile.objs b/Makefile.objs
index 10e794c..6a8b504 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -112,7 +112,7 @@ common-obj-y += bt-hci-csr.o
 common-obj-y += buffered_file.o migration.o migration-tcp.o
 common-obj-y += qemu-char.o savevm.o #aio.o
 common-obj-y += msmouse.o ps2.o
-common-obj-y += qdev.o qdev-properties.o
+common-obj-y += qdev.o qdev-properties.o qom.o
 common-obj-y += block-migration.o iohandler.o
 common-obj-y += pflib.o
 common-obj-y += bitmap.o bitops.o
diff --git a/hw/qdev-qom.h b/hw/qdev-qom.h
new file mode 100644
index 0000000..2036a6f
--- /dev/null
+++ b/hw/qdev-qom.h
@@ -0,0 +1,200 @@
+/*
+ * QEMU Object Model
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef QDEV_QOM_H
+#define QDEV_QOM_H
+
+#include "qdev.h"
+
+/**
+ * Do not include this file directly.  It's meant to be consumed through qdev.h.
+ *
+ * These functions are split out to isolate the legacy qdev code to make it
+ * easier to remove in the future.
+ */
+
+/**
+ * @qdev_property_add - add a new property to a device
+ *
+ * @dev - the device to add a property to
+ *
+ * @name - the name of the property.  This can contain any character except for
+ *         a forward slash.  In general, you should use hyphens '-' instead of
+ *         underscores '_' when naming properties.
+ *
+ * @type - the type name of the property.  This namespace is pretty loosely
+ *         defined.  Sub namespaces are constructed by using a prefix and then
+ *         to angle brackets.  For instance, the type 'virtio-net-pci' in the
+ *         'link' namespace would be 'link<virtio-net-pci>'.
+ *
+ * @get - the getter to be called to read a property.  If this is NULL, then
+ *        the property cannot be read.
+ *
+ * @set - the setter to be called to write a property.  If this is NULL, then
+ *        the property cannot be written.
+ *
+ * @release - called when the property is removed from the device.  This is
+ *            meant to allow a property to free its opaque upon device
+ *            destruction.  This may be NULL.
+ *
+ * @opaque - this is user data passed to @get, @set, and @release
+ *
+ * @errp - returns an error if this function fails
+ */
+void qdev_property_add(DeviceState *dev, const char *name, const char *type,
+                       DevicePropertyEtter *get, DevicePropertyEtter *set,
+                       DevicePropertyRelease *release, void *opaque,
+                       Error **errp);
+
+
+/**
+ * @qdev_property_get - reads a property from a device
+ *
+ * @dev - the device
+ *
+ * @v - the visitor that will receive the property value.  This should be an
+ *      Output visitor and the data will be written with @name as the name.
+ *
+ * @name - the name of the property
+ *
+ * @errp - returns an error if this function fails
+ */
+void qdev_property_get(DeviceState *dev, Visitor *v, const char *name,
+                       Error **errp);
+
+/**
+ * @qdev_property_set - writes a property to a device
+ *
+ * @dev - the device
+ *
+ * @v - the visitor that will used to write the property value.  This should be
+ *      an Input visitor and the data will be first read with @name as the name
+ *      and then written as the property value.
+ *
+ * @name - the name of the property
+ *
+ * @errp - returns an error if this function fails
+ */
+void qdev_property_set(DeviceState *dev, Visitor *v, const char *name,
+                       Error **errp);
+
+/**
+ * @qdev_property_get_type - returns the type of a property
+ *
+ * @dev - the device
+ *
+ * @name - the name of the property
+ *
+ * @errp - returns an error if this function fails
+ *
+ * Returns:
+ *   The type name of the property.
+ */
+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);
+
+/**
+ * @qdev_get_root - returns the root device of the composition tree
+ *
+ * Returns:
+ *   The root of the composition tree.
+ */
+DeviceState *qdev_get_root(void);
+
+/**
+ * @qdev_get_canonical_path - returns the canonical path for a device.  This
+ * is the path within the composition tree starting from the root.
+ *
+ * Returns:
+ *   The canonical path in the composition tree.
+ */
+gchar *qdev_get_canonical_path(DeviceState *dev);
+
+/**
+ * @qdev_resolve_path - resolves a path returning a device
+ *
+ * There are two types of supported paths--absolute paths and partial paths.
+ * 
+ * Absolute paths are derived from the root device and can follow child<> or
+ * link<> properties.  Since they can follow link<> properties, they can be
+ * arbitrarily long.  Absolute paths look like absolute filenames and are prefix
+ * with a leading slash.
+ * 
+ * Partial paths are look like relative filenames.  They do not begin with a
+ * prefix.  The matching rules for partial paths are subtle but designed to make
+ * specifying devices easy.  At each level of the composition tree, the partial
+ * path is matched as an absolute path.  The first match is not returned.  At
+ * least two matches are searched for.  A successful result is only returned if
+ * only one match is founded.  If more than one match is found, a flag is return
+ * to indicate that the match was ambiguous.
+ *
+ * @path - the path to resolve
+ *
+ * @ambiguous - returns true if the path resolution failed because of an
+ *              ambiguous match
+ *
+ * Returns:
+ *   The matched device.
+ */
+DeviceState *qdev_resolve_path(const char *path, bool *ambiguous);
+
+/**
+ * @qdev_property_add_child - Add a child property to a device
+ *
+ * Child properties form the composition tree.  All devices need to be a child
+ * of another device.  Devices can only be a child of one device.
+ *
+ * There is no way for a child to determine what it's parent is.  It is not
+ * a bidirectional relationship.  This is by design.
+ *
+ * @dev - the device to add a property to
+ *
+ * @name - the name of the property
+ *
+ * @child - the child device
+ *
+ * @errp - if an error occurs, a pointer to an area to store the area
+ */
+void qdev_property_add_child(DeviceState *dev, const char *name,
+                             DeviceState *child, Error **errp);
+
+/**
+ * @qdev_property_add_link - Add a link property to a device
+ *
+ * Links establish relationships between devices.  Links are unidirection
+ * although two links can be combined to form a bidirectional relationship
+ * between devices.
+ *
+ * Links form the graph in the device model.
+ *
+ * @dev - the device to add a property to
+ *
+ * @name - the name of the property
+ *
+ * @type - the qdev type of the link
+ *
+ * @child - a pointer to where the link device reference is stored
+ *
+ * @errp - if an error occurs, a pointer to an area to store the area
+ */
+void qdev_property_add_link(DeviceState *dev, const char *name,
+                            const char *type, DeviceState **child,
+                            Error **errp);
+#endif
diff --git a/hw/qdev.c b/hw/qdev.c
index 7da7196..8284840 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1023,404 +1023,3 @@ char* qdev_get_fw_dev_path(DeviceState *dev)
     return strdup(path);
 }
 
-void qdev_property_add(DeviceState *dev, const char *name, const char *type,
-                       DevicePropertyEtter *get, DevicePropertyEtter *set,
-                       DevicePropertyRelease *release, void *opaque,
-                       Error **errp)
-{
-    DeviceProperty *prop = g_malloc0(sizeof(*prop));
-
-    prop->name = g_strdup(name);
-    prop->type = g_strdup(type);
-    prop->get = get;
-    prop->set = set;
-    prop->release = release;
-    prop->opaque = opaque;
-
-    dev->properties = g_slist_append(dev->properties, prop);
-}
-
-static DeviceProperty *qdev_property_find(DeviceState *dev, const char *name)
-{
-    GSList *i;
-
-    for (i = dev->properties; i; i = i->next) {
-        DeviceProperty *prop = i->data;
-
-        if (strcmp(prop->name, name) == 0) {
-            return prop;
-        }
-    }
-
-    return NULL;
-}
-
-void qdev_property_get(DeviceState *dev, Visitor *v, const char *name,
-                       Error **errp)
-{
-    DeviceProperty *prop = qdev_property_find(dev, name);
-
-    if (prop == NULL) {
-        error_set(errp, QERR_PROPERTY_NOT_FOUND, dev->id?:"", name);
-        return;
-    }
-
-    if (!prop->get) {
-        error_set(errp, QERR_PERMISSION_DENIED);
-    } else {
-        prop->get(dev, v, prop->opaque, name, errp);
-    }
-}
-
-void qdev_property_set(DeviceState *dev, Visitor *v, const char *name,
-                       Error **errp)
-{
-    DeviceProperty *prop = qdev_property_find(dev, name);
-
-    if (prop == NULL) {
-        error_set(errp, QERR_PROPERTY_NOT_FOUND, dev->id?:"", name);
-        return;
-    }
-
-    if (!prop->set) {
-        error_set(errp, QERR_PERMISSION_DENIED);
-    } else {
-        prop->set(dev, prop->opaque, v, name, errp);
-    }
-}
-
-const char *qdev_property_get_type(DeviceState *dev, const char *name, Error **errp)
-{
-    DeviceProperty *prop = qdev_property_find(dev, name);
-
-    if (prop == NULL) {
-        error_set(errp, QERR_PROPERTY_NOT_FOUND, dev->id?:"", name);
-        return NULL;
-    }
-
-    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);
-}
-
-DeviceState *qdev_get_root(void)
-{
-    static DeviceState *qdev_root;
-
-    if (!qdev_root) {
-        qdev_root = qdev_create(NULL, "container");
-        qdev_init_nofail(qdev_root);
-    }
-
-    return qdev_root;
-}
-
-static void qdev_get_child_property(DeviceState *dev, Visitor *v, void *opaque,
-                                    const char *name, Error **errp)
-{
-    DeviceState *child = opaque;
-    gchar *path;
-
-    path = qdev_get_canonical_path(child);
-    visit_type_str(v, &path, name, errp);
-    g_free(path);
-}
-
-void qdev_property_add_child(DeviceState *dev, const char *name,
-                             DeviceState *child, Error **errp)
-{
-    gchar *type;
-
-    type = g_strdup_printf("child<%s>", child->info->name);
-
-    qdev_property_add(dev, name, type, qdev_get_child_property,
-                      NULL, NULL, child, errp);
-
-    g_assert(child->parent == NULL);
-    child->parent = dev;
-
-    g_free(type);
-}
-
-static void qdev_get_link_property(DeviceState *dev, Visitor *v, void *opaque,
-                                   const char *name, Error **errp)
-{
-    DeviceState **child = opaque;
-    gchar *path;
-
-    if (*child) {
-        path = qdev_get_canonical_path(*child);
-        visit_type_str(v, &path, name, errp);
-        g_free(path);
-    } else {
-        path = (gchar *)"";
-        visit_type_str(v, &path, name, errp);
-    }
-}
-
-static void qdev_set_link_property(DeviceState *dev, Visitor *v, void *opaque,
-                                   const char *name, Error **errp)
-{
-    DeviceState **child = opaque;
-    bool ambiguous = false;
-    const char *type;
-    char *path;
-
-    type = qdev_property_get_type(dev, name, NULL);
-
-    visit_type_str(v, &path, name, errp);
-
-    if (strcmp(path, "") != 0) {
-        DeviceState *target;
-
-        target = qdev_resolve_path(path, &ambiguous);
-        if (target) {
-            gchar *target_type;
-
-            target_type = g_strdup_printf("link<%s>", target->info->name);
-            if (strcmp(target_type, type) == 0) {
-                *child = target;
-            } else {
-                error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, type);
-            }
-
-            g_free(target_type);
-        } else {
-            error_set(errp, QERR_DEVICE_NOT_FOUND, path);
-        }
-    } else {
-        *child = NULL;
-    }
-
-    g_free(path);
-}
-
-void qdev_property_add_link(DeviceState *dev, const char *name,
-                            const char *type, DeviceState **child,
-                            Error **errp)
-{
-    gchar *full_type;
-
-    full_type = g_strdup_printf("link<%s>", type);
-
-    qdev_property_add(dev, name, full_type,
-                      qdev_get_link_property,
-                      qdev_set_link_property,
-                      NULL, child, errp);
-
-    g_free(full_type);
-}
-
-gchar *qdev_get_canonical_path(DeviceState *dev)
-{
-    DeviceState *root = qdev_get_root();
-    char *newpath = NULL, *path = NULL;
-
-    while (dev != root) {
-        GSList *i;
-
-        g_assert(dev->parent != NULL);
-
-        for (i = dev->parent->properties; i; i = i->next) {
-            DeviceProperty *prop = i->data;
-
-            if (!strstart(prop->type, "child<", NULL)) {
-                continue;
-            }
-
-            if (prop->opaque == dev) {
-                if (path) {
-                    newpath = g_strdup_printf("%s/%s", prop->name, path);
-                    g_free(path);
-                    path = newpath;
-                } else {
-                    path = g_strdup(prop->name);
-                }
-                break;
-            }
-        }
-
-        g_assert(i != NULL);
-
-        dev = dev->parent;
-    }
-
-    newpath = g_strdup_printf("/%s", path);
-    g_free(path);
-
-    return newpath;
-}
-
-static DeviceState *qdev_resolve_abs_path(DeviceState *parent,
-                                          gchar **parts,
-                                          int index)
-{
-    DeviceProperty *prop;
-    DeviceState *child;
-
-    if (parts[index] == NULL) {
-        return parent;
-    }
-
-    if (strcmp(parts[index], "") == 0) {
-        return qdev_resolve_abs_path(parent, parts, index + 1);
-    }
-
-    prop = qdev_property_find(parent, parts[index]);
-    if (prop == NULL) {
-        return NULL;
-    }
-
-    child = NULL;
-    if (strstart(prop->type, "link<", NULL)) {
-        DeviceState **pchild = prop->opaque;
-        if (*pchild) {
-            child = *pchild;
-        }
-    } else if (strstart(prop->type, "child<", NULL)) {
-        child = prop->opaque;
-    }
-
-    if (!child) {
-        return NULL;
-    }
-
-    return qdev_resolve_abs_path(child, parts, index + 1);
-}
-
-static DeviceState *qdev_resolve_partial_path(DeviceState *parent,
-                                              gchar **parts,
-                                              bool *ambiguous)
-{
-    DeviceState *dev;
-    GSList *i;
-
-    dev = qdev_resolve_abs_path(parent, parts, 0);
-
-    for (i = parent->properties; i; i = i->next) {
-        DeviceProperty *prop = i->data;
-        DeviceState *found;
-
-        if (!strstart(prop->type, "child<", NULL)) {
-            continue;
-        }
-
-        found = qdev_resolve_partial_path(prop->opaque, parts, ambiguous);
-        if (found) {
-            if (dev) {
-                if (ambiguous) {
-                    *ambiguous = true;
-                }
-                return NULL;
-            }
-            dev = found;
-        }
-
-        if (ambiguous && *ambiguous) {
-            return NULL;
-        }
-    }
-
-    return dev;
-}
-
-DeviceState *qdev_resolve_path(const char *path, bool *ambiguous)
-{
-    bool partial_path = true;
-    DeviceState *dev;
-    gchar **parts;
-
-    parts = g_strsplit(path, "/", 0);
-    if (parts == NULL || parts[0] == NULL) {
-        return qdev_get_root();
-    }
-
-    if (strcmp(parts[0], "") == 0) {
-        partial_path = false;
-    }
-
-    if (partial_path) {
-        dev = qdev_resolve_partial_path(qdev_get_root(), parts, ambiguous);
-    } else {
-        dev = qdev_resolve_abs_path(qdev_get_root(), parts, 1);
-    }
-
-    g_strfreev(parts);
-
-    return dev;
-}
-
diff --git a/hw/qdev.h b/hw/qdev.h
index 8eb7ce1..dcae2f9 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -371,179 +371,6 @@ char *qdev_get_fw_dev_path(DeviceState *dev);
 /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
 extern struct BusInfo system_bus_info;
 
-/**
- * @qdev_property_add - add a new property to a device
- *
- * @dev - the device to add a property to
- *
- * @name - the name of the property.  This can contain any character except for
- *         a forward slash.  In general, you should use hyphens '-' instead of
- *         underscores '_' when naming properties.
- *
- * @type - the type name of the property.  This namespace is pretty loosely
- *         defined.  Sub namespaces are constructed by using a prefix and then
- *         to angle brackets.  For instance, the type 'virtio-net-pci' in the
- *         'link' namespace would be 'link<virtio-net-pci>'.
- *
- * @get - the getter to be called to read a property.  If this is NULL, then
- *        the property cannot be read.
- *
- * @set - the setter to be called to write a property.  If this is NULL, then
- *        the property cannot be written.
- *
- * @release - called when the property is removed from the device.  This is
- *            meant to allow a property to free its opaque upon device
- *            destruction.  This may be NULL.
- *
- * @opaque - this is user data passed to @get, @set, and @release
- *
- * @errp - returns an error if this function fails
- */
-void qdev_property_add(DeviceState *dev, const char *name, const char *type,
-                       DevicePropertyEtter *get, DevicePropertyEtter *set,
-                       DevicePropertyRelease *release, void *opaque,
-                       Error **errp);
-
-
-/**
- * @qdev_property_get - reads a property from a device
- *
- * @dev - the device
- *
- * @v - the visitor that will receive the property value.  This should be an
- *      Output visitor and the data will be written with @name as the name.
- *
- * @name - the name of the property
- *
- * @errp - returns an error if this function fails
- */
-void qdev_property_get(DeviceState *dev, Visitor *v, const char *name,
-                       Error **errp);
-
-/**
- * @qdev_property_set - writes a property to a device
- *
- * @dev - the device
- *
- * @v - the visitor that will used to write the property value.  This should be
- *      an Input visitor and the data will be first read with @name as the name
- *      and then written as the property value.
- *
- * @name - the name of the property
- *
- * @errp - returns an error if this function fails
- */
-void qdev_property_set(DeviceState *dev, Visitor *v, const char *name,
-                       Error **errp);
-
-/**
- * @qdev_property_get_type - returns the type of a property
- *
- * @dev - the device
- *
- * @name - the name of the property
- *
- * @errp - returns an error if this function fails
- *
- * Returns:
- *   The type name of the property.
- */
-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);
-
-/**
- * @qdev_get_root - returns the root device of the composition tree
- *
- * Returns:
- *   The root of the composition tree.
- */
-DeviceState *qdev_get_root(void);
-
-/**
- * @qdev_get_canonical_path - returns the canonical path for a device.  This
- * is the path within the composition tree starting from the root.
- *
- * Returns:
- *   The canonical path in the composition tree.
- */
-gchar *qdev_get_canonical_path(DeviceState *dev);
-
-/**
- * @qdev_resolve_path - resolves a path returning a device
- *
- * There are two types of supported paths--absolute paths and partial paths.
- * 
- * Absolute paths are derived from the root device and can follow child<> or
- * link<> properties.  Since they can follow link<> properties, they can be
- * arbitrarily long.  Absolute paths look like absolute filenames and are prefix
- * with a leading slash.
- * 
- * Partial paths are look like relative filenames.  They do not begin with a
- * prefix.  The matching rules for partial paths are subtle but designed to make
- * specifying devices easy.  At each level of the composition tree, the partial
- * path is matched as an absolute path.  The first match is not returned.  At
- * least two matches are searched for.  A successful result is only returned if
- * only one match is founded.  If more than one match is found, a flag is return
- * to indicate that the match was ambiguous.
- *
- * @path - the path to resolve
- *
- * @ambiguous - returns true if the path resolution failed because of an
- *              ambiguous match
- *
- * Returns:
- *   The matched device.
- */
-DeviceState *qdev_resolve_path(const char *path, bool *ambiguous);
-
-/**
- * @qdev_property_add_child - Add a child property to a device
- *
- * Child properties form the composition tree.  All devices need to be a child
- * of another device.  Devices can only be a child of one device.
- *
- * There is no way for a child to determine what it's parent is.  It is not
- * a bidirectional relationship.  This is by design.
- *
- * @dev - the device to add a property to
- *
- * @name - the name of the property
- *
- * @child - the child device
- *
- * @errp - if an error occurs, a pointer to an area to store the area
- */
-void qdev_property_add_child(DeviceState *dev, const char *name,
-                             DeviceState *child, Error **errp);
-
-/**
- * @qdev_property_add_link - Add a link property to a device
- *
- * Links establish relationships between devices.  Links are unidirection
- * although two links can be combined to form a bidirectional relationship
- * between devices.
- *
- * Links form the graph in the device model.
- *
- * @dev - the device to add a property to
- *
- * @name - the name of the property
- *
- * @type - the qdev type of the link
- *
- * @child - a pointer to where the link device reference is stored
- *
- * @errp - if an error occurs, a pointer to an area to store the area
- */
-void qdev_property_add_link(DeviceState *dev, const char *name,
-                            const char *type, DeviceState **child,
-                            Error **errp);
+#include "qdev-qom.h"
 
 #endif
diff --git a/hw/qom.c b/hw/qom.c
new file mode 100644
index 0000000..4509d9a
--- /dev/null
+++ b/hw/qom.c
@@ -0,0 +1,414 @@
+/*
+ * QEMU Object Model
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+#include "qdev-qom.h"
+
+void qdev_property_add(DeviceState *dev, const char *name, const char *type,
+                       DevicePropertyEtter *get, DevicePropertyEtter *set,
+                       DevicePropertyRelease *release, void *opaque,
+                       Error **errp)
+{
+    DeviceProperty *prop = g_malloc0(sizeof(*prop));
+
+    prop->name = g_strdup(name);
+    prop->type = g_strdup(type);
+    prop->get = get;
+    prop->set = set;
+    prop->release = release;
+    prop->opaque = opaque;
+
+    dev->properties = g_slist_append(dev->properties, prop);
+}
+
+static DeviceProperty *qdev_property_find(DeviceState *dev, const char *name)
+{
+    GSList *i;
+
+    for (i = dev->properties; i; i = i->next) {
+        DeviceProperty *prop = i->data;
+
+        if (strcmp(prop->name, name) == 0) {
+            return prop;
+        }
+    }
+
+    return NULL;
+}
+
+void qdev_property_get(DeviceState *dev, Visitor *v, const char *name,
+                       Error **errp)
+{
+    DeviceProperty *prop = qdev_property_find(dev, name);
+
+    if (prop == NULL) {
+        error_set(errp, QERR_PROPERTY_NOT_FOUND, dev->id?:"", name);
+        return;
+    }
+
+    if (!prop->get) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+    } else {
+        prop->get(dev, v, prop->opaque, name, errp);
+    }
+}
+
+void qdev_property_set(DeviceState *dev, Visitor *v, const char *name,
+                       Error **errp)
+{
+    DeviceProperty *prop = qdev_property_find(dev, name);
+
+    if (prop == NULL) {
+        error_set(errp, QERR_PROPERTY_NOT_FOUND, dev->id?:"", name);
+        return;
+    }
+
+    if (!prop->set) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+    } else {
+        prop->set(dev, prop->opaque, v, name, errp);
+    }
+}
+
+const char *qdev_property_get_type(DeviceState *dev, const char *name, Error **errp)
+{
+    DeviceProperty *prop = qdev_property_find(dev, name);
+
+    if (prop == NULL) {
+        error_set(errp, QERR_PROPERTY_NOT_FOUND, dev->id?:"", name);
+        return NULL;
+    }
+
+    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);
+}
+
+DeviceState *qdev_get_root(void)
+{
+    static DeviceState *qdev_root;
+
+    if (!qdev_root) {
+        qdev_root = qdev_create(NULL, "container");
+        qdev_init_nofail(qdev_root);
+    }
+
+    return qdev_root;
+}
+
+static void qdev_get_child_property(DeviceState *dev, Visitor *v, void *opaque,
+                                    const char *name, Error **errp)
+{
+    DeviceState *child = opaque;
+    gchar *path;
+
+    path = qdev_get_canonical_path(child);
+    visit_type_str(v, &path, name, errp);
+    g_free(path);
+}
+
+void qdev_property_add_child(DeviceState *dev, const char *name,
+                             DeviceState *child, Error **errp)
+{
+    gchar *type;
+
+    type = g_strdup_printf("child<%s>", child->info->name);
+
+    qdev_property_add(dev, name, type, qdev_get_child_property,
+                      NULL, NULL, child, errp);
+
+    g_assert(child->parent == NULL);
+    child->parent = dev;
+
+    g_free(type);
+}
+
+static void qdev_get_link_property(DeviceState *dev, Visitor *v, void *opaque,
+                                   const char *name, Error **errp)
+{
+    DeviceState **child = opaque;
+    gchar *path;
+
+    if (*child) {
+        path = qdev_get_canonical_path(*child);
+        visit_type_str(v, &path, name, errp);
+        g_free(path);
+    } else {
+        path = (gchar *)"";
+        visit_type_str(v, &path, name, errp);
+    }
+}
+
+static void qdev_set_link_property(DeviceState *dev, Visitor *v, void *opaque,
+                                   const char *name, Error **errp)
+{
+    DeviceState **child = opaque;
+    bool ambiguous = false;
+    const char *type;
+    char *path;
+
+    type = qdev_property_get_type(dev, name, NULL);
+
+    visit_type_str(v, &path, name, errp);
+
+    if (strcmp(path, "") != 0) {
+        DeviceState *target;
+
+        target = qdev_resolve_path(path, &ambiguous);
+        if (target) {
+            gchar *target_type;
+
+            target_type = g_strdup_printf("link<%s>", target->info->name);
+            if (strcmp(target_type, type) == 0) {
+                *child = target;
+            } else {
+                error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, type);
+            }
+
+            g_free(target_type);
+        } else {
+            error_set(errp, QERR_DEVICE_NOT_FOUND, path);
+        }
+    } else {
+        *child = NULL;
+    }
+
+    g_free(path);
+}
+
+void qdev_property_add_link(DeviceState *dev, const char *name,
+                            const char *type, DeviceState **child,
+                            Error **errp)
+{
+    gchar *full_type;
+
+    full_type = g_strdup_printf("link<%s>", type);
+
+    qdev_property_add(dev, name, full_type,
+                      qdev_get_link_property,
+                      qdev_set_link_property,
+                      NULL, child, errp);
+
+    g_free(full_type);
+}
+
+gchar *qdev_get_canonical_path(DeviceState *dev)
+{
+    DeviceState *root = qdev_get_root();
+    char *newpath = NULL, *path = NULL;
+
+    while (dev != root) {
+        GSList *i;
+
+        g_assert(dev->parent != NULL);
+
+        for (i = dev->parent->properties; i; i = i->next) {
+            DeviceProperty *prop = i->data;
+
+            if (!strstart(prop->type, "child<", NULL)) {
+                continue;
+            }
+
+            if (prop->opaque == dev) {
+                if (path) {
+                    newpath = g_strdup_printf("%s/%s", prop->name, path);
+                    g_free(path);
+                    path = newpath;
+                } else {
+                    path = g_strdup(prop->name);
+                }
+                break;
+            }
+        }
+
+        g_assert(i != NULL);
+
+        dev = dev->parent;
+    }
+
+    newpath = g_strdup_printf("/%s", path);
+    g_free(path);
+
+    return newpath;
+}
+
+static DeviceState *qdev_resolve_abs_path(DeviceState *parent,
+                                          gchar **parts,
+                                          int index)
+{
+    DeviceProperty *prop;
+    DeviceState *child;
+
+    if (parts[index] == NULL) {
+        return parent;
+    }
+
+    if (strcmp(parts[index], "") == 0) {
+        return qdev_resolve_abs_path(parent, parts, index + 1);
+    }
+
+    prop = qdev_property_find(parent, parts[index]);
+    if (prop == NULL) {
+        return NULL;
+    }
+
+    child = NULL;
+    if (strstart(prop->type, "link<", NULL)) {
+        DeviceState **pchild = prop->opaque;
+        if (*pchild) {
+            child = *pchild;
+        }
+    } else if (strstart(prop->type, "child<", NULL)) {
+        child = prop->opaque;
+    }
+
+    if (!child) {
+        return NULL;
+    }
+
+    return qdev_resolve_abs_path(child, parts, index + 1);
+}
+
+static DeviceState *qdev_resolve_partial_path(DeviceState *parent,
+                                              gchar **parts,
+                                              bool *ambiguous)
+{
+    DeviceState *dev;
+    GSList *i;
+
+    dev = qdev_resolve_abs_path(parent, parts, 0);
+
+    for (i = parent->properties; i; i = i->next) {
+        DeviceProperty *prop = i->data;
+        DeviceState *found;
+
+        if (!strstart(prop->type, "child<", NULL)) {
+            continue;
+        }
+
+        found = qdev_resolve_partial_path(prop->opaque, parts, ambiguous);
+        if (found) {
+            if (dev) {
+                if (ambiguous) {
+                    *ambiguous = true;
+                }
+                return NULL;
+            }
+            dev = found;
+        }
+
+        if (ambiguous && *ambiguous) {
+            return NULL;
+        }
+    }
+
+    return dev;
+}
+
+DeviceState *qdev_resolve_path(const char *path, bool *ambiguous)
+{
+    bool partial_path = true;
+    DeviceState *dev;
+    gchar **parts;
+
+    parts = g_strsplit(path, "/", 0);
+    if (parts == NULL || parts[0] == NULL) {
+        return qdev_get_root();
+    }
+
+    if (strcmp(parts[0], "") == 0) {
+        partial_path = false;
+    }
+
+    if (partial_path) {
+        dev = qdev_resolve_partial_path(qdev_get_root(), parts, ambiguous);
+    } else {
+        dev = qdev_resolve_abs_path(qdev_get_root(), parts, 1);
+    }
+
+    g_strfreev(parts);
+
+    return dev;
+}
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 00/18] qom: dynamic properties and composition tree

[Anthony Liguori]

On 11/30/2011 03:03 PM, Anthony Liguori wrote:
> This is a follow up to my previous series to get us started in the QOM
> direction.  A few things are different this time around.  Most notably:
>
>   1) Devices no longer have names.  Instead, path names are always used to
>      identify devices.
>
>   2) In order to support (1), dynamic properties are now supported.
>
>   3) The concept of a "root device" has been introduced.  The root device is
>      roughly modelling the motherboard of a machine.  This type is a container
>      type and it's best to think of it as something like a PCB board I guess.
>
> To try it out, here's an example session:
>
> Launch:
>
> anthony@titi:~/build/qemu$ x86_64-softmmu/qemu-system-x86_64 -hda ~/images/linux.img -snapshot -device virtio-balloon-pci,id=foo -qmp unix:/tmp/server.sock,server,nowait
>
> Explore the object model:
>
> anthony@titi:~/git/qemu/QMP$ ./qom-list /
> peripheral/
> i440fx/
> anthony@titi:~/git/qemu/QMP$ ./qom-list /i440fx/
> piix3/
> anthony@titi:~/git/qemu/QMP$ ./qom-list /i440fx/piix3
> rtc/
> anthony@titi:~/git/qemu/QMP$ ./qom-list /i440fx/piix3/rtc
> date
> base_year
> anthony@titi:~/git/qemu/QMP$ ./qom-get /i440fx/piix3/rtc.date
> tm_sec: 33
> tm_hour: 21
> tm_mday: 30
> tm_year: 111
> tm_mon: 10
> tm_min: 2
> anthony@titi:~/git/qemu/QMP$ ./qom-get rtc.date
> tm_sec: 38
> tm_hour: 21
> tm_mday: 30
> tm_year: 111
> tm_mon: 10
> tm_min: 2

It's probably worth adding that this is the short form.  The full path for rtc 
is /i440fx/piix4/rtc.  If user did -device virtio-pci-blk,id=rtc, then that 
device would have a full path for /peripheral/rtc.

If you did that, then just saying 'rtc.date' would through an error because the 
relative path 'rtc' is ambiguous.

The purpose of this is to give stable, unambiguous paths while still making it 
easy to use for a command line user.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 00/18] qom: dynamic properties and composition tree

[Avi Kivity]

On 11/30/2011 11:03 PM, Anthony Liguori wrote:
> This is a follow up to my previous series to get us started in the QOM
> direction.  A few things are different this time around.  Most notably:
>
>  1) Devices no longer have names.  Instead, path names are always used to
>     identify devices.
>
>  2) In order to support (1), dynamic properties are now supported.
>
>  3) The concept of a "root device" has been introduced.  The root device is
>     roughly modelling the motherboard of a machine.  This type is a container
>     type and it's best to think of it as something like a PCB board I guess.
>
> To try it out, here's an example session:
>
> Launch:
>
> anthony@titi:~/build/qemu$ x86_64-softmmu/qemu-system-x86_64 -hda ~/images/linux.img -snapshot -device virtio-balloon-pci,id=foo -qmp unix:/tmp/server.sock,server,nowait
>
> Explore the object model:
>
> anthony@titi:~/git/qemu/QMP$ ./qom-list /
> peripheral/
> i440fx/
> anthony@titi:~/git/qemu/QMP$ ./qom-list /i440fx/
> piix3/
> anthony@titi:~/git/qemu/QMP$ ./qom-list /i440fx/piix3
> rtc/
> anthony@titi:~/git/qemu/QMP$ ./qom-list /i440fx/piix3/rtc
> date
> base_year
> anthony@titi:~/git/qemu/QMP$ ./qom-get /i440fx/piix3/rtc.date
> tm_sec: 33
> tm_hour: 21
> tm_mday: 30
> tm_year: 111
> tm_mon: 10
> tm_min: 2
>

So all of these become ABIs, right?

We need good tools to allow easy review of the ABI bits hiding in
patches, and to maintain ABI compatibility.  Something like
qemu-print-abi that dumps all properties for all devices.  Patches could
show the ABI changes by including a diff of the output of this program
from before and after a change, and we could add similar tests for
backwards compatibility.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 00/18] qom: dynamic properties and composition tree

[Anthony Liguori]

On 12/01/2011 08:20 AM, Avi Kivity wrote:
> On 11/30/2011 11:03 PM, Anthony Liguori wrote:
>> This is a follow up to my previous series to get us started in the QOM
>> direction.  A few things are different this time around.  Most notably:
>>
>>   1) Devices no longer have names.  Instead, path names are always used to
>>      identify devices.
>>
>>   2) In order to support (1), dynamic properties are now supported.
>>
>>   3) The concept of a "root device" has been introduced.  The root device is
>>      roughly modelling the motherboard of a machine.  This type is a container
>>      type and it's best to think of it as something like a PCB board I guess.
>>
>> To try it out, here's an example session:
>>
>> Launch:
>>
>> anthony@titi:~/build/qemu$ x86_64-softmmu/qemu-system-x86_64 -hda ~/images/linux.img -snapshot -device virtio-balloon-pci,id=foo -qmp unix:/tmp/server.sock,server,nowait
>>
>> Explore the object model:
>>
>> anthony@titi:~/git/qemu/QMP$ ./qom-list /
>> peripheral/
>> i440fx/
>> anthony@titi:~/git/qemu/QMP$ ./qom-list /i440fx/
>> piix3/
>> anthony@titi:~/git/qemu/QMP$ ./qom-list /i440fx/piix3
>> rtc/
>> anthony@titi:~/git/qemu/QMP$ ./qom-list /i440fx/piix3/rtc
>> date
>> base_year
>> anthony@titi:~/git/qemu/QMP$ ./qom-get /i440fx/piix3/rtc.date
>> tm_sec: 33
>> tm_hour: 21
>> tm_mday: 30
>> tm_year: 111
>> tm_mon: 10
>> tm_min: 2
>>
>
> So all of these become ABIs, right?

At first, no.  I marked all of these QMP functions as experimental specifically 
to avoid introducing a new ABI.  I want to give us some time to work out things 
out a bit more.  But yes, this will eventually become an ABI.

>
> We need good tools to allow easy review of the ABI bits hiding in
> patches, and to maintain ABI compatibility.  Something like
> qemu-print-abi that dumps all properties for all devices.  Patches could
> show the ABI changes by including a diff of the output of this program
> from before and after a change, and we could add similar tests for
> backwards compatibility.

I'm not sure that we want this interface to be backwards compatible.  I actually 
think we should provide a higher level interface that's explicitly there for 
compatibility.  Probably in the form of a C library that can be documented and 
reasoned with better.

That turns the problem into something that requires less cleverness.

Regards,

Anthony Liguori

>

Re: [Qemu-devel] [PATCH 00/18] qom: dynamic properties and composition tree

[Avi Kivity]

On 12/01/2011 04:42 PM, Anthony Liguori wrote:
>
>>
>> We need good tools to allow easy review of the ABI bits hiding in
>> patches, and to maintain ABI compatibility.  Something like
>> qemu-print-abi that dumps all properties for all devices.  Patches could
>> show the ABI changes by including a diff of the output of this program
>> from before and after a change, and we could add similar tests for
>> backwards compatibility.
>
> I'm not sure that we want this interface to be backwards compatible. 
> I actually think we should provide a higher level interface that's
> explicitly there for compatibility.  Probably in the form of a C
> library that can be documented and reasoned with better.
>

Does this force anyone who wants a stable ABI to use this library?

I don't have a good picture of this library.  If FooState has a bar
propery, do you generate qemu_foo_get_bar() and qemu_foo_set_bar()
accessors?

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 00/18] qom: dynamic properties and composition tree

[Anthony Liguori]

On 12/01/2011 08:48 AM, Avi Kivity wrote:
> On 12/01/2011 04:42 PM, Anthony Liguori wrote:
>>
>>>
>>> We need good tools to allow easy review of the ABI bits hiding in
>>> patches, and to maintain ABI compatibility.  Something like
>>> qemu-print-abi that dumps all properties for all devices.  Patches could
>>> show the ABI changes by including a diff of the output of this program
>>> from before and after a change, and we could add similar tests for
>>> backwards compatibility.
>>
>> I'm not sure that we want this interface to be backwards compatible.
>> I actually think we should provide a higher level interface that's
>> explicitly there for compatibility.  Probably in the form of a C
>> library that can be documented and reasoned with better.
>>
>
> Does this force anyone who wants a stable ABI to use this library?
>
> I don't have a good picture of this library.  If FooState has a bar
> propery, do you generate qemu_foo_get_bar() and qemu_foo_set_bar()
> accessors?

This is all extremely low level stuff.  My view is that if you are a user that 
cares about backwards compatibility, you probably don't want all of this low 
level stuff in the first place.

I think we need to provide two classes of interfaces.  A low-level interface 
that only something like libvirt would consume that is not 100% backwards 
compatible and a higher-level interface that less sophisticated tools would consume.

If we provide a not 100% backwards compatible interface, then we would also need 
to provide a good introspection/capabilities mechanism so that something like 
libvirt could find out whether a feature was available or whether a device has 
changed significantly.

To be very clear, I think "-drive if=virtio" should be absolutely stable 
regardless of what we do to the virtio device model.  However, I want the 
flexibility to remove "-device virtio-blk-pci" and replace it with "-device 
virtio-pci,id=foo -device virtio-blk,bus=foo".

We would provide a means to enumerate supported devices so that things like 
libvirt could see that virtio-blk-pci is not valid in this new version but now 
there is a virtio-pci and virtio-blk device.

Regards,

Anthony Liguori

>