| Message ID | 1322687028-29714-8-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
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/
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()?
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
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).
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 >
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 >
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
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 **.
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>?
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
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 >
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.
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
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
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.
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 > >
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
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 >
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
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(-)