| Message ID | 1543312923-3074-3-git-send-email-liq3ea@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw: vmmouse: use link property instead of DEFINE_PROP_PTR | expand |
Hi Li Qiang, This is only a suggestion, I'm sure someone else might also correct me, but I'm not sure the subject above really describes what is happening in the commit as a whole. It seems to miss the point that the main change here is to use a link type property, so maybe it might be better written as something along the lines of: Subject: [PATCH v2 2/2] hw: vmmouse: use link property instead of pointer Drop DEFINE_PROP_PTR() and replace with a link pointer via an instance init function (vmmouse_instance_initfn). But that's only my interpretation, you may have a better version. Thanks, Darren. On Tue, Nov 27, 2018 at 02:02:03AM -0800, Li Qiang wrote: >Use link property instead. > >Signed-off-by: Li Qiang <liq3ea@gmail.com> >--- > >Change since v1: use error_abort in object_property_set_link() > > hw/i386/pc.c | 3 ++- > hw/i386/vmmouse.c | 17 +++++++++++------ > 2 files changed, 13 insertions(+), 7 deletions(-) > >diff --git a/hw/i386/pc.c b/hw/i386/pc.c >index 73c7b777a0..64f0f233b8 100644 >--- a/hw/i386/pc.c >+++ b/hw/i386/pc.c >@@ -1552,7 +1552,8 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl, bool no_vmport) > } > if (vmmouse) { > DeviceState *dev = DEVICE(vmmouse); >- qdev_prop_set_ptr(dev, "ps2_mouse", i8042); >+ object_property_set_link(OBJECT(dev), OBJECT(i8042), >+ "ps2_mouse", &error_abort); > qdev_init_nofail(dev); > } > port92 = isa_create_simple(isa_bus, TYPE_PORT92); >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 >@@ -27,6 +27,7 @@ > #include "hw/i386/pc.h" > #include "hw/input/i8042.h" > #include "hw/qdev.h" >+#include "qapi/error.h" > > /* debug only vmmouse */ > //#define DEBUG_VMMOUSE >@@ -271,10 +272,15 @@ static void vmmouse_realizefn(DeviceState *dev, Error **errp) > vmport_register(VMMOUSE_DATA, vmmouse_ioport_read, s); > } > >-static Property vmmouse_properties[] = { >- DEFINE_PROP_PTR("ps2_mouse", VMMouseState, ps2_mouse), >- DEFINE_PROP_END_OF_LIST(), >-}; >+static void vmmouse_instance_initfn(Object *obj) >+{ >+ VMMouseState *s = VMMOUSE(obj); >+ >+ object_property_add_link(obj, "ps2_mouse", TYPE_I8042, >+ (Object **)&s->ps2_mouse, >+ qdev_prop_allow_set_link_before_realize, >+ 0, &error_abort); >+} > > static void vmmouse_class_initfn(ObjectClass *klass, void *data) > { >@@ -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; > } > >@@ -292,6 +296,7 @@ static const TypeInfo vmmouse_info = { > .name = TYPE_VMMOUSE, > .parent = TYPE_ISA_DEVICE, > .instance_size = sizeof(VMMouseState), >+ .instance_init = vmmouse_instance_initfn, > .class_init = vmmouse_class_initfn, > }; > >-- >2.11.0 > >
Darren Kenny <darren.kenny@oracle.com> writes: > Hi Li Qiang, > > This is only a suggestion, I'm sure someone else might also correct > me, but I'm not sure the subject above really describes what is > happening in the commit as a whole. > > It seems to miss the point that the main change here is to use a > link type property, so maybe it might be better written as something > along the lines of: > > Subject: [PATCH v2 2/2] hw: vmmouse: use link property instead of pointer > Drop DEFINE_PROP_PTR() and replace with a link pointer via an > instance init function (vmmouse_instance_initfn). Yes, that's better. Or take inspiration from recent similar work, such as commit a8299ec1b24, and say something like vmmouse: Use link instead of pointer property According to qdev-properties.h, properties of pointer type should be avoided. Turn "ps2_mouse" into a link. Preferably with the commit message improved along these lines: Reviewed-by: Markus Armbruster <armbru@redhat.com> [...]
On Tue, Nov 27, 2018 at 01:38:35PM +0100, Markus Armbruster wrote: >Darren Kenny <darren.kenny@oracle.com> writes: > >> Hi Li Qiang, >> >> This is only a suggestion, I'm sure someone else might also correct >> me, but I'm not sure the subject above really describes what is >> happening in the commit as a whole. >> >> It seems to miss the point that the main change here is to use a >> link type property, so maybe it might be better written as something >> along the lines of: >> >> Subject: [PATCH v2 2/2] hw: vmmouse: use link property instead of pointer >> Drop DEFINE_PROP_PTR() and replace with a link pointer via an >> instance init function (vmmouse_instance_initfn). > >Yes, that's better. Or take inspiration from recent similar work, such >as commit a8299ec1b24, and say something like > > vmmouse: Use link instead of pointer property > > According to qdev-properties.h, properties of pointer type should > be avoided. Turn "ps2_mouse" into a link. > >Preferably with the commit message improved along these lines: >Reviewed-by: Markus Armbruster <armbru@redhat.com> > >[...] > With something like that, Reviewed-by: Darren Kenny <darren.kenny@oracle.com> Thanks, Darren.
Markus Armbruster <armbru@redhat.com> 于2018年11月27日周二 下午8:38写道： > Darren Kenny <darren.kenny@oracle.com> writes: > > > Hi Li Qiang, > > > > This is only a suggestion, I'm sure someone else might also correct > > me, but I'm not sure the subject above really describes what is > > happening in the commit as a whole. > > > > It seems to miss the point that the main change here is to use a > > link type property, so maybe it might be better written as something > > along the lines of: > > > > Subject: [PATCH v2 2/2] hw: vmmouse: use link property instead of > pointer > > Drop DEFINE_PROP_PTR() and replace with a link pointer via an > > instance init function (vmmouse_instance_initfn). > > Yes, that's better. Or take inspiration from recent similar work, such > as commit a8299ec1b24, and say something like > > vmmouse: Use link instead of pointer property > > According to qdev-properties.h, properties of pointer type should > be avoided. Turn "ps2_mouse" into a link. > > Preferably with the commit message improved along these lines: > Reviewed-by: Markus Armbruster <armbru@redhat.com> > Hi Darren and Markus, Thanks so much for your advice. A new revision has been sent out. Thanks, Li Qiang > > [...] >
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 73c7b777a0..64f0f233b8 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1552,7 +1552,8 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl, bool no_vmport) } if (vmmouse) { DeviceState *dev = DEVICE(vmmouse); - qdev_prop_set_ptr(dev, "ps2_mouse", i8042); + object_property_set_link(OBJECT(dev), OBJECT(i8042), + "ps2_mouse", &error_abort); qdev_init_nofail(dev); } port92 = isa_create_simple(isa_bus, TYPE_PORT92); 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 @@ -27,6 +27,7 @@ #include "hw/i386/pc.h" #include "hw/input/i8042.h" #include "hw/qdev.h" +#include "qapi/error.h" /* debug only vmmouse */ //#define DEBUG_VMMOUSE @@ -271,10 +272,15 @@ static void vmmouse_realizefn(DeviceState *dev, Error **errp) vmport_register(VMMOUSE_DATA, vmmouse_ioport_read, s); } -static Property vmmouse_properties[] = { - DEFINE_PROP_PTR("ps2_mouse", VMMouseState, ps2_mouse), - DEFINE_PROP_END_OF_LIST(), -}; +static void vmmouse_instance_initfn(Object *obj) +{ + VMMouseState *s = VMMOUSE(obj); + + object_property_add_link(obj, "ps2_mouse", TYPE_I8042, + (Object **)&s->ps2_mouse, + qdev_prop_allow_set_link_before_realize, + 0, &error_abort); +} static void vmmouse_class_initfn(ObjectClass *klass, void *data) { @@ -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; } @@ -292,6 +296,7 @@ static const TypeInfo vmmouse_info = { .name = TYPE_VMMOUSE, .parent = TYPE_ISA_DEVICE, .instance_size = sizeof(VMMouseState), + .instance_init = vmmouse_instance_initfn, .class_init = vmmouse_class_initfn, };
Use link property instead. Signed-off-by: Li Qiang <liq3ea@gmail.com> --- Change since v1: use error_abort in object_property_set_link() hw/i386/pc.c | 3 ++- hw/i386/vmmouse.c | 17 +++++++++++------ 2 files changed, 13 insertions(+), 7 deletions(-)