| Message ID | 1442405768-23019-2-git-send-email-yanghy@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
This has finally reached the front of my review queue. I apologize for the loooong delay. Copying Paolo for another pair of eyeballs (he wrote this code). Yang Hongyang <yanghy@cn.fujitsu.com> writes: > When delete an object, we need to delete the associated qemu opts, > otherwise, we can not add another object with the same name using > object_add. > The case happens when we start qemu with: > -object xxx,id=aa > then we delete this object with: > object_del aa > then add the object with: > object_add xxx,id=aa > > QEMU will return error with Duplicate ID error. > > Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com> > --- > qmp.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/qmp.c b/qmp.c > index 9623c80..4bd44c3 100644 > --- a/qmp.c > +++ b/qmp.c > @@ -686,6 +686,7 @@ void qmp_object_del(const char *id, Error **errp) > { > Object *container; > Object *obj; > + QemuOpts *opts; > > container = object_get_objects_root(); > obj = object_resolve_path_component(container, id); > @@ -699,6 +700,9 @@ void qmp_object_del(const char *id, Error **errp) > return; > } > object_unparent(obj); > + > + opts = qemu_opts_find(qemu_find_opts_err("object", NULL), id); > + qemu_opts_del(opts); qemu_find_opts_err("object", &error_abort) please, because when it fails, we want to die right away, not when the null pointer it returns gets dereferenced. Same sloppiness in netdev_del_completion() and qmp_netdev_del(), not your patch's fault. Elsewhere, we store the QemuOpts in the object just so we can delete it: DeviceState, DriveInfo. Paolo, what do you think? > } > > MemoryDeviceInfoList *qmp_query_memory_devices(Error **errp)
On 09/24/2015 03:43 PM, Markus Armbruster wrote: > This has finally reached the front of my review queue. I apologize for > the loooong delay. > > Copying Paolo for another pair of eyeballs (he wrote this code). > [...] >> + >> + opts = qemu_opts_find(qemu_find_opts_err("object", NULL), id); >> + qemu_opts_del(opts); > > qemu_find_opts_err("object", &error_abort) please, because when it > fails, we want to die right away, not when the null pointer it returns > gets dereferenced. Thanks for the review. Jason, do you want me to propose a fix on top of this series or simply drop this for now because this patch is an independent bug fix and won't affect the other filter patch series. > > Same sloppiness in netdev_del_completion() and qmp_netdev_del(), not > your patch's fault. > > Elsewhere, we store the QemuOpts in the object just so we can delete it: > DeviceState, DriveInfo. Paolo, what do you think? I don't get it. Currently, only objects created at the beginning through QEMU command line will be stored in the QemuOpts, objects that created with object_add won't stored in QemuOpts. Do you mean for DeviceState, DriveInfo they store there QemuOpts explicity so that they can delete it? Why don't we just delete it from objects directly instead? > >> } >> >> MemoryDeviceInfoList *qmp_query_memory_devices(Error **errp) > . >
Yang Hongyang <yanghy@cn.fujitsu.com> writes: > On 09/24/2015 03:43 PM, Markus Armbruster wrote: >> This has finally reached the front of my review queue. I apologize for >> the loooong delay. >> >> Copying Paolo for another pair of eyeballs (he wrote this code). >> > [...] >>> + >>> + opts = qemu_opts_find(qemu_find_opts_err("object", NULL), id); >>> + qemu_opts_del(opts); >> >> qemu_find_opts_err("object", &error_abort) please, because when it >> fails, we want to die right away, not when the null pointer it returns >> gets dereferenced. > > Thanks for the review. > Jason, do you want me to propose a fix on top of this series or simply drop > this for now because this patch is an independent bug fix and won't affect the > other filter patch series. > >> >> Same sloppiness in netdev_del_completion() and qmp_netdev_del(), not >> your patch's fault. >> >> Elsewhere, we store the QemuOpts in the object just so we can delete it: >> DeviceState, DriveInfo. Paolo, what do you think? > > I don't get it. Currently, only objects created at the beginning through > QEMU command line will be stored in the QemuOpts, objects that created > with object_add won't stored in QemuOpts. Do you mean for DeviceState, > DriveInfo they store there QemuOpts explicity so that they can delete it? > Why don't we just delete it from objects directly instead? Let me elaborate. We have the same pattern in multiple places: some kind of object gets configured via QemuOpts, and an object's QemuOpts need to stay around until the object dies. Example 1: Block device backends DriveInfo has a member opts. drive_new() stores the QemuOpts in dinfo->opts. drive_info_del() destroys dinfo->opts. Note: DriveInfo member opts is always non-null. But not every BlockBackend has a DriveInfo. Example 2: Device frontends DeviceState has a member opts. qdev_device_add() stores the QemuOpts in dev->opts. device_finalize() destroys dev->opts. Note: DeviceState member opts may be null (not every device is created by qdev_device_add()). Fine, because qemu_opts_del(NULL) is a no-op. Example 3: Character device backends CharDriverState has a member opts. qemu_chr_new_from_opts() stores the QemuOpts in chr->opts. qemu_chr_delete() destroys chr->opts. Example 4: Network device backends Two cases A. netdev qmp_netdev_add() does not store the QemuOpts. qmp_netdev_del() still needs to destroy it. It has to find it somehow. Here's how it does it: opts = qemu_opts_find(qemu_find_opts_err("netdev", NULL), id); if (!opts) { error_setg(errp, "Device '%s' is not a netdev", id); return; } The !opts condition is a non-obvious way to test "not created with -netdev", see commit 645c949. Note that the commit's claim that qemu_opts_del(NULL) crashes is no longer true since commit 4782183. B. Legacy net hmp_host_net_add() does not store the QemuOpts. hmp_host_net_remove() still needs to destroy it. I can't see where that happens, and I'm not sure it does. Example 5: Generic object object_create() does not store the QemuOpts. It still needs to be destroyed along with the object. It isn't, and your patch fixes it. Personally, I find the technique in example 1-3 easier to understand than the one in example 4-5.
On 09/24/2015 05:42 PM, Markus Armbruster wrote: > Yang Hongyang <yanghy@cn.fujitsu.com> writes: > >> On 09/24/2015 03:43 PM, Markus Armbruster wrote: >>> This has finally reached the front of my review queue. I apologize for >>> the loooong delay. >>> >>> Copying Paolo for another pair of eyeballs (he wrote this code). >>> >> [...] >>>> + >>>> + opts = qemu_opts_find(qemu_find_opts_err("object", NULL), id); >>>> + qemu_opts_del(opts); >>> >>> qemu_find_opts_err("object", &error_abort) please, because when it >>> fails, we want to die right away, not when the null pointer it returns >>> gets dereferenced. >> >> Thanks for the review. >> Jason, do you want me to propose a fix on top of this series or simply drop >> this for now because this patch is an independent bug fix and won't affect the >> other filter patch series. >> >>> >>> Same sloppiness in netdev_del_completion() and qmp_netdev_del(), not >>> your patch's fault. >>> >>> Elsewhere, we store the QemuOpts in the object just so we can delete it: >>> DeviceState, DriveInfo. Paolo, what do you think? >> >> I don't get it. Currently, only objects created at the beginning through >> QEMU command line will be stored in the QemuOpts, objects that created >> with object_add won't stored in QemuOpts. Do you mean for DeviceState, >> DriveInfo they store there QemuOpts explicity so that they can delete it? >> Why don't we just delete it from objects directly instead? > > Let me elaborate. Thanks very much for the elaboration. > > We have the same pattern in multiple places: some kind of object gets > configured via QemuOpts, and an object's QemuOpts need to stay around > until the object dies. > > Example 1: Block device backends > > DriveInfo has a member opts. > > drive_new() stores the QemuOpts in dinfo->opts. > > drive_info_del() destroys dinfo->opts. > > Note: DriveInfo member opts is always non-null. But not every > BlockBackend has a DriveInfo. > > Example 2: Device frontends > > DeviceState has a member opts. > > qdev_device_add() stores the QemuOpts in dev->opts. > > device_finalize() destroys dev->opts. > > Note: DeviceState member opts may be null (not every device is > created by qdev_device_add()). Fine, because qemu_opts_del(NULL) is > a no-op. > > Example 3: Character device backends > > CharDriverState has a member opts. > > qemu_chr_new_from_opts() stores the QemuOpts in chr->opts. > > qemu_chr_delete() destroys chr->opts. 1-3 store there ops in there own state, not in global ops group right? > > Example 4: Network device backends > > Two cases > > A. netdev > > qmp_netdev_add() does not store the QemuOpts. > > qmp_netdev_del() still needs to destroy it. It has to find it > somehow. Here's how it does it: > > opts = qemu_opts_find(qemu_find_opts_err("netdev", NULL), id); > if (!opts) { > error_setg(errp, "Device '%s' is not a netdev", id); > return; > } > > The !opts condition is a non-obvious way to test "not created > with -netdev", see commit 645c949. Note that the commit's claim > that qemu_opts_del(NULL) crashes is no longer true since commit > 4782183. > > B. Legacy net > > hmp_host_net_add() does not store the QemuOpts. I'm afraid it does store the QemuOpts, but not in it's own state. net/net.c: 1088 qemu_opt_set(opts, "type", device, &error_abort); This will store the QemuOpts, or am I misunderstood it? > > hmp_host_net_remove() still needs to destroy it. I can't see > where that happens, and I'm not sure it does. > > Example 5: Generic object > > object_create() does not store the QemuOpts. > > It still needs to be destroyed along with the object. It isn't, and > your patch fixes it. > > Personally, I find the technique in example 1-3 easier to understand > than the one in example 4-5. I agree that opts should not be used to determine not created something while there's case when something created but Opts not stored. > . >
On 09/24/2015 05:42 PM, Markus Armbruster wrote: > Yang Hongyang <yanghy@cn.fujitsu.com> writes: > >> On 09/24/2015 03:43 PM, Markus Armbruster wrote: >>> This has finally reached the front of my review queue. I apologize for >>> the loooong delay. >>> >>> Copying Paolo for another pair of eyeballs (he wrote this code). >>> >> [...] >>>> + >>>> + opts = qemu_opts_find(qemu_find_opts_err("object", NULL), id); >>>> + qemu_opts_del(opts); >>> >>> qemu_find_opts_err("object", &error_abort) please, because when it >>> fails, we want to die right away, not when the null pointer it returns >>> gets dereferenced. >> >> Thanks for the review. >> Jason, do you want me to propose a fix on top of this series or simply drop >> this for now because this patch is an independent bug fix and won't affect the >> other filter patch series. >> >>> >>> Same sloppiness in netdev_del_completion() and qmp_netdev_del(), not >>> your patch's fault. >>> >>> Elsewhere, we store the QemuOpts in the object just so we can delete it: >>> DeviceState, DriveInfo. Paolo, what do you think? >> >> I don't get it. Currently, only objects created at the beginning through >> QEMU command line will be stored in the QemuOpts, objects that created >> with object_add won't stored in QemuOpts. Do you mean for DeviceState, >> DriveInfo they store there QemuOpts explicity so that they can delete it? >> Why don't we just delete it from objects directly instead? > > Let me elaborate. > > We have the same pattern in multiple places: some kind of object gets > configured via QemuOpts, and an object's QemuOpts need to stay around > until the object dies. > > Example 1: Block device backends > > DriveInfo has a member opts. > > drive_new() stores the QemuOpts in dinfo->opts. > > drive_info_del() destroys dinfo->opts. > > Note: DriveInfo member opts is always non-null. But not every > BlockBackend has a DriveInfo. > > Example 2: Device frontends > > DeviceState has a member opts. > > qdev_device_add() stores the QemuOpts in dev->opts. > > device_finalize() destroys dev->opts. > > Note: DeviceState member opts may be null (not every device is > created by qdev_device_add()). Fine, because qemu_opts_del(NULL) is > a no-op. > > Example 3: Character device backends > > CharDriverState has a member opts. > > qemu_chr_new_from_opts() stores the QemuOpts in chr->opts. > > qemu_chr_delete() destroys chr->opts. > > Example 4: Network device backends > > Two cases > > A. netdev > > qmp_netdev_add() does not store the QemuOpts. The QemuOpts stored by qmp_netdev_add() and also hmp_netdev_add(). through this function: net/net.c: qmp_netdev_add() 1134 opts = qemu_opts_from_qdict(opts_list, qdict, &local_err); hmp.c: hmp_netdev_add() 1579 opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err); > > qmp_netdev_del() still needs to destroy it. It has to find it > somehow. Here's how it does it: > > opts = qemu_opts_find(qemu_find_opts_err("netdev", NULL), id); > if (!opts) { > error_setg(errp, "Device '%s' is not a netdev", id); > return; > } > > The !opts condition is a non-obvious way to test "not created > with -netdev", see commit 645c949. Note that the commit's claim > that qemu_opts_del(NULL) crashes is no longer true since commit > 4782183. > > B. Legacy net > > hmp_host_net_add() does not store the QemuOpts. > > hmp_host_net_remove() still needs to destroy it. I can't see > where that happens, and I'm not sure it does. > > Example 5: Generic object > > object_create() does not store the QemuOpts. > > It still needs to be destroyed along with the object. It isn't, and > your patch fixes it. > > Personally, I find the technique in example 1-3 easier to understand > than the one in example 4-5. > . >
Yang Hongyang <yanghy@cn.fujitsu.com> writes: > On 09/24/2015 05:42 PM, Markus Armbruster wrote: >> Yang Hongyang <yanghy@cn.fujitsu.com> writes: >> >>> On 09/24/2015 03:43 PM, Markus Armbruster wrote: >>>> This has finally reached the front of my review queue. I apologize for >>>> the loooong delay. >>>> >>>> Copying Paolo for another pair of eyeballs (he wrote this code). >>>> >>> [...] >>>>> + >>>>> + opts = qemu_opts_find(qemu_find_opts_err("object", NULL), id); >>>>> + qemu_opts_del(opts); >>>> >>>> qemu_find_opts_err("object", &error_abort) please, because when it >>>> fails, we want to die right away, not when the null pointer it returns >>>> gets dereferenced. >>> >>> Thanks for the review. >>> Jason, do you want me to propose a fix on top of this series or simply drop >>> this for now because this patch is an independent bug fix and won't >>> affect the >>> other filter patch series. >>> >>>> >>>> Same sloppiness in netdev_del_completion() and qmp_netdev_del(), not >>>> your patch's fault. >>>> >>>> Elsewhere, we store the QemuOpts in the object just so we can delete it: >>>> DeviceState, DriveInfo. Paolo, what do you think? >>> >>> I don't get it. Currently, only objects created at the beginning through >>> QEMU command line will be stored in the QemuOpts, objects that created >>> with object_add won't stored in QemuOpts. Do you mean for DeviceState, >>> DriveInfo they store there QemuOpts explicity so that they can delete it? >>> Why don't we just delete it from objects directly instead? >> >> Let me elaborate. > > Thanks very much for the elaboration. > >> >> We have the same pattern in multiple places: some kind of object gets >> configured via QemuOpts, and an object's QemuOpts need to stay around >> until the object dies. >> >> Example 1: Block device backends >> >> DriveInfo has a member opts. >> >> drive_new() stores the QemuOpts in dinfo->opts. >> >> drive_info_del() destroys dinfo->opts. >> >> Note: DriveInfo member opts is always non-null. But not every >> BlockBackend has a DriveInfo. >> >> Example 2: Device frontends >> >> DeviceState has a member opts. >> >> qdev_device_add() stores the QemuOpts in dev->opts. >> >> device_finalize() destroys dev->opts. >> >> Note: DeviceState member opts may be null (not every device is >> created by qdev_device_add()). Fine, because qemu_opts_del(NULL) is >> a no-op. >> >> Example 3: Character device backends >> >> CharDriverState has a member opts. >> >> qemu_chr_new_from_opts() stores the QemuOpts in chr->opts. >> >> qemu_chr_delete() destroys chr->opts. > > 1-3 store there ops in there own state, not in global ops group right? Both! But keeping a pointer in their own state simplifies calling qemu_opts_del() on destruction, and also makes it more obvious what is keeping the QemuOpts alive. >> Example 4: Network device backends >> >> Two cases >> >> A. netdev >> >> qmp_netdev_add() does not store the QemuOpts. >> >> qmp_netdev_del() still needs to destroy it. It has to find it >> somehow. Here's how it does it: >> >> opts = qemu_opts_find(qemu_find_opts_err("netdev", NULL), id); >> if (!opts) { >> error_setg(errp, "Device '%s' is not a netdev", id); >> return; >> } >> >> The !opts condition is a non-obvious way to test "not created >> with -netdev", see commit 645c949. Note that the commit's claim >> that qemu_opts_del(NULL) crashes is no longer true since commit >> 4782183. >> >> B. Legacy net >> >> hmp_host_net_add() does not store the QemuOpts. > > I'm afraid it does store the QemuOpts, but not in it's own state. > net/net.c: > 1088 qemu_opt_set(opts, "type", device, &error_abort); > This will store the QemuOpts, or am I misunderstood it? Doesn't store opts anywhere, actually. It merely modifies it (adds a parameter "type") >> >> hmp_host_net_remove() still needs to destroy it. I can't see >> where that happens, and I'm not sure it does. >> >> Example 5: Generic object >> >> object_create() does not store the QemuOpts. >> >> It still needs to be destroyed along with the object. It isn't, and >> your patch fixes it. >> >> Personally, I find the technique in example 1-3 easier to understand >> than the one in example 4-5. > > I agree that opts should not be used to determine not created something > while there's case when something created but Opts not stored.
Yang Hongyang <yanghy@cn.fujitsu.com> writes: > On 09/24/2015 05:42 PM, Markus Armbruster wrote: >> Yang Hongyang <yanghy@cn.fujitsu.com> writes: >> >>> On 09/24/2015 03:43 PM, Markus Armbruster wrote: >>>> This has finally reached the front of my review queue. I apologize for >>>> the loooong delay. >>>> >>>> Copying Paolo for another pair of eyeballs (he wrote this code). >>>> >>> [...] >>>>> + >>>>> + opts = qemu_opts_find(qemu_find_opts_err("object", NULL), id); >>>>> + qemu_opts_del(opts); >>>> >>>> qemu_find_opts_err("object", &error_abort) please, because when it >>>> fails, we want to die right away, not when the null pointer it returns >>>> gets dereferenced. >>> >>> Thanks for the review. >>> Jason, do you want me to propose a fix on top of this series or simply drop >>> this for now because this patch is an independent bug fix and won't >>> affect the >>> other filter patch series. >>> >>>> >>>> Same sloppiness in netdev_del_completion() and qmp_netdev_del(), not >>>> your patch's fault. >>>> >>>> Elsewhere, we store the QemuOpts in the object just so we can delete it: >>>> DeviceState, DriveInfo. Paolo, what do you think? >>> >>> I don't get it. Currently, only objects created at the beginning through >>> QEMU command line will be stored in the QemuOpts, objects that created >>> with object_add won't stored in QemuOpts. Do you mean for DeviceState, >>> DriveInfo they store there QemuOpts explicity so that they can delete it? >>> Why don't we just delete it from objects directly instead? >> >> Let me elaborate. >> >> We have the same pattern in multiple places: some kind of object gets >> configured via QemuOpts, and an object's QemuOpts need to stay around >> until the object dies. >> >> Example 1: Block device backends >> >> DriveInfo has a member opts. >> >> drive_new() stores the QemuOpts in dinfo->opts. >> >> drive_info_del() destroys dinfo->opts. >> >> Note: DriveInfo member opts is always non-null. But not every >> BlockBackend has a DriveInfo. >> >> Example 2: Device frontends >> >> DeviceState has a member opts. >> >> qdev_device_add() stores the QemuOpts in dev->opts. >> >> device_finalize() destroys dev->opts. >> >> Note: DeviceState member opts may be null (not every device is >> created by qdev_device_add()). Fine, because qemu_opts_del(NULL) is >> a no-op. >> >> Example 3: Character device backends >> >> CharDriverState has a member opts. >> >> qemu_chr_new_from_opts() stores the QemuOpts in chr->opts. >> >> qemu_chr_delete() destroys chr->opts. >> >> Example 4: Network device backends >> >> Two cases >> >> A. netdev >> >> qmp_netdev_add() does not store the QemuOpts. > > The QemuOpts stored by qmp_netdev_add() and also hmp_netdev_add(). > through this function: > net/net.c: qmp_netdev_add() > 1134 opts = qemu_opts_from_qdict(opts_list, qdict, &local_err); > > hmp.c: hmp_netdev_add() > 1579 opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err); That's where the QemuOpts are created. By "does not store" I mean "does not store in its own state, unlike example 1-3". >> >> qmp_netdev_del() still needs to destroy it. It has to find it >> somehow. Here's how it does it: >> >> opts = qemu_opts_find(qemu_find_opts_err("netdev", NULL), id); >> if (!opts) { >> error_setg(errp, "Device '%s' is not a netdev", id); >> return; >> } >> >> The !opts condition is a non-obvious way to test "not created >> with -netdev", see commit 645c949. Note that the commit's claim >> that qemu_opts_del(NULL) crashes is no longer true since commit >> 4782183. >> >> B. Legacy net >> >> hmp_host_net_add() does not store the QemuOpts. >> >> hmp_host_net_remove() still needs to destroy it. I can't see >> where that happens, and I'm not sure it does. >> >> Example 5: Generic object >> >> object_create() does not store the QemuOpts. >> >> It still needs to be destroyed along with the object. It isn't, and >> your patch fixes it. >> >> Personally, I find the technique in example 1-3 easier to understand >> than the one in example 4-5. >> . >>
On 09/24/2015 07:35 PM, Markus Armbruster wrote: > Yang Hongyang <yanghy@cn.fujitsu.com> writes: > >> On 09/24/2015 05:42 PM, Markus Armbruster wrote: >>> Yang Hongyang <yanghy@cn.fujitsu.com> writes: >>> >>>> On 09/24/2015 03:43 PM, Markus Armbruster wrote: >>>>> This has finally reached the front of my review queue. I apologize for >>>>> the loooong delay. >>>>> >>>>> Copying Paolo for another pair of eyeballs (he wrote this code). >>>>> >>>> [...] >>>>>> + >>>>>> + opts = qemu_opts_find(qemu_find_opts_err("object", NULL), id); >>>>>> + qemu_opts_del(opts); >>>>> >>>>> qemu_find_opts_err("object", &error_abort) please, because when it >>>>> fails, we want to die right away, not when the null pointer it returns >>>>> gets dereferenced. >>>> >>>> Thanks for the review. >>>> Jason, do you want me to propose a fix on top of this series or simply drop >>>> this for now because this patch is an independent bug fix and won't >>>> affect the >>>> other filter patch series. >>>> >>>>> >>>>> Same sloppiness in netdev_del_completion() and qmp_netdev_del(), not >>>>> your patch's fault. >>>>> >>>>> Elsewhere, we store the QemuOpts in the object just so we can delete it: >>>>> DeviceState, DriveInfo. Paolo, what do you think? >>>> >>>> I don't get it. Currently, only objects created at the beginning through >>>> QEMU command line will be stored in the QemuOpts, objects that created >>>> with object_add won't stored in QemuOpts. Do you mean for DeviceState, >>>> DriveInfo they store there QemuOpts explicity so that they can delete it? >>>> Why don't we just delete it from objects directly instead? >>> >>> Let me elaborate. >> >> Thanks very much for the elaboration. >> >>> >>> We have the same pattern in multiple places: some kind of object gets >>> configured via QemuOpts, and an object's QemuOpts need to stay around >>> until the object dies. >>> >>> Example 1: Block device backends >>> >>> DriveInfo has a member opts. >>> >>> drive_new() stores the QemuOpts in dinfo->opts. >>> >>> drive_info_del() destroys dinfo->opts. >>> >>> Note: DriveInfo member opts is always non-null. But not every >>> BlockBackend has a DriveInfo. >>> >>> Example 2: Device frontends >>> >>> DeviceState has a member opts. >>> >>> qdev_device_add() stores the QemuOpts in dev->opts. >>> >>> device_finalize() destroys dev->opts. >>> >>> Note: DeviceState member opts may be null (not every device is >>> created by qdev_device_add()). Fine, because qemu_opts_del(NULL) is >>> a no-op. >>> >>> Example 3: Character device backends >>> >>> CharDriverState has a member opts. >>> >>> qemu_chr_new_from_opts() stores the QemuOpts in chr->opts. >>> >>> qemu_chr_delete() destroys chr->opts. >> >> 1-3 store there ops in there own state, not in global ops group right? > > Both! But keeping a pointer in their own state simplifies calling > qemu_opts_del() on destruction, and also makes it more obvious what is > keeping the QemuOpts alive. I see. Thanks. > >>> Example 4: Network device backends >>> >>> Two cases >>> >>> A. netdev >>> >>> qmp_netdev_add() does not store the QemuOpts. >>> >>> qmp_netdev_del() still needs to destroy it. It has to find it >>> somehow. Here's how it does it: >>> >>> opts = qemu_opts_find(qemu_find_opts_err("netdev", NULL), id); >>> if (!opts) { >>> error_setg(errp, "Device '%s' is not a netdev", id); >>> return; >>> } >>> >>> The !opts condition is a non-obvious way to test "not created >>> with -netdev", see commit 645c949. Note that the commit's claim >>> that qemu_opts_del(NULL) crashes is no longer true since commit >>> 4782183. >>> >>> B. Legacy net >>> >>> hmp_host_net_add() does not store the QemuOpts. >> >> I'm afraid it does store the QemuOpts, but not in it's own state. >> net/net.c: >> 1088 qemu_opt_set(opts, "type", device, &error_abort); >> This will store the QemuOpts, or am I misunderstood it? > > Doesn't store opts anywhere, actually. It merely modifies it (adds a > parameter "type") As you said "store" means store in there own state, then I see...thanks > >>> >>> hmp_host_net_remove() still needs to destroy it. I can't see >>> where that happens, and I'm not sure it does. >>> >>> Example 5: Generic object >>> >>> object_create() does not store the QemuOpts. >>> >>> It still needs to be destroyed along with the object. It isn't, and >>> your patch fixes it. >>> >>> Personally, I find the technique in example 1-3 easier to understand >>> than the one in example 4-5. >> >> I agree that opts should not be used to determine not created something >> while there's case when something created but Opts not stored. > . >
On 09/24/2015 07:36 PM, Markus Armbruster wrote: > Yang Hongyang <yanghy@cn.fujitsu.com> writes: > >> On 09/24/2015 05:42 PM, Markus Armbruster wrote: >>> Yang Hongyang <yanghy@cn.fujitsu.com> writes: >>> >>>> On 09/24/2015 03:43 PM, Markus Armbruster wrote: >>>>> This has finally reached the front of my review queue. I apologize for >>>>> the loooong delay. >>>>> >>>>> Copying Paolo for another pair of eyeballs (he wrote this code). >>>>> >>>> [...] >>>>>> + >>>>>> + opts = qemu_opts_find(qemu_find_opts_err("object", NULL), id); >>>>>> + qemu_opts_del(opts); >>>>> >>>>> qemu_find_opts_err("object", &error_abort) please, because when it >>>>> fails, we want to die right away, not when the null pointer it returns >>>>> gets dereferenced. >>>> >>>> Thanks for the review. >>>> Jason, do you want me to propose a fix on top of this series or simply drop >>>> this for now because this patch is an independent bug fix and won't >>>> affect the >>>> other filter patch series. >>>> >>>>> >>>>> Same sloppiness in netdev_del_completion() and qmp_netdev_del(), not >>>>> your patch's fault. >>>>> >>>>> Elsewhere, we store the QemuOpts in the object just so we can delete it: >>>>> DeviceState, DriveInfo. Paolo, what do you think? >>>> >>>> I don't get it. Currently, only objects created at the beginning through >>>> QEMU command line will be stored in the QemuOpts, objects that created >>>> with object_add won't stored in QemuOpts. Do you mean for DeviceState, >>>> DriveInfo they store there QemuOpts explicity so that they can delete it? >>>> Why don't we just delete it from objects directly instead? >>> >>> Let me elaborate. >>> >>> We have the same pattern in multiple places: some kind of object gets >>> configured via QemuOpts, and an object's QemuOpts need to stay around >>> until the object dies. >>> >>> Example 1: Block device backends >>> >>> DriveInfo has a member opts. >>> >>> drive_new() stores the QemuOpts in dinfo->opts. >>> >>> drive_info_del() destroys dinfo->opts. >>> >>> Note: DriveInfo member opts is always non-null. But not every >>> BlockBackend has a DriveInfo. >>> >>> Example 2: Device frontends >>> >>> DeviceState has a member opts. >>> >>> qdev_device_add() stores the QemuOpts in dev->opts. >>> >>> device_finalize() destroys dev->opts. >>> >>> Note: DeviceState member opts may be null (not every device is >>> created by qdev_device_add()). Fine, because qemu_opts_del(NULL) is >>> a no-op. >>> >>> Example 3: Character device backends >>> >>> CharDriverState has a member opts. >>> >>> qemu_chr_new_from_opts() stores the QemuOpts in chr->opts. >>> >>> qemu_chr_delete() destroys chr->opts. >>> >>> Example 4: Network device backends >>> >>> Two cases >>> >>> A. netdev >>> >>> qmp_netdev_add() does not store the QemuOpts. >> >> The QemuOpts stored by qmp_netdev_add() and also hmp_netdev_add(). >> through this function: >> net/net.c: qmp_netdev_add() >> 1134 opts = qemu_opts_from_qdict(opts_list, qdict, &local_err); >> >> hmp.c: hmp_netdev_add() >> 1579 opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err); > > That's where the QemuOpts are created. By "does not store" I mean "does > not store in its own state, unlike example 1-3". Understand, thank you. > >>> >>> qmp_netdev_del() still needs to destroy it. It has to find it >>> somehow. Here's how it does it: >>> >>> opts = qemu_opts_find(qemu_find_opts_err("netdev", NULL), id); >>> if (!opts) { >>> error_setg(errp, "Device '%s' is not a netdev", id); >>> return; >>> } >>> >>> The !opts condition is a non-obvious way to test "not created >>> with -netdev", see commit 645c949. Note that the commit's claim >>> that qemu_opts_del(NULL) crashes is no longer true since commit >>> 4782183. >>> >>> B. Legacy net >>> >>> hmp_host_net_add() does not store the QemuOpts. >>> >>> hmp_host_net_remove() still needs to destroy it. I can't see >>> where that happens, and I'm not sure it does. >>> >>> Example 5: Generic object >>> >>> object_create() does not store the QemuOpts. >>> >>> It still needs to be destroyed along with the object. It isn't, and >>> your patch fixes it. >>> >>> Personally, I find the technique in example 1-3 easier to understand >>> than the one in example 4-5. >>> . >>> > . >
On 09/24/2015 04:35 PM, Yang Hongyang wrote: > On 09/24/2015 03:43 PM, Markus Armbruster wrote: >> This has finally reached the front of my review queue. I apologize for >> the loooong delay. >> >> Copying Paolo for another pair of eyeballs (he wrote this code). >> > [...] >>> + >>> + opts = qemu_opts_find(qemu_find_opts_err("object", NULL), id); >>> + qemu_opts_del(opts); >> >> qemu_find_opts_err("object", &error_abort) please, because when it >> fails, we want to die right away, not when the null pointer it returns >> gets dereferenced. > > Thanks for the review. > Jason, do you want me to propose a fix on top of this series or simply > drop > this for now because this patch is an independent bug fix and won't > affect the > other filter patch series. Will drop this patch from my tree. Thanks > >> >> Same sloppiness in netdev_del_completion() and qmp_netdev_del(), not >> your patch's fault. >> >> Elsewhere, we store the QemuOpts in the object just so we can delete it: >> DeviceState, DriveInfo. Paolo, what do you think? > > I don't get it. Currently, only objects created at the beginning through > QEMU command line will be stored in the QemuOpts, objects that created > with object_add won't stored in QemuOpts. Do you mean for DeviceState, > DriveInfo they store there QemuOpts explicity so that they can delete it? > Why don't we just delete it from objects directly instead? > >> >>> } >>> >>> MemoryDeviceInfoList *qmp_query_memory_devices(Error **errp) >> . >> >
diff --git a/qmp.c b/qmp.c index 9623c80..4bd44c3 100644 --- a/qmp.c +++ b/qmp.c @@ -686,6 +686,7 @@ void qmp_object_del(const char *id, Error **errp) { Object *container; Object *obj; + QemuOpts *opts; container = object_get_objects_root(); obj = object_resolve_path_component(container, id); @@ -699,6 +700,9 @@ void qmp_object_del(const char *id, Error **errp) return; } object_unparent(obj); + + opts = qemu_opts_find(qemu_find_opts_err("object", NULL), id); + qemu_opts_del(opts); } MemoryDeviceInfoList *qmp_query_memory_devices(Error **errp)
When delete an object, we need to delete the associated qemu opts, otherwise, we can not add another object with the same name using object_add. The case happens when we start qemu with: -object xxx,id=aa then we delete this object with: object_del aa then add the object with: object_add xxx,id=aa QEMU will return error with Duplicate ID error. Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com> --- qmp.c | 4 ++++ 1 file changed, 4 insertions(+)