Re: [Qemu-devel] [PATCH v11 01/12] qmp: delete qemu opts when delete an object

[Yang Hongyang <yanghy@cn.fujitsu.com>]

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.
>>> .
>>>
> .
>

-- 
Thanks,
Yang.