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