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

[Li Qiang]

Markus Armbruster <address@hidden> 于2018年12月18日周二 下午3:10写道：

> 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.
>
>
Agree,

AFAIK there is no way to set link property in '-device'.
Anyway I think the maintainer can just merge or add this minor adjustment
to the patch.

Thanks,
Li Qiang



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