Patchwork [07/18] qom: add link properties

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

Comments

Anthony Liguori - Nov. 30, 2011, 9:03 p.m.
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(-)
Stefan Hajnoczi - Dec. 1, 2011, 10:55 a.m.
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/
Avi Kivity - Dec. 1, 2011, 11:21 a.m.
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()?
Stefan Hajnoczi - Dec. 1, 2011, 11:35 a.m.
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
Avi Kivity - Dec. 1, 2011, 12:34 p.m.
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).
Anthony Liguori - Dec. 1, 2011, 1:40 p.m.
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

>
Anthony Liguori - Dec. 1, 2011, 1:44 p.m.
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

>
Anthony Liguori - Dec. 1, 2011, 1:47 p.m.
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
Avi Kivity - Dec. 1, 2011, 1:50 p.m.
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 **.
Avi Kivity - Dec. 1, 2011, 2:03 p.m.
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>?
Anthony Liguori - Dec. 1, 2011, 2:53 p.m.
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
Anthony Liguori - Dec. 1, 2011, 2:56 p.m.
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

>
Avi Kivity - Dec. 1, 2011, 3 p.m.
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.
Gerd Hoffmann - Dec. 1, 2011, 3:03 p.m.
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
Anthony Liguori - Dec. 1, 2011, 3:10 p.m.
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
Avi Kivity - Dec. 1, 2011, 3:13 p.m.
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.
Anthony Liguori - Dec. 1, 2011, 3:14 p.m.
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
>
>
Kevin Wolf - Dec. 2, 2011, 12:15 p.m.
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
Anthony Liguori - Dec. 2, 2011, 2:57 p.m.
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
>

Patch

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