Re: [Qemu-devel] [PATCH v3 2/2] hw: vmmouse: Use link instead of pointer property

[Markus Armbruster]

Philippe Mathieu-Daudé <address@hidden> writes:

> On 12/17/18 8:01 PM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <address@hidden> writes:
>> 
>>> Hi Li,
>>>
>>> On 11/29/18 5:52 AM, Li Qiang wrote:
>>>> According to qdev-properties.h, properties of pointer type should
>>>> be avoided. Turn "ps2_mouse" into a link.
>>>>
>>>> Reviewed-by: Markus Armbruster <address@hidden>
>>>> Reviewed-by: Darren Kenny <address@hidden>
>>>> Signed-off-by: Li Qiang <address@hidden>
>>>> ---
>> [...]
>>>> diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
>>>> index 4412eaf604..f63aac6673 100644
>>>> --- a/hw/i386/vmmouse.c
>>>> +++ b/hw/i386/vmmouse.c
>> [...]
>>>> @@ -283,8 +289,6 @@ static void vmmouse_class_initfn(ObjectClass *klass, 
>>>> void *data)
>>>>      dc->realize = vmmouse_realizefn;
>>>>      dc->reset = vmmouse_reset;
>>>>      dc->vmsd = &vmstate_vmmouse;
>>>> -    dc->props = vmmouse_properties;
>>>> -    /* Reason: pointer property "ps2_mouse" */
>>>>      dc->user_creatable = false;
>>>
>>> "user_creatable = false" must have an justification comment.
>> 
>> Correct.

Aside: more ->user_creatable = false without a comment have crept in
since I last swept them out.

>>> Can you keep 'Reason: link property "ps2_mouse"'?
>> 
>> Is this a valid reason?
>> 
>> A pointer property is one, because it can only be set by code.
>
> I prefer the original comment :)
>
> /* Reason: pointer property "ps2_mouse" */

A pointer property can't be set by -device because there's no sane way
to pick a value.  Not true for a link property: it's value is a QOM
path.  But I'm not sure such setting of link properties has been
implemented.  If it isn't, then /* Reason: link property "ps2_mouse */
is perfect.  If it isn, then it's wrong, possibly along with with
dc->user_creatable = false.

>>> Eventually the maintainer taking this patch can fix this for you.
>>>
>>> With the comment:
>>> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>>>
>>>>  }
>> [...]
>>