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

[Anthony Liguori <anthony@codemonkey.ws>]

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
>