On 25/05/20 08:38, Markus Armbruster wrote: > Paolo Bonzini <pbonz...@redhat.com> writes: > >> On 20/05/20 17:02, Markus Armbruster wrote: >>>>> >>>>> qdev_realize_and_unref() remains restricted, because its reference >>>>> counting would become rather confusing for bus-less devices. >>>> I think it would be fine, you would just rely on the reference held by >>>> the QOM parent (via the child property). >>> I took one look at the contract I wrote for it, and balked :) >>> >>> qdev_realize()'s contract before this patch: >>> >>> /* >>> * Realize @dev. >>> * @dev must not be plugged into a bus. >>> * Plug @dev into @bus. This takes a reference to @dev. >>> * If @dev has no QOM parent, make one up, taking another reference. >>> * On success, return true. >>> * On failure, store an error through @errp and return false. >>> */ >>> bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp) >>> >>> Simple enough. >>> >>> This patch merely adds "If @bus, " before "plug". Still simple enough. >>> >>> qdev_realize_and_unref()'s contract: >>> >>> /* >>> * Realize @dev and drop a reference. >>> * This is like qdev_realize(), except it steals a reference rather >>> * than take one to plug @dev into @bus. On failure, it drops that >>> * reference instead. @bus must not be null. Intended use: >>> * dev = qdev_new(); >>> * [...] >>> * qdev_realize_and_unref(dev, bus, errp); >>> * Now @dev can go away without further ado. >>> */ >>> bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error >>> **errp) >>> >>> If @bus is null, who gets to hold the stolen reference? >>> >>> You seem to suggest the QOM parent. What if @dev already has a parent? >> >> The caller would still hold the stolen reference, and it would be >> dropped. > > I read this sentence three times, and still don't get it. Is the > reference held or is it dropped?
To call qdev_realize_and_unref, you need to have your own reference, which you probably got from qdev_new. The function might add one via object_property_add_child or it might not; it might add one via qdev_set_parent_bus or it might not. But in any case, when it returns you won't have a reference anymore. One possibility is to think of it in terms of stealing the reference and passing it to the bus. However, as in the lifetime phases that I posted earlier, once you realize a device you are no longer in charge of its lifetime. Instead, the unparent callback will take care of unrealizing the device and dropping all outstanding long-living references. So... >> Or alternatively, ignore all the stolen references stuff, and merely see >> qdev_realize_and_unref as a shortcut for qdev_realize+object_unref, >> because it's a common idiom. > > Even common idioms need to make sense :) ... that's why the common idiom makes sense. > The contract must specify exactly what happens to the reference count, > case by case. For both qdev_realize and qdev_realize_and_unref, on return the caller need not care about keeping alive the device in the long-term. For qdev_realize_and_unref, the caller must _also_ have a "private" reference to the object, which will be dropped on return. For qdev_realize, the caller _can_ have a private reference that it has to later drop at a convenient time, but it could also ensure that the device has a long-term reference via object->parent instead. Perhaps this tells us that the /machine/unattached automation actually shouldn't be moved to qdev_realize, but rather to qdev_realize_and_unref, and qdev_realize could assert that there is a parent already at the time of the call. However it is probably too early to make a decision on that. Paolo
