| Message ID | 1339097465-22977-2-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
On 06/08/2012 03:30 AM, Andreas Färber wrote: > From: Paolo Bonzini<pbonzini@redhat.com> > > qdev properties use the state member (an embryo of the "realized" > property) in order to disable setting them after a device has been > initialized. So, in order to push qdev properties up to Object > we need to push this bit there too. > > Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> > [AF: Rename to OBJECT_STATE_INITIALIZED and set it after instance_init.] > Signed-off-by: Andreas Färber<afaerber@suse.de> > --- > hw/qdev-addr.c | 3 ++- > hw/qdev-properties.c | 26 +++++++++++++------------- > hw/qdev.c | 11 +++++------ > hw/qdev.h | 6 ------ > include/qemu/object.h | 14 ++++++++++++++ > qom/object.c | 7 +++++++ > 6 files changed, 41 insertions(+), 26 deletions(-) > > diff --git a/hw/qdev-addr.c b/hw/qdev-addr.c > index b711b6b..a3796bd 100644 > --- a/hw/qdev-addr.c > +++ b/hw/qdev-addr.c > @@ -1,3 +1,4 @@ > +#include "qemu/object.h" > #include "qdev.h" > #include "qdev-addr.h" > #include "targphys.h" > @@ -39,7 +40,7 @@ static void set_taddr(Object *obj, Visitor *v, void *opaque, > Error *local_err = NULL; > int64_t value; > > - if (dev->state != DEV_STATE_CREATED) { > + if (object_is_realized(obj)) { > error_set(errp, QERR_PERMISSION_DENIED); > return; > } > diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c > index 099a7aa..fcc0bed 100644 > --- a/hw/qdev-properties.c > +++ b/hw/qdev-properties.c > @@ -53,7 +53,7 @@ static void set_bit(Object *obj, Visitor *v, void *opaque, > Error *local_err = NULL; > bool value; > > - if (dev->state != DEV_STATE_CREATED) { > + if (object_is_realized(obj)) { > error_set(errp, QERR_PERMISSION_DENIED); > return; > } > @@ -93,7 +93,7 @@ static void set_uint8(Object *obj, Visitor *v, void *opaque, > Property *prop = opaque; > uint8_t *ptr = qdev_get_prop_ptr(dev, prop); > > - if (dev->state != DEV_STATE_CREATED) { > + if (object_is_realized(obj)) { > error_set(errp, QERR_PERMISSION_DENIED); > return; > } > @@ -160,7 +160,7 @@ static void set_uint16(Object *obj, Visitor *v, void *opaque, > Property *prop = opaque; > uint16_t *ptr = qdev_get_prop_ptr(dev, prop); > > - if (dev->state != DEV_STATE_CREATED) { > + if (object_is_realized(obj)) { > error_set(errp, QERR_PERMISSION_DENIED); > return; > } > @@ -193,7 +193,7 @@ static void set_uint32(Object *obj, Visitor *v, void *opaque, > Property *prop = opaque; > uint32_t *ptr = qdev_get_prop_ptr(dev, prop); > > - if (dev->state != DEV_STATE_CREATED) { > + if (object_is_realized(obj)) { > error_set(errp, QERR_PERMISSION_DENIED); > return; > } > @@ -218,7 +218,7 @@ static void set_int32(Object *obj, Visitor *v, void *opaque, > Property *prop = opaque; > int32_t *ptr = qdev_get_prop_ptr(dev, prop); > > - if (dev->state != DEV_STATE_CREATED) { > + if (object_is_realized(obj)) { > error_set(errp, QERR_PERMISSION_DENIED); > return; > } > @@ -291,7 +291,7 @@ static void set_uint64(Object *obj, Visitor *v, void *opaque, > Property *prop = opaque; > uint64_t *ptr = qdev_get_prop_ptr(dev, prop); > > - if (dev->state != DEV_STATE_CREATED) { > + if (object_is_realized(obj)) { > error_set(errp, QERR_PERMISSION_DENIED); > return; > } > @@ -379,7 +379,7 @@ static void set_string(Object *obj, Visitor *v, void *opaque, > Error *local_err = NULL; > char *str; > > - if (dev->state != DEV_STATE_CREATED) { > + if (object_is_realized(obj)) { > error_set(errp, QERR_PERMISSION_DENIED); > return; > } > @@ -457,7 +457,7 @@ static void set_pointer(Object *obj, Visitor *v, Property *prop, > char *str; > int ret; > > - if (dev->state != DEV_STATE_CREATED) { > + if (object_is_realized(obj)) { > error_set(errp, QERR_PERMISSION_DENIED); > return; > } > @@ -626,7 +626,7 @@ static void set_vlan(Object *obj, Visitor *v, void *opaque, > int64_t id; > VLANState *vlan; > > - if (dev->state != DEV_STATE_CREATED) { > + if (object_is_realized(obj)) { > error_set(errp, QERR_PERMISSION_DENIED); > return; > } > @@ -696,7 +696,7 @@ static void set_mac(Object *obj, Visitor *v, void *opaque, > int i, pos; > char *str, *p; > > - if (dev->state != DEV_STATE_CREATED) { > + if (object_is_realized(obj)) { > error_set(errp, QERR_PERMISSION_DENIED); > return; > } > @@ -766,7 +766,7 @@ static void set_enum(Object *obj, Visitor *v, void *opaque, > Property *prop = opaque; > int *ptr = qdev_get_prop_ptr(dev, prop); > > - if (dev->state != DEV_STATE_CREATED) { > + if (object_is_realized(obj)) { > error_set(errp, QERR_PERMISSION_DENIED); > return; > } > @@ -797,7 +797,7 @@ static void set_pci_devfn(Object *obj, Visitor *v, void *opaque, > Error *local_err = NULL; > char *str; > > - if (dev->state != DEV_STATE_CREATED) { > + if (object_is_realized(obj)) { > error_set(errp, QERR_PERMISSION_DENIED); > return; > } > @@ -867,7 +867,7 @@ static void set_blocksize(Object *obj, Visitor *v, void *opaque, > const int64_t min = 512; > const int64_t max = 32768; > > - if (dev->state != DEV_STATE_CREATED) { > + if (object_is_realized(obj)) { > error_set(errp, QERR_PERMISSION_DENIED); > return; > } > diff --git a/hw/qdev.c b/hw/qdev.c > index b20b34d..c12e151 100644 > --- a/hw/qdev.c > +++ b/hw/qdev.c > @@ -155,7 +155,7 @@ int qdev_init(DeviceState *dev) > DeviceClass *dc = DEVICE_GET_CLASS(dev); > int rc; > > - assert(dev->state == DEV_STATE_CREATED); > + assert(!object_is_realized(OBJECT(dev))); > > rc = dc->init(dev); > if (rc< 0) { > @@ -178,7 +178,7 @@ int qdev_init(DeviceState *dev) > dev->instance_id_alias, > dev->alias_required_for_version); > } > - dev->state = DEV_STATE_INITIALIZED; > + OBJECT(dev)->state = OBJECT_STATE_REALIZED; > if (dev->hotplugged) { > device_reset(dev); > } > @@ -188,7 +188,7 @@ int qdev_init(DeviceState *dev) > void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id, > int required_for_version) > { > - assert(dev->state == DEV_STATE_CREATED); > + assert(!object_is_realized(OBJECT(dev))); > dev->instance_id_alias = alias_id; > dev->alias_required_for_version = required_for_version; > } > @@ -567,7 +567,7 @@ static void qdev_set_legacy_property(Object *obj, Visitor *v, void *opaque, > char *ptr = NULL; > int ret; > > - if (dev->state != DEV_STATE_CREATED) { > + if (object_is_realized(obj)) { > error_set(errp, QERR_PERMISSION_DENIED); > return; > } > @@ -674,7 +674,6 @@ static void device_initfn(Object *obj) > } > > dev->instance_id_alias = -1; > - dev->state = DEV_STATE_CREATED; > > class = object_get_class(OBJECT(dev)); > do { > @@ -697,7 +696,7 @@ static void device_finalize(Object *obj) > BusState *bus; > DeviceClass *dc = DEVICE_GET_CLASS(dev); > > - if (dev->state == DEV_STATE_INITIALIZED) { > + if (object_is_realized(obj)) { > while (dev->num_child_bus) { > bus = QLIST_FIRST(&dev->child_bus); > qbus_free(bus); > diff --git a/hw/qdev.h b/hw/qdev.h > index ae1d281..a1e40c5 100644 > --- a/hw/qdev.h > +++ b/hw/qdev.h > @@ -19,11 +19,6 @@ typedef struct BusState BusState; > > typedef struct BusClass BusClass; > > -enum DevState { > - DEV_STATE_CREATED = 1, > - DEV_STATE_INITIALIZED, > -}; > - > enum { > DEV_NVECTORS_UNSPECIFIED = -1, > }; > @@ -64,7 +59,6 @@ struct DeviceState { > Object parent_obj; > > const char *id; > - enum DevState state; > QemuOpts *opts; > int hotplugged; > BusState *parent_bus; > diff --git a/include/qemu/object.h b/include/qemu/object.h > index 8b17776..1606777 100644 > --- a/include/qemu/object.h > +++ b/include/qemu/object.h > @@ -241,6 +241,11 @@ struct ObjectClass > Type type; > }; > > +typedef enum ObjectState { > + OBJECT_STATE_INITIALIZED = 1, > + OBJECT_STATE_REALIZED, > +} ObjectState; I think using a bool would be better since it reduces the temptation to add additional states. Regards, Anthony Liguori
Il 08/06/2012 03:19, Anthony Liguori ha scritto: >> >> +typedef enum ObjectState { >> + OBJECT_STATE_INITIALIZED = 1, >> + OBJECT_STATE_REALIZED, >> +} ObjectState; > > I think using a bool would be better since it reduces the temptation to > add additional states. In fact someone already discussed having a third state for block devices... :) Paolo
On 06/10/2012 10:49 AM, Paolo Bonzini wrote: > Il 08/06/2012 03:19, Anthony Liguori ha scritto: >>> >>> +typedef enum ObjectState { >>> + OBJECT_STATE_INITIALIZED = 1, >>> + OBJECT_STATE_REALIZED, >>> +} ObjectState; >> >> I think using a bool would be better since it reduces the temptation to >> add additional states. > > In fact someone already discussed having a third state for block > devices... :) This is *exactly* why bool is better. Because too many people will be tempted to add stuff that doesn't belong in Object. A classic problem with OO design is bloating base classes--especially when you have one common base class. Everything in Object must be relevant to all subclasses, not just some. Having an OBJECT_STATE_OPENED only makes sense for BlockDevices. Therefore the concept of opened should live in the BlockDevice parent class. Regards, Anthony Liguori > > Paolo
Am 10.06.2012 17:49, schrieb Paolo Bonzini: > Il 08/06/2012 03:19, Anthony Liguori ha scritto: >>> >>> +typedef enum ObjectState { >>> + OBJECT_STATE_INITIALIZED = 1, >>> + OBJECT_STATE_REALIZED, >>> +} ObjectState; >> >> I think using a bool would be better since it reduces the temptation to >> add additional states. > > In fact someone already discussed having a third state for block > devices... :) I would expect that file_opened state to remain internal to the block layer. Thought we discussed that on IRC? Andreas
Am 10.06.2012 19:38, schrieb Andreas Färber: > Am 10.06.2012 17:49, schrieb Paolo Bonzini: >> Il 08/06/2012 03:19, Anthony Liguori ha scritto: >>>> >>>> +typedef enum ObjectState { >>>> + OBJECT_STATE_INITIALIZED = 1, >>>> + OBJECT_STATE_REALIZED, >>>> +} ObjectState; >>> >>> I think using a bool would be better since it reduces the temptation to >>> add additional states. >> >> In fact someone already discussed having a third state for block >> devices... :) > > I would expect that file_opened state to remain internal to the block > layer. Thought we discussed that on IRC? I think I still don't understand well enough what 'realized' is really supposed to mean. Does some magic happen when an object gets realised? From outside, what's the difference between an INITIALIZED and a REALIZED object? Is it more or less an implementation detail of the base class or is it something that affects the object model itself? I think the file_opened state should have more or less the same status as the realisation of an object. They are quite similar, so they should both be an implementation detail of their respective class, or they should both be baked into the object model. A comment in this patch says it means that an object is fully constructed. So if we add a file_opened state in the block layer, in order to keep this consistent, it would have to be a state that comes between INITIALIZED and REALIZED. Where would we have checks, for example whether the image that you assign to a block device is really opened? Would QOM commands to create a link only link to already realised objects? Would each single block device have to check this during its own realisation phase? Would we check object_is_realized() or rather bdrv_file_is_opened()? If the latter, it can't be generic monitor code that checks it. Let's start with this, I'm sure I'll have more questions... Kevin
On 06/11/2012 03:25 AM, Kevin Wolf wrote: > Am 10.06.2012 19:38, schrieb Andreas Färber: >> Am 10.06.2012 17:49, schrieb Paolo Bonzini: >>> Il 08/06/2012 03:19, Anthony Liguori ha scritto: >>>>> >>>>> +typedef enum ObjectState { >>>>> + OBJECT_STATE_INITIALIZED = 1, >>>>> + OBJECT_STATE_REALIZED, >>>>> +} ObjectState; >>>> >>>> I think using a bool would be better since it reduces the temptation to >>>> add additional states. >>> >>> In fact someone already discussed having a third state for block >>> devices... :) >> >> I would expect that file_opened state to remain internal to the block >> layer. Thought we discussed that on IRC? > > I think I still don't understand well enough what 'realized' is really > supposed to mean. realized is essentially the Vcc pin for the device. When realized = true, it means power has been applied to the device (and the guest potentially is interacting with it). When realized = false, it means that power is not applied to the device and the guest is not running. > > Does some magic happen when an object gets realised? From outside, > what's the difference between an INITIALIZED and a REALIZED object? Is > it more or less an implementation detail of the base class or is it > something that affects the object model itself? On the rising edge of realized, you typically will validate any parameters and do any actions necessary based on the parameters. As long as the guest isn't running, we want the ability to make changes to the devices that we normally couldn't change while the guest is running (like updating the CharDriverState pointer). > I think the file_opened state should have more or less the same status > as the realisation of an object. They are quite similar, so they should > both be an implementation detail of their respective class, or they > should both be baked into the object model. I think we need to think about what realized means to a non-Device. Does file_opened have the same logical semantics as the above? > > A comment in this patch says it means that an object is fully > constructed. That's not really the best wording. See above. Regards, Anthony Liguori > So if we add a file_opened state in the block layer, in > order to keep this consistent, it would have to be a state that comes > between INITIALIZED and REALIZED. > > Where would we have checks, for example whether the image that you > assign to a block device is really opened? Would QOM commands to create > a link only link to already realised objects? Would each single block > device have to check this during its own realisation phase? Would we > check object_is_realized() or rather bdrv_file_is_opened()? If the > latter, it can't be generic monitor code that checks it. > > Let's start with this, I'm sure I'll have more questions... > > Kevin
Am 11.06.2012 15:21, schrieb Anthony Liguori: > On 06/11/2012 03:25 AM, Kevin Wolf wrote: >> Am 10.06.2012 19:38, schrieb Andreas Färber: >>> Am 10.06.2012 17:49, schrieb Paolo Bonzini: >>>> Il 08/06/2012 03:19, Anthony Liguori ha scritto: >>>>>> >>>>>> +typedef enum ObjectState { >>>>>> + OBJECT_STATE_INITIALIZED = 1, >>>>>> + OBJECT_STATE_REALIZED, >>>>>> +} ObjectState; >>>>> >>>>> I think using a bool would be better since it reduces the temptation to >>>>> add additional states. >>>> >>>> In fact someone already discussed having a third state for block >>>> devices... :) >>> >>> I would expect that file_opened state to remain internal to the block >>> layer. Thought we discussed that on IRC? >> >> I think I still don't understand well enough what 'realized' is really >> supposed to mean. > > realized is essentially the Vcc pin for the device. My BlockDriverState doesn't have a Vcc pin, and neither has my Error object. I thought realize() did exist in Object and not only in Device? If so, it must have a more generic meaning. >> Does some magic happen when an object gets realised? From outside, >> what's the difference between an INITIALIZED and a REALIZED object? Is >> it more or less an implementation detail of the base class or is it >> something that affects the object model itself? > > On the rising edge of realized, you typically will validate any parameters and > do any actions necessary based on the parameters. As long as the guest isn't > running, we want the ability to make changes to the devices that we normally > couldn't change while the guest is running (like updating the CharDriverState > pointer). Okay, that matches my understanding. Are all of the checks and the status change of properties (modifiable -> const) done by the specific subclass or is it done by QOM to some degree? >> I think the file_opened state should have more or less the same status >> as the realisation of an object. They are quite similar, so they should >> both be an implementation detail of their respective class, or they >> should both be baked into the object model. > > I think we need to think about what realized means to a non-Device. Does > file_opened have the same logical semantics as the above? Yes, I think it's quite similar, just at an intermediate stage of the object creation. As I imagine it, you would: 1. Create a BlockDriveState 2. Modify the filename property (you can really modify any property if you want for some reason) 3. Call file_open, which opens the image file and may derive some default property values from the image file (flags in the header or whatever). Typical example: The backing file path. I think this would roughly correspond to an extended .bdrv_probe. 4. Modify more properties, overriding any defaults of the image. file_open has made some properties read-only (like the filename), which cannot be modified any more. 5. Call realize. This is the actual .bdrv_open call that makes the BlockDriverState ready to be used. This makes some more properties read-only, like the backing file and format. >> A comment in this patch says it means that an object is fully >> constructed. > > That's not really the best wording. See above. For non-Devices it's still the best explanation I have found. Kevin
Am 11.06.2012 15:21, schrieb Anthony Liguori: > On 06/11/2012 03:25 AM, Kevin Wolf wrote: >> Am 10.06.2012 19:38, schrieb Andreas Färber: >>> Am 10.06.2012 17:49, schrieb Paolo Bonzini: >>>> Il 08/06/2012 03:19, Anthony Liguori ha scritto: >>>>>> >>>>>> +typedef enum ObjectState { >>>>>> + OBJECT_STATE_INITIALIZED = 1, >>>>>> + OBJECT_STATE_REALIZED, >>>>>> +} ObjectState; >>>>> >>>>> I think using a bool would be better since it reduces the >>>>> temptation to >>>>> add additional states. >>>> >>>> In fact someone already discussed having a third state for block >>>> devices... :) >>> >>> I would expect that file_opened state to remain internal to the block >>> layer. Thought we discussed that on IRC? >> >> I think I still don't understand well enough what 'realized' is really >> supposed to mean. > > realized is essentially the Vcc pin for the device. > > When realized = true, it means power has been applied to the device (and > the guest potentially is interacting with it). > > When realized = false, it means that power is not applied to the device > and the guest is not running. That does not match my understanding of realize. To me, realize is the second-stage (final) initialization of an object. It's purpose is to set up an object based on properties set after its initialization, so that it can be fully used. Contrary to the initialization phase, where failure would lead to inability to run finalizers, realization can fail and leaves the object in a defined state so that it can either be realized again with changed properties or deleted, running any finalizers. The main difference to qdev init is that we are working towards replacing the init-after-create pattern with late, central realization so that users have a chance to modify objects through QMP once initialized. (Which is what the don't-create-objects-in-realize and in-place initialization discussion is about.) Thus I do not think this has anything to do with devices or power. A device within a SoC or Super I/O chip that is turned off / powered down may still be there wrt MemoryRegions. It would be possible though to amend realize functionality by overriding realize for DeviceState or specific devices. For block devices I thought the discussion had been that they would get a block-specific open(Error**) method, called after initialization and setting the file name / URL, setting some bool opened state. Some block properties would then depend on "opened" rather than on object_is_realized() and fail otherwise. Realize would still be the final construction of the object based on user-settable properties. A variation of this patch here would be to introduce bool realized while leaving the qdev state untouched. But that would be in the way of generalizing static properties to Object, which would mean for the block layer that any trivial property would need to be implemented through manually coded getters and setters. Regards, Andreas
Am 11.06.2012 23:31, schrieb Andreas Färber: > Am 11.06.2012 15:21, schrieb Anthony Liguori: >> On 06/11/2012 03:25 AM, Kevin Wolf wrote: >>> Am 10.06.2012 19:38, schrieb Andreas Färber: >>>> Am 10.06.2012 17:49, schrieb Paolo Bonzini: >>>>> Il 08/06/2012 03:19, Anthony Liguori ha scritto: >>>>>>> >>>>>>> +typedef enum ObjectState { >>>>>>> + OBJECT_STATE_INITIALIZED = 1, >>>>>>> + OBJECT_STATE_REALIZED, >>>>>>> +} ObjectState; >>>>>> >>>>>> I think using a bool would be better since it reduces the >>>>>> temptation to >>>>>> add additional states. >>>>> >>>>> In fact someone already discussed having a third state for block >>>>> devices... :) >>>> >>>> I would expect that file_opened state to remain internal to the block >>>> layer. Thought we discussed that on IRC? >>> >>> I think I still don't understand well enough what 'realized' is really >>> supposed to mean. >> >> realized is essentially the Vcc pin for the device. >> >> When realized = true, it means power has been applied to the device (and >> the guest potentially is interacting with it). >> >> When realized = false, it means that power is not applied to the device >> and the guest is not running. > > That does not match my understanding of realize. > > To me, realize is the second-stage (final) initialization of an object. > It's purpose is to set up an object based on properties set after its > initialization, so that it can be fully used. > Contrary to the initialization phase, where failure would lead to > inability to run finalizers, realization can fail and leaves the object > in a defined state so that it can either be realized again with changed > properties or deleted, running any finalizers. > > The main difference to qdev init is that we are working towards > replacing the init-after-create pattern with late, central realization > so that users have a chance to modify objects through QMP once > initialized. (Which is what the don't-create-objects-in-realize and > in-place initialization discussion is about.) This is the part that I think can be interpreted as "machine is powered on" or Vcc, but there's nothing stopping us from calling realize at a slightly different point in time for non-device objects not connected to /machine. An SD card for instance could be realized before it is inserted into the drive and thereby before it gets any Vcc. /-F > Thus I do not think this has anything to do with devices or power. A > device within a SoC or Super I/O chip that is turned off / powered down > may still be there wrt MemoryRegions. It would be possible though to > amend realize functionality by overriding realize for DeviceState or > specific devices. > > For block devices I thought the discussion had been that they would get > a block-specific open(Error**) method, called after initialization and > setting the file name / URL, setting some bool opened state. Some block > properties would then depend on "opened" rather than on > object_is_realized() and fail otherwise. Realize would still be the > final construction of the object based on user-settable properties. > > A variation of this patch here would be to introduce bool realized while > leaving the qdev state untouched. But that would be in the way of > generalizing static properties to Object, which would mean for the block > layer that any trivial property would need to be implemented through > manually coded getters and setters. > > Regards, > Andreas
On 06/11/2012 04:31 PM, Andreas Färber wrote: > Am 11.06.2012 15:21, schrieb Anthony Liguori: >> On 06/11/2012 03:25 AM, Kevin Wolf wrote: >>> Am 10.06.2012 19:38, schrieb Andreas Färber: >>>> Am 10.06.2012 17:49, schrieb Paolo Bonzini: >>>>> Il 08/06/2012 03:19, Anthony Liguori ha scritto: >>>>>>> >>>>>>> +typedef enum ObjectState { >>>>>>> + OBJECT_STATE_INITIALIZED = 1, >>>>>>> + OBJECT_STATE_REALIZED, >>>>>>> +} ObjectState; >>>>>> >>>>>> I think using a bool would be better since it reduces the >>>>>> temptation to >>>>>> add additional states. >>>>> >>>>> In fact someone already discussed having a third state for block >>>>> devices... :) >>>> >>>> I would expect that file_opened state to remain internal to the block >>>> layer. Thought we discussed that on IRC? >>> >>> I think I still don't understand well enough what 'realized' is really >>> supposed to mean. >> >> realized is essentially the Vcc pin for the device. >> >> When realized = true, it means power has been applied to the device (and >> the guest potentially is interacting with it). >> >> When realized = false, it means that power is not applied to the device >> and the guest is not running. > > That does not match my understanding of realize. > > To me, realize is the second-stage (final) initialization of an object. > It's purpose is to set up an object based on properties set after its > initialization, so that it can be fully used. > Contrary to the initialization phase, where failure would lead to > inability to run finalizers, realization can fail and leaves the object > in a defined state so that it can either be realized again with changed > properties or deleted, running any finalizers. This is not the intention of realize. There's nothing final about realize either. Realize attempts to give you a hook at the very last moment before you can no longer change a device. In X11, realize happens before a window is mapped to the visible screen. X11 has the same problem where there's a relationship between widgets and there's some work that requires that the parent widget is known. realize() provides that last possible hook where you can defer the work that requires the parent and children to be setup. That's what realize achieves. And unrealize is just the opposite. It's called as soon as you unmap a window and can be used to undo the things you did in realize. Regards, Anthony Liguori > > The main difference to qdev init is that we are working towards > replacing the init-after-create pattern with late, central realization > so that users have a chance to modify objects through QMP once > initialized. (Which is what the don't-create-objects-in-realize and > in-place initialization discussion is about.) > > Thus I do not think this has anything to do with devices or power. A > device within a SoC or Super I/O chip that is turned off / powered down > may still be there wrt MemoryRegions. It would be possible though to > amend realize functionality by overriding realize for DeviceState or > specific devices. > > For block devices I thought the discussion had been that they would get > a block-specific open(Error**) method, called after initialization and > setting the file name / URL, setting some bool opened state. Some block > properties would then depend on "opened" rather than on > object_is_realized() and fail otherwise. Realize would still be the > final construction of the object based on user-settable properties. > > A variation of this patch here would be to introduce bool realized while > leaving the qdev state untouched. But that would be in the way of > generalizing static properties to Object, which would mean for the block > layer that any trivial property would need to be implemented through > manually coded getters and setters. > > Regards, > Andreas >
Am 11.06.2012 23:48, schrieb Anthony Liguori: > On 06/11/2012 04:31 PM, Andreas Färber wrote: >> Am 11.06.2012 15:21, schrieb Anthony Liguori: >>> On 06/11/2012 03:25 AM, Kevin Wolf wrote: >>>> Am 10.06.2012 19:38, schrieb Andreas Färber: >>>>> Am 10.06.2012 17:49, schrieb Paolo Bonzini: >>>>>> Il 08/06/2012 03:19, Anthony Liguori ha scritto: >>>>>>>> >>>>>>>> +typedef enum ObjectState { >>>>>>>> + OBJECT_STATE_INITIALIZED = 1, >>>>>>>> + OBJECT_STATE_REALIZED, >>>>>>>> +} ObjectState; >>>>>>> >>>>>>> I think using a bool would be better since it reduces the >>>>>>> temptation to >>>>>>> add additional states. >>>>>> >>>>>> In fact someone already discussed having a third state for block >>>>>> devices... :) >>>>> >>>>> I would expect that file_opened state to remain internal to the block >>>>> layer. Thought we discussed that on IRC? >>>> >>>> I think I still don't understand well enough what 'realized' is really >>>> supposed to mean. >>> >>> realized is essentially the Vcc pin for the device. >>> >>> When realized = true, it means power has been applied to the device (and >>> the guest potentially is interacting with it). >>> >>> When realized = false, it means that power is not applied to the device >>> and the guest is not running. >> >> That does not match my understanding of realize. >> >> To me, realize is the second-stage (final) initialization of an object. >> It's purpose is to set up an object based on properties set after its >> initialization, so that it can be fully used. >> Contrary to the initialization phase, where failure would lead to >> inability to run finalizers, realization can fail and leaves the object >> in a defined state so that it can either be realized again with changed >> properties or deleted, running any finalizers. > > This is not the intention of realize. There's nothing final about > realize either. Final in the sense of second-stage of two stages: No further construction happens after realize because realize is being deferred to the latest point possible. > Realize attempts to give you a hook at the very last moment before you > can no longer change a device. In X11, realize happens before a window > is mapped to the visible screen. X11 has the same problem where there's > a relationship between widgets and there's some work that requires that > the parent widget is known. realize() provides that last possible hook > where you can defer the work that requires the parent and children to be > setup. > > That's what realize achieves. And unrealize is just the opposite. It's > called as soon as you unmap a window and can be used to undo the things > you did in realize. We don't seem to be in disagreement except that you say it's not the intention of realize... Hook, function or constructor ... it basically does the same thing. If you want to have differently named hooks doing roughly the same thing in different contexts, then what is your suggestion? The qdev init model doesn't fully translate to realize yet, so add DEVICE_STATE_REALIZED to the qdev state enum as third state? Andreas > > Regards, > > Anthony Liguori > >> >> The main difference to qdev init is that we are working towards >> replacing the init-after-create pattern with late, central realization >> so that users have a chance to modify objects through QMP once >> initialized. (Which is what the don't-create-objects-in-realize and >> in-place initialization discussion is about.) >> >> Thus I do not think this has anything to do with devices or power. A >> device within a SoC or Super I/O chip that is turned off / powered down >> may still be there wrt MemoryRegions. It would be possible though to >> amend realize functionality by overriding realize for DeviceState or >> specific devices. >> >> For block devices I thought the discussion had been that they would get >> a block-specific open(Error**) method, called after initialization and >> setting the file name / URL, setting some bool opened state. Some block >> properties would then depend on "opened" rather than on >> object_is_realized() and fail otherwise. Realize would still be the >> final construction of the object based on user-settable properties. >> >> A variation of this patch here would be to introduce bool realized while >> leaving the qdev state untouched. But that would be in the way of >> generalizing static properties to Object, which would mean for the block >> layer that any trivial property would need to be implemented through >> manually coded getters and setters. >> >> Regards, >> Andreas
diff --git a/hw/qdev-addr.c b/hw/qdev-addr.c index b711b6b..a3796bd 100644 --- a/hw/qdev-addr.c +++ b/hw/qdev-addr.c @@ -1,3 +1,4 @@ +#include "qemu/object.h" #include "qdev.h" #include "qdev-addr.h" #include "targphys.h" @@ -39,7 +40,7 @@ static void set_taddr(Object *obj, Visitor *v, void *opaque, Error *local_err = NULL; int64_t value; - if (dev->state != DEV_STATE_CREATED) { + if (object_is_realized(obj)) { error_set(errp, QERR_PERMISSION_DENIED); return; } diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c index 099a7aa..fcc0bed 100644 --- a/hw/qdev-properties.c +++ b/hw/qdev-properties.c @@ -53,7 +53,7 @@ static void set_bit(Object *obj, Visitor *v, void *opaque, Error *local_err = NULL; bool value; - if (dev->state != DEV_STATE_CREATED) { + if (object_is_realized(obj)) { error_set(errp, QERR_PERMISSION_DENIED); return; } @@ -93,7 +93,7 @@ static void set_uint8(Object *obj, Visitor *v, void *opaque, Property *prop = opaque; uint8_t *ptr = qdev_get_prop_ptr(dev, prop); - if (dev->state != DEV_STATE_CREATED) { + if (object_is_realized(obj)) { error_set(errp, QERR_PERMISSION_DENIED); return; } @@ -160,7 +160,7 @@ static void set_uint16(Object *obj, Visitor *v, void *opaque, Property *prop = opaque; uint16_t *ptr = qdev_get_prop_ptr(dev, prop); - if (dev->state != DEV_STATE_CREATED) { + if (object_is_realized(obj)) { error_set(errp, QERR_PERMISSION_DENIED); return; } @@ -193,7 +193,7 @@ static void set_uint32(Object *obj, Visitor *v, void *opaque, Property *prop = opaque; uint32_t *ptr = qdev_get_prop_ptr(dev, prop); - if (dev->state != DEV_STATE_CREATED) { + if (object_is_realized(obj)) { error_set(errp, QERR_PERMISSION_DENIED); return; } @@ -218,7 +218,7 @@ static void set_int32(Object *obj, Visitor *v, void *opaque, Property *prop = opaque; int32_t *ptr = qdev_get_prop_ptr(dev, prop); - if (dev->state != DEV_STATE_CREATED) { + if (object_is_realized(obj)) { error_set(errp, QERR_PERMISSION_DENIED); return; } @@ -291,7 +291,7 @@ static void set_uint64(Object *obj, Visitor *v, void *opaque, Property *prop = opaque; uint64_t *ptr = qdev_get_prop_ptr(dev, prop); - if (dev->state != DEV_STATE_CREATED) { + if (object_is_realized(obj)) { error_set(errp, QERR_PERMISSION_DENIED); return; } @@ -379,7 +379,7 @@ static void set_string(Object *obj, Visitor *v, void *opaque, Error *local_err = NULL; char *str; - if (dev->state != DEV_STATE_CREATED) { + if (object_is_realized(obj)) { error_set(errp, QERR_PERMISSION_DENIED); return; } @@ -457,7 +457,7 @@ static void set_pointer(Object *obj, Visitor *v, Property *prop, char *str; int ret; - if (dev->state != DEV_STATE_CREATED) { + if (object_is_realized(obj)) { error_set(errp, QERR_PERMISSION_DENIED); return; } @@ -626,7 +626,7 @@ static void set_vlan(Object *obj, Visitor *v, void *opaque, int64_t id; VLANState *vlan; - if (dev->state != DEV_STATE_CREATED) { + if (object_is_realized(obj)) { error_set(errp, QERR_PERMISSION_DENIED); return; } @@ -696,7 +696,7 @@ static void set_mac(Object *obj, Visitor *v, void *opaque, int i, pos; char *str, *p; - if (dev->state != DEV_STATE_CREATED) { + if (object_is_realized(obj)) { error_set(errp, QERR_PERMISSION_DENIED); return; } @@ -766,7 +766,7 @@ static void set_enum(Object *obj, Visitor *v, void *opaque, Property *prop = opaque; int *ptr = qdev_get_prop_ptr(dev, prop); - if (dev->state != DEV_STATE_CREATED) { + if (object_is_realized(obj)) { error_set(errp, QERR_PERMISSION_DENIED); return; } @@ -797,7 +797,7 @@ static void set_pci_devfn(Object *obj, Visitor *v, void *opaque, Error *local_err = NULL; char *str; - if (dev->state != DEV_STATE_CREATED) { + if (object_is_realized(obj)) { error_set(errp, QERR_PERMISSION_DENIED); return; } @@ -867,7 +867,7 @@ static void set_blocksize(Object *obj, Visitor *v, void *opaque, const int64_t min = 512; const int64_t max = 32768; - if (dev->state != DEV_STATE_CREATED) { + if (object_is_realized(obj)) { error_set(errp, QERR_PERMISSION_DENIED); return; } diff --git a/hw/qdev.c b/hw/qdev.c index b20b34d..c12e151 100644 --- a/hw/qdev.c +++ b/hw/qdev.c @@ -155,7 +155,7 @@ int qdev_init(DeviceState *dev) DeviceClass *dc = DEVICE_GET_CLASS(dev); int rc; - assert(dev->state == DEV_STATE_CREATED); + assert(!object_is_realized(OBJECT(dev))); rc = dc->init(dev); if (rc < 0) { @@ -178,7 +178,7 @@ int qdev_init(DeviceState *dev) dev->instance_id_alias, dev->alias_required_for_version); } - dev->state = DEV_STATE_INITIALIZED; + OBJECT(dev)->state = OBJECT_STATE_REALIZED; if (dev->hotplugged) { device_reset(dev); } @@ -188,7 +188,7 @@ int qdev_init(DeviceState *dev) void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id, int required_for_version) { - assert(dev->state == DEV_STATE_CREATED); + assert(!object_is_realized(OBJECT(dev))); dev->instance_id_alias = alias_id; dev->alias_required_for_version = required_for_version; } @@ -567,7 +567,7 @@ static void qdev_set_legacy_property(Object *obj, Visitor *v, void *opaque, char *ptr = NULL; int ret; - if (dev->state != DEV_STATE_CREATED) { + if (object_is_realized(obj)) { error_set(errp, QERR_PERMISSION_DENIED); return; } @@ -674,7 +674,6 @@ static void device_initfn(Object *obj) } dev->instance_id_alias = -1; - dev->state = DEV_STATE_CREATED; class = object_get_class(OBJECT(dev)); do { @@ -697,7 +696,7 @@ static void device_finalize(Object *obj) BusState *bus; DeviceClass *dc = DEVICE_GET_CLASS(dev); - if (dev->state == DEV_STATE_INITIALIZED) { + if (object_is_realized(obj)) { while (dev->num_child_bus) { bus = QLIST_FIRST(&dev->child_bus); qbus_free(bus); diff --git a/hw/qdev.h b/hw/qdev.h index ae1d281..a1e40c5 100644 --- a/hw/qdev.h +++ b/hw/qdev.h @@ -19,11 +19,6 @@ typedef struct BusState BusState; typedef struct BusClass BusClass; -enum DevState { - DEV_STATE_CREATED = 1, - DEV_STATE_INITIALIZED, -}; - enum { DEV_NVECTORS_UNSPECIFIED = -1, }; @@ -64,7 +59,6 @@ struct DeviceState { Object parent_obj; const char *id; - enum DevState state; QemuOpts *opts; int hotplugged; BusState *parent_bus; diff --git a/include/qemu/object.h b/include/qemu/object.h index 8b17776..1606777 100644 --- a/include/qemu/object.h +++ b/include/qemu/object.h @@ -241,6 +241,11 @@ struct ObjectClass Type type; }; +typedef enum ObjectState { + OBJECT_STATE_INITIALIZED = 1, + OBJECT_STATE_REALIZED, +} ObjectState; + /** * Object: * @@ -264,6 +269,7 @@ struct Object QTAILQ_HEAD(, ObjectProperty) properties; uint32_t ref; Object *parent; + ObjectState state; }; /** @@ -812,6 +818,14 @@ const char *object_property_get_type(Object *obj, const char *name, Object *object_get_root(void); /** + * object_is_realized: + * @obj: the object + * + * Returns whether @obj has been realized (i.e. completely constructed). + */ +bool object_is_realized(Object *obj); + +/** * object_get_canonical_path: * * Returns: The canonical path for a object. This is the path within the diff --git a/qom/object.c b/qom/object.c index 00bb3b0..93e0499 100644 --- a/qom/object.c +++ b/qom/object.c @@ -280,6 +280,8 @@ static void object_init_with_type(Object *obj, TypeImpl *ti) if (ti->instance_init) { ti->instance_init(obj); } + + obj->state = OBJECT_STATE_INITIALIZED; } void object_initialize_with_type(void *data, TypeImpl *type) @@ -1237,6 +1239,11 @@ static char *qdev_get_type(Object *obj, Error **errp) return g_strdup(object_get_typename(obj)); } +bool object_is_realized(Object *obj) +{ + return obj->state == OBJECT_STATE_REALIZED; +} + static void object_instance_init(Object *obj) { object_property_add_str(obj, "type", qdev_get_type, NULL, NULL);