| Message ID | 1431533649-23115-6-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 13.05.2015 um 18:14 schrieb Daniel P. Berrange: > +Object *object_new_with_propv(const char *typename, > + Object *parent, > + const char *id, > + Error **errp, > + va_list vargs) > +{ > + Object *obj; > + ObjectClass *klass; > + Error *local_err = NULL; > + > + klass = object_class_by_name(typename); > + if (!klass) { > + error_setg(errp, "invalid object type: %s", typename); > + return NULL; > + } > + > + if (object_class_is_abstract(klass)) { > + error_setg(errp, "object type '%s' is abstract", typename); > + return NULL; > + } > + obj = object_new(typename); > + > + if (object_set_propv(obj, &local_err, vargs) < 0) { > + goto error; > + } > + > + object_property_add_child(parent, id, obj, &local_err); > + if (local_err) { > + goto error; > + } > + > + if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) { > + user_creatable_complete(obj, &local_err); > + if (local_err) { > + object_unparent(obj); > + goto error; > + } > + } > + > + object_unref(OBJECT(obj)); > + return obj; This looks fishy. Either we return obj and need to retain a ref for that (caller's responsibility to unref) or we unref it and return void. Or am I misreading the code? > + > + error: > + if (local_err) { > + error_propagate(errp, local_err); > + } > + object_unref(obj); > + return NULL; > +} [snip] Rest looks good. Regards, Andreas
On Tue, May 19, 2015 at 05:52:14PM +0200, Andreas Färber wrote: > Am 13.05.2015 um 18:14 schrieb Daniel P. Berrange: > > +Object *object_new_with_propv(const char *typename, > > + Object *parent, > > + const char *id, > > + Error **errp, > > + va_list vargs) > > +{ > > + Object *obj; > > + ObjectClass *klass; > > + Error *local_err = NULL; > > + > > + klass = object_class_by_name(typename); > > + if (!klass) { > > + error_setg(errp, "invalid object type: %s", typename); > > + return NULL; > > + } > > + > > + if (object_class_is_abstract(klass)) { > > + error_setg(errp, "object type '%s' is abstract", typename); > > + return NULL; > > + } > > + obj = object_new(typename); > > + > > + if (object_set_propv(obj, &local_err, vargs) < 0) { > > + goto error; > > + } > > + > > + object_property_add_child(parent, id, obj, &local_err); > > + if (local_err) { > > + goto error; > > + } > > + > > + if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) { > > + user_creatable_complete(obj, &local_err); > > + if (local_err) { > > + object_unparent(obj); > > + goto error; > > + } > > + } > > + > > + object_unref(OBJECT(obj)); > > + return obj; > > This looks fishy. Either we return obj and need to retain a ref for that > (caller's responsibility to unref) or we unref it and return void. > Or am I misreading the code? Paolo told me on previous posting that object_property_add_child() holds a reference on 'obj' for as long as it is registered in the object hierarchy composition. So it sufficient to rely on that long term reference, and let the caller dispose of the object by calling object_unparent(obj) when finally done. Regards, Daniel
Am 19.05.2015 um 17:55 schrieb Daniel P. Berrange: > On Tue, May 19, 2015 at 05:52:14PM +0200, Andreas Färber wrote: >> Am 13.05.2015 um 18:14 schrieb Daniel P. Berrange: >>> +Object *object_new_with_propv(const char *typename, >>> + Object *parent, >>> + const char *id, >>> + Error **errp, >>> + va_list vargs) >>> +{ >>> + Object *obj; >>> + ObjectClass *klass; >>> + Error *local_err = NULL; >>> + >>> + klass = object_class_by_name(typename); >>> + if (!klass) { >>> + error_setg(errp, "invalid object type: %s", typename); >>> + return NULL; >>> + } >>> + >>> + if (object_class_is_abstract(klass)) { >>> + error_setg(errp, "object type '%s' is abstract", typename); >>> + return NULL; >>> + } >>> + obj = object_new(typename); >>> + >>> + if (object_set_propv(obj, &local_err, vargs) < 0) { >>> + goto error; >>> + } >>> + >>> + object_property_add_child(parent, id, obj, &local_err); >>> + if (local_err) { >>> + goto error; >>> + } >>> + >>> + if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) { >>> + user_creatable_complete(obj, &local_err); >>> + if (local_err) { >>> + object_unparent(obj); >>> + goto error; >>> + } >>> + } >>> + >>> + object_unref(OBJECT(obj)); >>> + return obj; >> >> This looks fishy. Either we return obj and need to retain a ref for that >> (caller's responsibility to unref) or we unref it and return void. >> Or am I misreading the code? > > Paolo told me on previous posting that object_property_add_child() > holds a reference on 'obj' for as long as it is registered in the > object hierarchy composition. So it sufficient to rely on that long > term reference, and let the caller dispose of the object by calling > object_unparent(obj) when finally done. Heh, that's why I like reviewing my patches myself. That reference can go away via QMP. For the CPU I have a patch pending to fix some corner cases. We can do that as follow-up here though. Regards, Andreas
On 19/05/2015 17:55, Daniel P. Berrange wrote: > Paolo told me on previous posting that object_property_add_child() > holds a reference on 'obj' for as long as it is registered in the > object hierarchy composition. So it sufficient to rely on that long > term reference, and let the caller dispose of the object by calling > object_unparent(obj) when finally done. For an example of the same pattern: DeviceState *qdev_try_create(BusState *bus, const char *type) { DeviceState *dev; if (object_class_by_name(type) == NULL) { return NULL; } dev = DEVICE(object_new(type)); if (!dev) { return NULL; } if (!bus) { bus = sysbus_get_default(); } qdev_set_parent_bus(dev, bus); object_unref(OBJECT(dev)); return dev; } Effectively this is idea as GObject's "floating reference". qdev_set_parent_bus (in qdev_try_create) and object_property_add_child (in Daniel's patches) "sink" the floating reference by doing object_unref. If we had floating references, the object would be returned to the caller unref'ed anyway. Of course, the reference can go away via QMP. But that will only happen after the caller would have called object_unref itself. Paolo
On Tue, May 19, 2015 at 06:11:05PM +0200, Paolo Bonzini wrote: > On 19/05/2015 17:55, Daniel P. Berrange wrote: > > Paolo told me on previous posting that object_property_add_child() > > holds a reference on 'obj' for as long as it is registered in the > > object hierarchy composition. So it sufficient to rely on that long > > term reference, and let the caller dispose of the object by calling > > object_unparent(obj) when finally done. > > For an example of the same pattern: > > DeviceState *qdev_try_create(BusState *bus, const char *type) > { > DeviceState *dev; > > if (object_class_by_name(type) == NULL) { > return NULL; > } > dev = DEVICE(object_new(type)); > if (!dev) { > return NULL; > } > > if (!bus) { > bus = sysbus_get_default(); > } > > qdev_set_parent_bus(dev, bus); > object_unref(OBJECT(dev)); > return dev; > } > > Effectively this is idea as GObject's "floating reference". > qdev_set_parent_bus (in qdev_try_create) and object_property_add_child > (in Daniel's patches) "sink" the floating reference by doing > object_unref. If we had floating references, the object would be > returned to the caller unref'ed anyway. I was agreeing with Andreas at first (because it would make the reference ownership rules simpler and easier to understand), until I noticed that every call of qdev_try_create() and object_resolve_path() in the code would need an additional object_unref() call if we didn't use this pattern. But it bothers me that this exceptional behavior is not documented on neither qdev_try_create() or object_resolve_path(). > > Of course, the reference can go away via QMP. But that will only happen > after the caller would have called object_unref itself. But the caller won't ever call object_unref() because it doesn't own any reference, right? In this case, can we clarify the rules about how long can callers safely expect the object to stay around? Can the object be disposed in another thread? Can it be disposed only when some specific events happen?
On Wed, May 20, 2015 at 11:44:19AM -0300, Eduardo Habkost wrote: > On Tue, May 19, 2015 at 06:11:05PM +0200, Paolo Bonzini wrote: > > On 19/05/2015 17:55, Daniel P. Berrange wrote: > > > Paolo told me on previous posting that object_property_add_child() > > > holds a reference on 'obj' for as long as it is registered in the > > > object hierarchy composition. So it sufficient to rely on that long > > > term reference, and let the caller dispose of the object by calling > > > object_unparent(obj) when finally done. > > > > For an example of the same pattern: > > > > DeviceState *qdev_try_create(BusState *bus, const char *type) > > { > > DeviceState *dev; > > > > if (object_class_by_name(type) == NULL) { > > return NULL; > > } > > dev = DEVICE(object_new(type)); > > if (!dev) { > > return NULL; > > } > > > > if (!bus) { > > bus = sysbus_get_default(); > > } > > > > qdev_set_parent_bus(dev, bus); > > object_unref(OBJECT(dev)); > > return dev; > > } > > > > Effectively this is idea as GObject's "floating reference". > > qdev_set_parent_bus (in qdev_try_create) and object_property_add_child > > (in Daniel's patches) "sink" the floating reference by doing > > object_unref. If we had floating references, the object would be > > returned to the caller unref'ed anyway. > > I was agreeing with Andreas at first (because it would make the > reference ownership rules simpler and easier to understand), until I > noticed that every call of qdev_try_create() and object_resolve_path() > in the code would need an additional object_unref() call if we didn't > use this pattern. > > But it bothers me that this exceptional behavior is not documented on > neither qdev_try_create() or object_resolve_path(). > > > > > Of course, the reference can go away via QMP. But that will only happen > > after the caller would have called object_unref itself. > > But the caller won't ever call object_unref() because it doesn't own any > reference, right? In this case, can we clarify the rules about how long > can callers safely expect the object to stay around? Can the object be > disposed in another thread? Can it be disposed only when some specific > events happen? In the inline docs for object_new_with_props I wrote * The returned object will have one stable reference maintained * for as long as it is present in the object hierarchy. We could expand it to explicitly say that 'object_unparent' is required to remove the object from the hierarchy and free it. Regards, Daniel
Am 20.05.2015 um 17:18 schrieb Daniel P. Berrange: > On Wed, May 20, 2015 at 11:44:19AM -0300, Eduardo Habkost wrote: >> On Tue, May 19, 2015 at 06:11:05PM +0200, Paolo Bonzini wrote: >>> On 19/05/2015 17:55, Daniel P. Berrange wrote: >>>> Paolo told me on previous posting that object_property_add_child() >>>> holds a reference on 'obj' for as long as it is registered in the >>>> object hierarchy composition. So it sufficient to rely on that long >>>> term reference, and let the caller dispose of the object by calling >>>> object_unparent(obj) when finally done. >>> >>> For an example of the same pattern: >>> >>> DeviceState *qdev_try_create(BusState *bus, const char *type) >>> { >>> DeviceState *dev; >>> >>> if (object_class_by_name(type) == NULL) { >>> return NULL; >>> } >>> dev = DEVICE(object_new(type)); >>> if (!dev) { >>> return NULL; >>> } >>> >>> if (!bus) { >>> bus = sysbus_get_default(); >>> } >>> >>> qdev_set_parent_bus(dev, bus); >>> object_unref(OBJECT(dev)); >>> return dev; >>> } >>> >>> Effectively this is idea as GObject's "floating reference". >>> qdev_set_parent_bus (in qdev_try_create) and object_property_add_child >>> (in Daniel's patches) "sink" the floating reference by doing >>> object_unref. If we had floating references, the object would be >>> returned to the caller unref'ed anyway. >> >> I was agreeing with Andreas at first (because it would make the >> reference ownership rules simpler and easier to understand), until I >> noticed that every call of qdev_try_create() and object_resolve_path() >> in the code would need an additional object_unref() call if we didn't >> use this pattern. >> >> But it bothers me that this exceptional behavior is not documented on >> neither qdev_try_create() or object_resolve_path(). >> >>> >>> Of course, the reference can go away via QMP. But that will only happen >>> after the caller would have called object_unref itself. >> >> But the caller won't ever call object_unref() because it doesn't own any >> reference, right? In this case, can we clarify the rules about how long >> can callers safely expect the object to stay around? Can the object be >> disposed in another thread? Can it be disposed only when some specific >> events happen? > > In the inline docs for object_new_with_props I wrote > > * The returned object will have one stable reference maintained > * for as long as it is present in the object hierarchy. > > We could expand it to explicitly say that 'object_unparent' is required > to remove the object from the hierarchy and free it. I've already queued this series - let's hold off changes for a bit. :) Regards, Andreas
On Wed, May 20, 2015 at 04:18:03PM +0100, Daniel P. Berrange wrote: > On Wed, May 20, 2015 at 11:44:19AM -0300, Eduardo Habkost wrote: > > On Tue, May 19, 2015 at 06:11:05PM +0200, Paolo Bonzini wrote: > > > On 19/05/2015 17:55, Daniel P. Berrange wrote: > > > > Paolo told me on previous posting that object_property_add_child() > > > > holds a reference on 'obj' for as long as it is registered in the > > > > object hierarchy composition. So it sufficient to rely on that long > > > > term reference, and let the caller dispose of the object by calling > > > > object_unparent(obj) when finally done. > > > > > > For an example of the same pattern: > > > > > > DeviceState *qdev_try_create(BusState *bus, const char *type) > > > { > > > DeviceState *dev; > > > > > > if (object_class_by_name(type) == NULL) { > > > return NULL; > > > } > > > dev = DEVICE(object_new(type)); > > > if (!dev) { > > > return NULL; > > > } > > > > > > if (!bus) { > > > bus = sysbus_get_default(); > > > } > > > > > > qdev_set_parent_bus(dev, bus); > > > object_unref(OBJECT(dev)); > > > return dev; > > > } > > > > > > Effectively this is idea as GObject's "floating reference". > > > qdev_set_parent_bus (in qdev_try_create) and object_property_add_child > > > (in Daniel's patches) "sink" the floating reference by doing > > > object_unref. If we had floating references, the object would be > > > returned to the caller unref'ed anyway. > > > > I was agreeing with Andreas at first (because it would make the > > reference ownership rules simpler and easier to understand), until I > > noticed that every call of qdev_try_create() and object_resolve_path() > > in the code would need an additional object_unref() call if we didn't > > use this pattern. > > > > But it bothers me that this exceptional behavior is not documented on > > neither qdev_try_create() or object_resolve_path(). > > > > > > > > Of course, the reference can go away via QMP. But that will only happen > > > after the caller would have called object_unref itself. > > > > But the caller won't ever call object_unref() because it doesn't own any > > reference, right? In this case, can we clarify the rules about how long > > can callers safely expect the object to stay around? Can the object be > > disposed in another thread? Can it be disposed only when some specific > > events happen? > > In the inline docs for object_new_with_props I wrote > > * The returned object will have one stable reference maintained > * for as long as it is present in the object hierarchy. > > We could expand it to explicitly say that 'object_unparent' is required > to remove the object from the hierarchy and free it. What's missing to me is some clarification on how long it is safe to assume that the object won't be removed from the hierarchy by other code.
On Wed, May 20, 2015 at 01:06:21PM -0300, Eduardo Habkost wrote: > On Wed, May 20, 2015 at 04:18:03PM +0100, Daniel P. Berrange wrote: > > On Wed, May 20, 2015 at 11:44:19AM -0300, Eduardo Habkost wrote: > > > On Tue, May 19, 2015 at 06:11:05PM +0200, Paolo Bonzini wrote: > > > > On 19/05/2015 17:55, Daniel P. Berrange wrote: > > > > > Paolo told me on previous posting that object_property_add_child() > > > > > holds a reference on 'obj' for as long as it is registered in the > > > > > object hierarchy composition. So it sufficient to rely on that long > > > > > term reference, and let the caller dispose of the object by calling > > > > > object_unparent(obj) when finally done. > > > > > > > > For an example of the same pattern: > > > > > > > > DeviceState *qdev_try_create(BusState *bus, const char *type) > > > > { > > > > DeviceState *dev; > > > > > > > > if (object_class_by_name(type) == NULL) { > > > > return NULL; > > > > } > > > > dev = DEVICE(object_new(type)); > > > > if (!dev) { > > > > return NULL; > > > > } > > > > > > > > if (!bus) { > > > > bus = sysbus_get_default(); > > > > } > > > > > > > > qdev_set_parent_bus(dev, bus); > > > > object_unref(OBJECT(dev)); > > > > return dev; > > > > } > > > > > > > > Effectively this is idea as GObject's "floating reference". > > > > qdev_set_parent_bus (in qdev_try_create) and object_property_add_child > > > > (in Daniel's patches) "sink" the floating reference by doing > > > > object_unref. If we had floating references, the object would be > > > > returned to the caller unref'ed anyway. > > > > > > I was agreeing with Andreas at first (because it would make the > > > reference ownership rules simpler and easier to understand), until I > > > noticed that every call of qdev_try_create() and object_resolve_path() > > > in the code would need an additional object_unref() call if we didn't > > > use this pattern. > > > > > > But it bothers me that this exceptional behavior is not documented on > > > neither qdev_try_create() or object_resolve_path(). > > > > > > > > > > > Of course, the reference can go away via QMP. But that will only happen > > > > after the caller would have called object_unref itself. > > > > > > But the caller won't ever call object_unref() because it doesn't own any > > > reference, right? In this case, can we clarify the rules about how long > > > can callers safely expect the object to stay around? Can the object be > > > disposed in another thread? Can it be disposed only when some specific > > > events happen? > > > > In the inline docs for object_new_with_props I wrote > > > > * The returned object will have one stable reference maintained > > * for as long as it is present in the object hierarchy. > > > > We could expand it to explicitly say that 'object_unparent' is required > > to remove the object from the hierarchy and free it. > > What's missing to me is some clarification on how long it is safe to > assume that the object won't be removed from the hierarchy by other > code. It seems that there is implicit assumption existing in QEMU, that objects will only be deleted from the hierarchy by code running in the main event loop thread. So if a method in the main event loop has got a pointer to the object it can assume it is safe to use. If anything spawns off a background thread, or passes the object pointer to a non-main loop thread, then it should first have acquired an extra reference on the object, and that thread would have to release the reference when done. I guess this needs documenting somewhere for clarity Regards, Daniel
Am 20.05.2015 um 18:06 schrieb Eduardo Habkost: > On Wed, May 20, 2015 at 04:18:03PM +0100, Daniel P. Berrange wrote: >> On Wed, May 20, 2015 at 11:44:19AM -0300, Eduardo Habkost wrote: >>> On Tue, May 19, 2015 at 06:11:05PM +0200, Paolo Bonzini wrote: >>>> On 19/05/2015 17:55, Daniel P. Berrange wrote: >>>>> Paolo told me on previous posting that object_property_add_child() >>>>> holds a reference on 'obj' for as long as it is registered in the >>>>> object hierarchy composition. So it sufficient to rely on that long >>>>> term reference, and let the caller dispose of the object by calling >>>>> object_unparent(obj) when finally done. >>>> >>>> For an example of the same pattern: >>>> >>>> DeviceState *qdev_try_create(BusState *bus, const char *type) >>>> { >>>> DeviceState *dev; >>>> >>>> if (object_class_by_name(type) == NULL) { >>>> return NULL; >>>> } >>>> dev = DEVICE(object_new(type)); >>>> if (!dev) { >>>> return NULL; >>>> } >>>> >>>> if (!bus) { >>>> bus = sysbus_get_default(); >>>> } >>>> >>>> qdev_set_parent_bus(dev, bus); >>>> object_unref(OBJECT(dev)); >>>> return dev; >>>> } >>>> >>>> Effectively this is idea as GObject's "floating reference". >>>> qdev_set_parent_bus (in qdev_try_create) and object_property_add_child >>>> (in Daniel's patches) "sink" the floating reference by doing >>>> object_unref. If we had floating references, the object would be >>>> returned to the caller unref'ed anyway. >>> >>> I was agreeing with Andreas at first (because it would make the >>> reference ownership rules simpler and easier to understand), until I >>> noticed that every call of qdev_try_create() and object_resolve_path() >>> in the code would need an additional object_unref() call if we didn't >>> use this pattern. >>> >>> But it bothers me that this exceptional behavior is not documented on >>> neither qdev_try_create() or object_resolve_path(). >>> >>>> >>>> Of course, the reference can go away via QMP. But that will only happen >>>> after the caller would have called object_unref itself. >>> >>> But the caller won't ever call object_unref() because it doesn't own any >>> reference, right? In this case, can we clarify the rules about how long >>> can callers safely expect the object to stay around? Can the object be >>> disposed in another thread? Can it be disposed only when some specific >>> events happen? >> >> In the inline docs for object_new_with_props I wrote >> >> * The returned object will have one stable reference maintained >> * for as long as it is present in the object hierarchy. >> >> We could expand it to explicitly say that 'object_unparent' is required >> to remove the object from the hierarchy and free it. > > What's missing to me is some clarification on how long it is safe to > assume that the object won't be removed from the hierarchy by other > code. There is no guarantee. Therefore any caller of object_resolve_path() must ref and unref as needed if doing more than a single operation, such as setting a link target. qdev is a different matter - there's too many instances to fix properly right away. The current code serves to balance the ref count for hot-unplug etc. to work. But it is not a template to copy for new QOM code IMO. Regards, Andreas
On Wed, May 20, 2015 at 06:11:33PM +0200, Andreas Färber wrote: > Am 20.05.2015 um 18:06 schrieb Eduardo Habkost: > > On Wed, May 20, 2015 at 04:18:03PM +0100, Daniel P. Berrange wrote: > >> On Wed, May 20, 2015 at 11:44:19AM -0300, Eduardo Habkost wrote: > >>> On Tue, May 19, 2015 at 06:11:05PM +0200, Paolo Bonzini wrote: > >>>> On 19/05/2015 17:55, Daniel P. Berrange wrote: > >>>>> Paolo told me on previous posting that object_property_add_child() > >>>>> holds a reference on 'obj' for as long as it is registered in the > >>>>> object hierarchy composition. So it sufficient to rely on that long > >>>>> term reference, and let the caller dispose of the object by calling > >>>>> object_unparent(obj) when finally done. > >>>> > >>>> For an example of the same pattern: > >>>> > >>>> DeviceState *qdev_try_create(BusState *bus, const char *type) > >>>> { > >>>> DeviceState *dev; > >>>> > >>>> if (object_class_by_name(type) == NULL) { > >>>> return NULL; > >>>> } > >>>> dev = DEVICE(object_new(type)); > >>>> if (!dev) { > >>>> return NULL; > >>>> } > >>>> > >>>> if (!bus) { > >>>> bus = sysbus_get_default(); > >>>> } > >>>> > >>>> qdev_set_parent_bus(dev, bus); > >>>> object_unref(OBJECT(dev)); > >>>> return dev; > >>>> } > >>>> > >>>> Effectively this is idea as GObject's "floating reference". > >>>> qdev_set_parent_bus (in qdev_try_create) and object_property_add_child > >>>> (in Daniel's patches) "sink" the floating reference by doing > >>>> object_unref. If we had floating references, the object would be > >>>> returned to the caller unref'ed anyway. > >>> > >>> I was agreeing with Andreas at first (because it would make the > >>> reference ownership rules simpler and easier to understand), until I > >>> noticed that every call of qdev_try_create() and object_resolve_path() > >>> in the code would need an additional object_unref() call if we didn't > >>> use this pattern. > >>> > >>> But it bothers me that this exceptional behavior is not documented on > >>> neither qdev_try_create() or object_resolve_path(). > >>> > >>>> > >>>> Of course, the reference can go away via QMP. But that will only happen > >>>> after the caller would have called object_unref itself. > >>> > >>> But the caller won't ever call object_unref() because it doesn't own any > >>> reference, right? In this case, can we clarify the rules about how long > >>> can callers safely expect the object to stay around? Can the object be > >>> disposed in another thread? Can it be disposed only when some specific > >>> events happen? > >> > >> In the inline docs for object_new_with_props I wrote > >> > >> * The returned object will have one stable reference maintained > >> * for as long as it is present in the object hierarchy. > >> > >> We could expand it to explicitly say that 'object_unparent' is required > >> to remove the object from the hierarchy and free it. > > > > What's missing to me is some clarification on how long it is safe to > > assume that the object won't be removed from the hierarchy by other > > code. > > There is no guarantee. Therefore any caller of object_resolve_path() > must ref and unref as needed if doing more than a single operation, such > as setting a link target. So, does that mean zero guarantee, or guarantee for "a single operation"? It can't be both. (And how exactly "a single operation" should be defined?)
Am 20.05.2015 um 18:22 schrieb Eduardo Habkost: > On Wed, May 20, 2015 at 06:11:33PM +0200, Andreas Färber wrote: >> Am 20.05.2015 um 18:06 schrieb Eduardo Habkost: >>> On Wed, May 20, 2015 at 04:18:03PM +0100, Daniel P. Berrange wrote: >>>> On Wed, May 20, 2015 at 11:44:19AM -0300, Eduardo Habkost wrote: >>>>> On Tue, May 19, 2015 at 06:11:05PM +0200, Paolo Bonzini wrote: >>>>>> On 19/05/2015 17:55, Daniel P. Berrange wrote: >>>>>>> Paolo told me on previous posting that object_property_add_child() >>>>>>> holds a reference on 'obj' for as long as it is registered in the >>>>>>> object hierarchy composition. So it sufficient to rely on that long >>>>>>> term reference, and let the caller dispose of the object by calling >>>>>>> object_unparent(obj) when finally done. >>>>>> >>>>>> For an example of the same pattern: >>>>>> >>>>>> DeviceState *qdev_try_create(BusState *bus, const char *type) >>>>>> { >>>>>> DeviceState *dev; >>>>>> >>>>>> if (object_class_by_name(type) == NULL) { >>>>>> return NULL; >>>>>> } >>>>>> dev = DEVICE(object_new(type)); >>>>>> if (!dev) { >>>>>> return NULL; >>>>>> } >>>>>> >>>>>> if (!bus) { >>>>>> bus = sysbus_get_default(); >>>>>> } >>>>>> >>>>>> qdev_set_parent_bus(dev, bus); >>>>>> object_unref(OBJECT(dev)); >>>>>> return dev; >>>>>> } >>>>>> >>>>>> Effectively this is idea as GObject's "floating reference". >>>>>> qdev_set_parent_bus (in qdev_try_create) and object_property_add_child >>>>>> (in Daniel's patches) "sink" the floating reference by doing >>>>>> object_unref. If we had floating references, the object would be >>>>>> returned to the caller unref'ed anyway. >>>>> >>>>> I was agreeing with Andreas at first (because it would make the >>>>> reference ownership rules simpler and easier to understand), until I >>>>> noticed that every call of qdev_try_create() and object_resolve_path() >>>>> in the code would need an additional object_unref() call if we didn't >>>>> use this pattern.changes >>>>> >>>>> But it bothers me that this exceptional behavior is not documented on >>>>> neither qdev_try_create() or object_resolve_path(). >>>>> >>>>>> >>>>>> Of course, the reference can go away via QMP. But that will only happen >>>>>> after the caller would have called object_unref itself. >>>>> >>>>> But the caller won't ever call object_unref() because it doesn't own any >>>>> reference, right? In this case, can we clarify the rules about how long >>>>> can callers safely expect the object to stay around? Can the object be >>>>> disposed in another thread? Can it be disposed only when some specific >>>>> events happen? >>>> >>>> In the inline docs for object_new_with_props I wrote >>>> >>>> * The returned object will have one stable reference maintained >>>> * for as long as it is present in the object hierarchy. >>>> >>>> We could expand it to explicitly say that 'object_unparent' is required >>>> to remove the object from the hierarchy and free it. >>> >>> What's missing to me is some clarification on how long it is safe to >>> assume that the object won't be removed from the hierarchy by other >>> code. >> >> There is no guarantee. Therefore any caller of object_resolve_path() >> must ref and unref as needed if doing more than a single operation, such >> as setting a link target. > > So, does that mean zero guarantee, or guarantee for "a single > operation"? It can't be both. > > (And how exactly "a single operation" should be defined?) I see your point, after all object_ref() is a single instruction, too. We could of course change it to return a ref'ed object, but that would require a lot of code review and probably unrefs. Mostly for objects that don't go away, but still. Andreas
On Wed, May 20, 2015 at 06:24:47PM +0200, Andreas Färber wrote: > Am 20.05.2015 um 18:22 schrieb Eduardo Habkost: > > On Wed, May 20, 2015 at 06:11:33PM +0200, Andreas Färber wrote: > >> Am 20.05.2015 um 18:06 schrieb Eduardo Habkost: > >>> On Wed, May 20, 2015 at 04:18:03PM +0100, Daniel P. Berrange wrote: > >>>> On Wed, May 20, 2015 at 11:44:19AM -0300, Eduardo Habkost wrote: > >>>>> On Tue, May 19, 2015 at 06:11:05PM +0200, Paolo Bonzini wrote: > >>>>>> On 19/05/2015 17:55, Daniel P. Berrange wrote: > >>>>>>> Paolo told me on previous posting that object_property_add_child() > >>>>>>> holds a reference on 'obj' for as long as it is registered in the > >>>>>>> object hierarchy composition. So it sufficient to rely on that long > >>>>>>> term reference, and let the caller dispose of the object by calling > >>>>>>> object_unparent(obj) when finally done. > >>>>>> > >>>>>> For an example of the same pattern: > >>>>>> > >>>>>> DeviceState *qdev_try_create(BusState *bus, const char *type) > >>>>>> { > >>>>>> DeviceState *dev; > >>>>>> > >>>>>> if (object_class_by_name(type) == NULL) { > >>>>>> return NULL; > >>>>>> } > >>>>>> dev = DEVICE(object_new(type)); > >>>>>> if (!dev) { > >>>>>> return NULL; > >>>>>> } > >>>>>> > >>>>>> if (!bus) { > >>>>>> bus = sysbus_get_default(); > >>>>>> } > >>>>>> > >>>>>> qdev_set_parent_bus(dev, bus); > >>>>>> object_unref(OBJECT(dev)); > >>>>>> return dev; > >>>>>> } > >>>>>> > >>>>>> Effectively this is idea as GObject's "floating reference". > >>>>>> qdev_set_parent_bus (in qdev_try_create) and object_property_add_child > >>>>>> (in Daniel's patches) "sink" the floating reference by doing > >>>>>> object_unref. If we had floating references, the object would be > >>>>>> returned to the caller unref'ed anyway. > >>>>> > >>>>> I was agreeing with Andreas at first (because it would make the > >>>>> reference ownership rules simpler and easier to understand), until I > >>>>> noticed that every call of qdev_try_create() and object_resolve_path() > >>>>> in the code would need an additional object_unref() call if we didn't > >>>>> use this pattern.changes > >>>>> > >>>>> But it bothers me that this exceptional behavior is not documented on > >>>>> neither qdev_try_create() or object_resolve_path(). > >>>>> > >>>>>> > >>>>>> Of course, the reference can go away via QMP. But that will only happen > >>>>>> after the caller would have called object_unref itself. > >>>>> > >>>>> But the caller won't ever call object_unref() because it doesn't own any > >>>>> reference, right? In this case, can we clarify the rules about how long > >>>>> can callers safely expect the object to stay around? Can the object be > >>>>> disposed in another thread? Can it be disposed only when some specific > >>>>> events happen? > >>>> > >>>> In the inline docs for object_new_with_props I wrote > >>>> > >>>> * The returned object will have one stable reference maintained > >>>> * for as long as it is present in the object hierarchy. > >>>> > >>>> We could expand it to explicitly say that 'object_unparent' is required > >>>> to remove the object from the hierarchy and free it. > >>> > >>> What's missing to me is some clarification on how long it is safe to > >>> assume that the object won't be removed from the hierarchy by other > >>> code. > >> > >> There is no guarantee. Therefore any caller of object_resolve_path() > >> must ref and unref as needed if doing more than a single operation, such > >> as setting a link target. > > > > So, does that mean zero guarantee, or guarantee for "a single > > operation"? It can't be both. > > > > (And how exactly "a single operation" should be defined?) > > I see your point, after all object_ref() is a single instruction, too. > > We could of course change it to return a ref'ed object, but that would > require a lot of code review and probably unrefs. Mostly for objects > that don't go away, but still. I don't think we should change it to return a ref'ed object, the existing "floating reference" pattern is useful and keeps code simpler. But even if we have plans to change it later, it would be nice to clarify what are the assumptions/guarantees of the existing code.
diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h index ac7c4c4..df9dd51 100644 --- a/include/qemu/compiler.h +++ b/include/qemu/compiler.h @@ -24,6 +24,12 @@ #define QEMU_WARN_UNUSED_RESULT #endif +#if QEMU_GNUC_PREREQ(4, 0) +#define QEMU_SENTINEL __attribute__((sentinel)) +#else +#define QEMU_SENTINEL +#endif + #if QEMU_GNUC_PREREQ(4, 3) #define QEMU_ARTIFICIAL __attribute__((always_inline, artificial)) #else diff --git a/include/qom/object.h b/include/qom/object.h index d30c68c..fcacdb0 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -607,6 +607,134 @@ Object *object_new(const char *typename); Object *object_new_with_type(Type type); /** + * object_new_with_props: + * @typename: The name of the type of the object to instantiate. + * @parent: the parent object + * @id: The unique ID of the object + * @errp: pointer to error object + * @...: list of property names and values + * + * This function will initialize a new object using heap allocated memory. + * The returned object has a reference count of 1, and will be freed when + * the last reference is dropped. + * + * The @id parameter will be used when registering the object as a + * child of @parent in the objects composition tree. + * + * The variadic parameters are a list of pairs of (propname, propvalue) + * strings. The propname of %NULL indicates the end of the property + * list. If the object implements the user creatable interface, the + * object will be marked complete once all the properties have been + * processed. + * + * <example> + * <title>Creating an object with properties</title> + * <programlisting> + * Error *err = NULL; + * Object *obj; + * + * obj = object_new_with_props(TYPE_MEMORY_BACKEND_FILE, + * object_get_objects_root(), + * "hostmem0", + * &err, + * "share", "yes", + * "mem-path", "/dev/shm/somefile", + * "prealloc", "yes", + * "size", "1048576", + * NULL); + * + * if (!obj) { + * g_printerr("Cannot create memory backend: %s\n", + * error_get_pretty(err)); + * } + * </programlisting> + * </example> + * + * The returned object will have one stable reference maintained + * for as long as it is present in the object hierarchy. + * + * Returns: The newly allocated, instantiated & initialized object. + */ +Object *object_new_with_props(const char *typename, + Object *parent, + const char *id, + Error **errp, + ...) QEMU_SENTINEL; + +/** + * object_new_with_propv: + * @typename: The name of the type of the object to instantiate. + * @parent: the parent object + * @id: The unique ID of the object + * @errp: pointer to error object + * @vargs: list of property names and values + * + * See object_new_with_props() for documentation. + */ +Object *object_new_with_propv(const char *typename, + Object *parent, + const char *id, + Error **errp, + va_list vargs); + +/** + * object_set_props: + * @obj: the object instance to set properties on + * @errp: pointer to error object + * @...: list of property names and values + * + * This function will set a list of properties on an existing object + * instance. + * + * The variadic parameters are a list of pairs of (propname, propvalue) + * strings. The propname of %NULL indicates the end of the property + * list. + * + * <example> + * <title>Update an object's properties</title> + * <programlisting> + * Error *err = NULL; + * Object *obj = ...get / create object...; + * + * obj = object_set_props(obj, + * &err, + * "share", "yes", + * "mem-path", "/dev/shm/somefile", + * "prealloc", "yes", + * "size", "1048576", + * NULL); + * + * if (!obj) { + * g_printerr("Cannot set properties: %s\n", + * error_get_pretty(err)); + * } + * </programlisting> + * </example> + * + * The returned object will have one stable reference maintained + * for as long as it is present in the object hierarchy. + * + * Returns: -1 on error, 0 on success + */ +int object_set_props(Object *obj, + Error **errp, + ...) QEMU_SENTINEL; + +/** + * object_set_propv: + * @obj: the object instance to set properties on + * @errp: pointer to error object + * @vargs: list of property names and values + * + * See object_set_props() for documentation. + * + * Returns: -1 on error, 0 on success + */ +int object_set_propv(Object *obj, + Error **errp, + va_list vargs); + +/** * object_initialize_with_type: * @data: A pointer to the memory to be used for the object. * @size: The maximum size available at @data for the object. diff --git a/qom/object.c b/qom/object.c index baeece2..55ab081 100644 --- a/qom/object.c +++ b/qom/object.c @@ -11,6 +11,7 @@ */ #include "qom/object.h" +#include "qom/object_interfaces.h" #include "qemu-common.h" #include "qapi/visitor.h" #include "qapi-visit.h" @@ -439,6 +440,114 @@ Object *object_new(const char *typename) return object_new_with_type(ti); } + +Object *object_new_with_props(const char *typename, + Object *parent, + const char *id, + Error **errp, + ...) +{ + va_list vargs; + Object *obj; + + va_start(vargs, errp); + obj = object_new_with_propv(typename, parent, id, errp, vargs); + va_end(vargs); + + return obj; +} + + +Object *object_new_with_propv(const char *typename, + Object *parent, + const char *id, + Error **errp, + va_list vargs) +{ + Object *obj; + ObjectClass *klass; + Error *local_err = NULL; + + klass = object_class_by_name(typename); + if (!klass) { + error_setg(errp, "invalid object type: %s", typename); + return NULL; + } + + if (object_class_is_abstract(klass)) { + error_setg(errp, "object type '%s' is abstract", typename); + return NULL; + } + obj = object_new(typename); + + if (object_set_propv(obj, &local_err, vargs) < 0) { + goto error; + } + + object_property_add_child(parent, id, obj, &local_err); + if (local_err) { + goto error; + } + + if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) { + user_creatable_complete(obj, &local_err); + if (local_err) { + object_unparent(obj); + goto error; + } + } + + object_unref(OBJECT(obj)); + return obj; + + error: + if (local_err) { + error_propagate(errp, local_err); + } + object_unref(obj); + return NULL; +} + + +int object_set_props(Object *obj, + Error **errp, + ...) +{ + va_list vargs; + int ret; + + va_start(vargs, errp); + ret = object_set_propv(obj, errp, vargs); + va_end(vargs); + + return ret; +} + + +int object_set_propv(Object *obj, + Error **errp, + va_list vargs) +{ + const char *propname; + Error *local_err = NULL; + + propname = va_arg(vargs, char *); + while (propname != NULL) { + const char *value = va_arg(vargs, char *); + + g_assert(value != NULL); + object_property_parse(obj, value, propname, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return -1; + } + propname = va_arg(vargs, char *); + } + + return 0; +} + + Object *object_dynamic_cast(Object *obj, const char *typename) { if (obj && object_class_dynamic_cast(object_get_class(obj), typename)) { diff --git a/tests/.gitignore b/tests/.gitignore index 0dcb618..dc813c2 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -5,6 +5,7 @@ check-qjson check-qlist check-qstring check-qom-interface +check-qom-proplist rcutorture test-aio test-bitops diff --git a/tests/Makefile b/tests/Makefile index 666aee2..1973d35 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -68,6 +68,8 @@ check-unit-y += tests/test-bitops$(EXESUF) check-unit-$(CONFIG_HAS_GLIB_SUBPROCESS_TESTS) += tests/test-qdev-global-props$(EXESUF) check-unit-y += tests/check-qom-interface$(EXESUF) gcov-files-check-qom-interface-y = qom/object.c +check-unit-y += tests/check-qom-proplist$(EXESUF) +gcov-files-check-qom-proplist-y = qom/object.c check-unit-y += tests/test-qemu-opts$(EXESUF) gcov-files-test-qemu-opts-y = qom/test-qemu-opts.c check-unit-y += tests/test-write-threshold$(EXESUF) @@ -264,7 +266,7 @@ test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o \ $(test-obj-y): QEMU_INCLUDES += -Itests QEMU_CFLAGS += -I$(SRC_PATH)/tests -qom-core-obj = qom/object.o qom/qom-qobject.o qom/container.o +qom-core-obj = qom/object.o qom/qom-qobject.o qom/container.o qom/object_interfaces.o tests/check-qint$(EXESUF): tests/check-qint.o libqemuutil.a tests/check-qstring$(EXESUF): tests/check-qstring.o libqemuutil.a @@ -273,6 +275,7 @@ tests/check-qlist$(EXESUF): tests/check-qlist.o libqemuutil.a tests/check-qfloat$(EXESUF): tests/check-qfloat.o libqemuutil.a tests/check-qjson$(EXESUF): tests/check-qjson.o libqemuutil.a libqemustub.a tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o $(qom-core-obj) libqemuutil.a libqemustub.a +tests/check-qom-proplist$(EXESUF): tests/check-qom-proplist.o $(qom-core-obj) libqemuutil.a libqemustub.a tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(block-obj-y) libqemuutil.a libqemustub.a tests/test-aio$(EXESUF): tests/test-aio.o $(block-obj-y) libqemuutil.a libqemustub.a tests/test-rfifolock$(EXESUF): tests/test-rfifolock.o libqemuutil.a libqemustub.a diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c new file mode 100644 index 0000000..e82532e --- /dev/null +++ b/tests/check-qom-proplist.c @@ -0,0 +1,186 @@ +/* + * Copyright (C) 2015 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Daniel P. Berrange <berrange@redhat.com> + */ + +#include <glib.h> + +#include "qom/object.h" +#include "qemu/module.h" + + +#define TYPE_DUMMY "qemu-dummy" + +typedef struct DummyObject DummyObject; +typedef struct DummyObjectClass DummyObjectClass; + +#define DUMMY_OBJECT(obj) \ + OBJECT_CHECK(DummyObject, (obj), TYPE_DUMMY) + +struct DummyObject { + Object parent_obj; + + bool bv; + char *sv; +}; + +struct DummyObjectClass { + ObjectClass parent_class; +}; + + +static void dummy_set_bv(Object *obj, + bool value, + Error **errp) +{ + DummyObject *dobj = DUMMY_OBJECT(obj); + + dobj->bv = value; +} + +static bool dummy_get_bv(Object *obj, + Error **errp) +{ + DummyObject *dobj = DUMMY_OBJECT(obj); + + return dobj->bv; +} + + +static void dummy_set_sv(Object *obj, + const char *value, + Error **errp) +{ + DummyObject *dobj = DUMMY_OBJECT(obj); + + g_free(dobj->sv); + dobj->sv = g_strdup(value); +} + +static char *dummy_get_sv(Object *obj, + Error **errp) +{ + DummyObject *dobj = DUMMY_OBJECT(obj); + + return g_strdup(dobj->sv); +} + + +static void dummy_init(Object *obj) +{ + object_property_add_bool(obj, "bv", + dummy_get_bv, + dummy_set_bv, + NULL); + object_property_add_str(obj, "sv", + dummy_get_sv, + dummy_set_sv, + NULL); +} + +static void dummy_finalize(Object *obj) +{ + DummyObject *dobj = DUMMY_OBJECT(obj); + + g_free(dobj->sv); +} + + +static const TypeInfo dummy_info = { + .name = TYPE_DUMMY, + .parent = TYPE_OBJECT, + .instance_size = sizeof(DummyObject), + .instance_init = dummy_init, + .instance_finalize = dummy_finalize, + .class_size = sizeof(DummyObjectClass), +}; + +static void test_dummy_createv(void) +{ + Error *err = NULL; + Object *parent = object_get_objects_root(); + DummyObject *dobj = DUMMY_OBJECT( + object_new_with_props(TYPE_DUMMY, + parent, + "dummy0", + &err, + "bv", "yes", + "sv", "Hiss hiss hiss", + NULL)); + + g_assert(err == NULL); + g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss"); + g_assert(dobj->bv == true); + + g_assert(object_resolve_path_component(parent, "dummy0") + == OBJECT(dobj)); + + object_unparent(OBJECT(dobj)); +} + + +static Object *new_helper(Error **errp, + Object *parent, + ...) +{ + va_list vargs; + Object *obj; + + va_start(vargs, parent); + obj = object_new_with_propv(TYPE_DUMMY, + parent, + "dummy0", + errp, + vargs); + va_end(vargs); + return obj; +} + +static void test_dummy_createlist(void) +{ + Error *err = NULL; + Object *parent = object_get_objects_root(); + DummyObject *dobj = DUMMY_OBJECT( + new_helper(&err, + parent, + "bv", "yes", + "sv", "Hiss hiss hiss", + NULL)); + + g_assert(err == NULL); + g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss"); + g_assert(dobj->bv == true); + + g_assert(object_resolve_path_component(parent, "dummy0") + == OBJECT(dobj)); + + object_unparent(OBJECT(dobj)); +} + +int main(int argc, char **argv) +{ + g_test_init(&argc, &argv, NULL); + + module_call_init(MODULE_INIT_QOM); + type_register_static(&dummy_info); + + g_test_add_func("/qom/proplist/createlist", test_dummy_createlist); + g_test_add_func("/qom/proplist/createv", test_dummy_createv); + + return g_test_run(); +}
It is reasonably common to want to create an object, set a number of properties, register it in the hierarchy and then mark it as complete (if a user creatable type). This requires quite a lot of error prone, verbose, boilerplate code to achieve. First a pair of methods object_set_props / object_set_propv are added which allow for a list of objects to be set in one single API call. Then object_new_with_props / object_new_with_propv constructors are added which simplify the sequence of calls to create an object, populate properties, register in the object composition tree and mark the object complete, into a single method call. Usage would be: Error *err = NULL; Object *obj; obj = object_new_with_propv(TYPE_MEMORY_BACKEND_FILE, object_get_objects_root(), "hostmem0", &err, "share", "yes", "mem-path", "/dev/shm/somefile", "prealloc", "yes", "size", "1048576", NULL); Note all property values are passed in string form and will be parsed into their required data types, using normal QOM semantics for parsing from string format. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- include/qemu/compiler.h | 6 ++ include/qom/object.h | 128 +++++++++++++++++++++++++++++++ qom/object.c | 109 ++++++++++++++++++++++++++ tests/.gitignore | 1 + tests/Makefile | 5 +- tests/check-qom-proplist.c | 186 +++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 434 insertions(+), 1 deletion(-) create mode 100644 tests/check-qom-proplist.c