| Message ID | 1316188834-13675-5-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 09/16/11 18:00, Anthony Liguori wrote: > qdev has this quirk that it owns a seemingly arbitrary QemuOpts pointer. That's > because qdev expects a static string for the id (which really makes no sense > since ids are supposed to be provided by the user). Instead of managing just > the id pointer, we currently take ownership of the entire QemuOpts structure > that was used to create the device just to keep the name around. FYI: Keeping the pointer to the QemuOpts has one more reason: It will free the QemuOpts on hot-unplug, which is needed to free the id from QemuOpts point of view, which in turn allows to re-use the id when hot-plugging the same (or another) device later on. cheers, Gerd
On 09/19/2011 02:34 AM, Gerd Hoffmann wrote: > On 09/16/11 18:00, Anthony Liguori wrote: >> qdev has this quirk that it owns a seemingly arbitrary QemuOpts pointer. That's >> because qdev expects a static string for the id (which really makes no sense >> since ids are supposed to be provided by the user). Instead of managing just >> the id pointer, we currently take ownership of the entire QemuOpts structure >> that was used to create the device just to keep the name around. > > FYI: Keeping the pointer to the QemuOpts has one more reason: It will free the > QemuOpts on hot-unplug, which is needed to free the id from QemuOpts point of > view, which in turn allows to re-use the id when hot-plugging the same (or > another) device later on. You mean, tie QemuOpts life cycle to devices life cycle such that you cannot accidentally create a non-device QemuOpts that conflicts with the id of a device? Regard, Anthony Liguori > > cheers, > Gerd > >
On 09/19/11 18:27, Anthony Liguori wrote: > On 09/19/2011 02:34 AM, Gerd Hoffmann wrote: >> FYI: Keeping the pointer to the QemuOpts has one more reason: It will >> free the >> QemuOpts on hot-unplug, which is needed to free the id from QemuOpts >> point of >> view, which in turn allows to re-use the id when hot-plugging the same >> (or >> another) device later on. > > You mean, tie QemuOpts life cycle to devices life cycle Yes. > such that you > cannot accidentally create a non-device QemuOpts that conflicts with the > id of a device? Device QemuOpts have their own id namespace, so this is just about conflicts within devices. This ... device_add e1000,id=nic1 device_del nic1 device_add e1000,id=nic1 ... will work only if you free the QemuOpts when deleting a device, otherwise QemuOpts will complain that nic1 is used already. cheers, Gerd
On 09/20/2011 01:36 AM, Gerd Hoffmann wrote: > On 09/19/11 18:27, Anthony Liguori wrote: >> On 09/19/2011 02:34 AM, Gerd Hoffmann wrote: >>> FYI: Keeping the pointer to the QemuOpts has one more reason: It will >>> free the >>> QemuOpts on hot-unplug, which is needed to free the id from QemuOpts >>> point of >>> view, which in turn allows to re-use the id when hot-plugging the same >>> (or >>> another) device later on. >> >> You mean, tie QemuOpts life cycle to devices life cycle > > Yes. > >> such that you >> cannot accidentally create a non-device QemuOpts that conflicts with the >> id of a device? > > Device QemuOpts have their own id namespace, so this is just about conflicts > within devices. This ... > > device_add e1000,id=nic1 > device_del nic1 > device_add e1000,id=nic1 > > ... will work only if you free the QemuOpts when deleting a device, otherwise > QemuOpts will complain that nic1 is used already. But we can just verify that the id specified for qdev is unique at creation time and fail creation if it isn't, no? Since not all devices are assigned names via qemuopts, that seems like a safer approach anyway. Regards, Anthony Liguori > > cheers, > Gerd
On 09/20/11 15:04, Anthony Liguori wrote: > On 09/20/2011 01:36 AM, Gerd Hoffmann wrote: >> On 09/19/11 18:27, Anthony Liguori wrote: >>> On 09/19/2011 02:34 AM, Gerd Hoffmann wrote: >>>> FYI: Keeping the pointer to the QemuOpts has one more reason: It will >>>> free the >>>> QemuOpts on hot-unplug, which is needed to free the id from QemuOpts >>>> point of >>>> view, which in turn allows to re-use the id when hot-plugging the same >>>> (or >>>> another) device later on. >>> >>> You mean, tie QemuOpts life cycle to devices life cycle >> >> Yes. >> >>> such that you >>> cannot accidentally create a non-device QemuOpts that conflicts with the >>> id of a device? >> >> Device QemuOpts have their own id namespace, so this is just about >> conflicts >> within devices. This ... >> >> device_add e1000,id=nic1 >> device_del nic1 >> device_add e1000,id=nic1 >> >> ... will work only if you free the QemuOpts when deleting a device, >> otherwise >> QemuOpts will complain that nic1 is used already. > > But we can just verify that the id specified for qdev is unique at > creation time and fail creation if it isn't, no? > > Since not all devices are assigned names via qemuopts, that seems like a > safer approach anyway. I think that happens anyway (didn't check the source though). Problem is that QemuOpts wants IDs being unique too, so keep the QemuOpts hanging around instead of releasing them makes QemuOpts complain about nic1 not being unique although there isn't such a device in qdev space. Oh, and not releasing the QemuOpts would also leak memory on each hot-unplug. cheers, Gerd
On 09/20/2011 08:21 AM, Gerd Hoffmann wrote: > On 09/20/11 15:04, Anthony Liguori wrote: >> On 09/20/2011 01:36 AM, Gerd Hoffmann wrote: >>> On 09/19/11 18:27, Anthony Liguori wrote: >>>> On 09/19/2011 02:34 AM, Gerd Hoffmann wrote: >>>>> FYI: Keeping the pointer to the QemuOpts has one more reason: It will >>>>> free the >>>>> QemuOpts on hot-unplug, which is needed to free the id from QemuOpts >>>>> point of >>>>> view, which in turn allows to re-use the id when hot-plugging the same >>>>> (or >>>>> another) device later on. >>>> >>>> You mean, tie QemuOpts life cycle to devices life cycle >>> >>> Yes. >>> >>>> such that you >>>> cannot accidentally create a non-device QemuOpts that conflicts with the >>>> id of a device? >>> >>> Device QemuOpts have their own id namespace, so this is just about >>> conflicts >>> within devices. This ... >>> >>> device_add e1000,id=nic1 >>> device_del nic1 >>> device_add e1000,id=nic1 >>> >>> ... will work only if you free the QemuOpts when deleting a device, >>> otherwise >>> QemuOpts will complain that nic1 is used already. >> >> But we can just verify that the id specified for qdev is unique at >> creation time and fail creation if it isn't, no? >> >> Since not all devices are assigned names via qemuopts, that seems like a >> safer approach anyway. > > I think that happens anyway (didn't check the source though). > > Problem is that QemuOpts wants IDs being unique too, so keep the QemuOpts > hanging around instead of releasing them makes QemuOpts complain about nic1 not > being unique although there isn't such a device in qdev space. I don't think we have a firm requirement that the QemuOpts namespace == device namespace. At any rate, it's not enforced today because devices can be created (with an id) outside of device_add. > Oh, and not > releasing the QemuOpts would also leak memory on each hot-unplug. If you look at my patch, opts is freed at the end of device_add so there is no leak. Regards, Anthony Liguori > > cheers, > Gerd > > >
Hi, >> Oh, and not >> releasing the QemuOpts would also leak memory on each hot-unplug. > > If you look at my patch, opts is freed at the end of device_add so there > is no leak. Ah, ok. Missed that. cheers, Gerd
diff --git a/hw/qdev.c b/hw/qdev.c index aeebdb9..41ed872 100644 --- a/hw/qdev.c +++ b/hw/qdev.c @@ -107,7 +107,7 @@ static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info, const } dev->instance_id_alias = -1; dev->state = DEV_STATE_CREATED; - dev->id = id; + dev->id = g_strdup(id); return dev; } @@ -414,6 +414,7 @@ void qdev_free(DeviceState *dev) qemu_opts_del(dev->opts); } QLIST_REMOVE(dev, sibling); + g_free(dev->id); for (prop = dev->info->props; prop && prop->name; prop++) { if (prop->info->free) { prop->info->free(dev, prop); diff --git a/hw/qdev.h b/hw/qdev.h index 626a1b6..c86736a 100644 --- a/hw/qdev.h +++ b/hw/qdev.h @@ -30,7 +30,7 @@ enum { /* This structure should not be accessed directly. We declare it here so that it can be embedded in individual device state structures. */ struct DeviceState { - const char *id; + char *id; enum DevState state; QemuOpts *opts; int hotplugged;
qdev has this quirk that it owns a seemingly arbitrary QemuOpts pointer. That's because qdev expects a static string for the id (which really makes no sense since ids are supposed to be provided by the user). Instead of managing just the id pointer, we currently take ownership of the entire QemuOpts structure that was used to create the device just to keep the name around. Just strdup the pointer we actually need. Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> --- hw/qdev.c | 3 ++- hw/qdev.h | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-)