Hi, > From: Eduardo Habkost [mailto:ehabk...@redhat.com] > > > > Subject: Re: [Qemu-devel] [PATCH v7 24/28] ide: add bootindex to qom > > > property > > > > > > > > On Fri, Sep 05, 2014 at 04:37:32PM +0800, arei.gong...@huawei.com > wrote: > > > > > From: Gonglei <arei.gong...@huawei.com> > > > > > > > > > > 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 <arei.gong...@huawei.com> > > > > > --- > > > > > 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 ? "/disk@1" : > > > "/disk@0", > > > > > + &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 ? "/disk@1" : "/disk@0"; > } > Thank you so much!
I agree more with #1, because only IDE device has this special case. This is not worth add a property to DeviceState struct for a special device IMO. Best regards, -Gonglei
