Re: [Qemu-devel] [PATCH 07/18] qom: add link properties

[Anthony Liguori <anthony@codemonkey.ws>]

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