Re: [Qemu-devel] [PATCH v7 24/28] ide: add bootindex to qom property

[Eduardo Habkost]

On Tue, Sep 09, 2014 at 07:51:49AM +0000, Gonglei (Arei) wrote:
> Hi,
> 
> > Subject: RE: [Qemu-devel] [PATCH v7 24/28] ide: add bootindex to qom 
> > property
> > 
> > > Subject: Re: [Qemu-devel] [PATCH v7 24/28] ide: add bootindex to qom
> > property
> > >
> > > On Fri, Sep 05, 2014 at 04:37:32PM +0800, address@hidden wrote:
> > > > From: Gonglei <address@hidden>
> > > >
> > > > Add a qom property with the same name 'bootindex',
> > > > when we remove it form qdev property, things will
> > > > continue to work just fine, and we can use qom features
> > > > which are not supported by qdev property.
> > > >
> > > > Signed-off-by: Gonglei <address@hidden>
> > > > ---
> > > >  hw/ide/qdev.c | 14 ++++++++++++++
> > > >  1 file changed, 14 insertions(+)
> > > >
> > > > diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> > > > index efab95b..9e2ed40 100644
> > > > --- a/hw/ide/qdev.c
> > > > +++ b/hw/ide/qdev.c
> > > > @@ -191,6 +191,17 @@ static int ide_dev_initfn(IDEDevice *dev,
> > > IDEDriveKind kind)
> > > >      return 0;
> > > >  }
> > > >
> > > > +static void ide_dev_instance_init(Object *obj)
> > > > +{
> > > > +    DeviceState *dev = DEVICE(obj);
> > > > +    IDEDevice *d = DO_UPCAST(IDEDevice, qdev, dev);
> > > > +
> > > > +    device_add_bootindex_property(obj, &d->conf.bootindex,
> > > > +                                  "bootindex",
> > > > +                                  d->unit ? "/address@hidden" :
> > "/address@hidden",
> > > > +                                  &d->qdev, NULL);
> > > > +}
> > > > +
> Oops, I found a thorny issue that the d->unit parameter had not been 
> initialized
> in ide_dev_instance_init(). d->unit maybe is a random value, which will 
> against
> the original purpose in this situation. 
> 
> What's your opinion, Eduardo? Thanks!

It looks like you can call add_boot_device_path() only on realize, on
IDE. If IDE is the only case, we could just change IDE to not use
device_add_bootindex_property(), having its own getter/setter and a call
to add_boot_device_path() on realize().

Or, to keep the code generic, we could just make get_boot_devices_list()
query the device for the suffix, instead of requiring the suffix to be
set before device_add_bootindex_property() is called. With something
like:

typedef struct BootDeviceInfo {
    int bootindex;
    const char *suffix;
} BootDeviceInfo;

struct FWBootEntry {
    QTAILQ_ENTRY(FWBootEntry) link;
    DeviceState *dev;
    BootDeviceInfo *bootdev;
};

void add_boot_device_path(DeviceState *dev, BootDeviceInfo *info);
void del_boot_device_path(BootDeviceInfo *info);
void device_add_bootindex_property(Object *obj, const char *property,
                                   DeviceState *dev, BootDeviceInfo *info,
                                   Error **errp);

This way, the suffix can be set between device_add_bootindex_property()
and get_boot_devices_list() if necessary:

static void somedevice_instance_init(Object *obj)
{
    SomeDevice *somedev = SOME_DEVICE(obj);
    device_add_bootindex_property(obj, DEVICE(obj), &somedev->bootinfo, 
&error_abort);
}

static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
{
    [...]
    dev->bootinfo.suffix = dev->unit ? "/address@hidden" : "/address@hidden";
}

-- 
Eduardo