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
