[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v11 01/12] qmp: delete qemu opts when delete an
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v11 01/12] qmp: delete qemu opts when delete an object |
Date: |
Thu, 24 Sep 2015 13:36:53 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Yang Hongyang <address@hidden> writes:
> On 09/24/2015 05:42 PM, Markus Armbruster wrote:
>> Yang Hongyang <address@hidden> 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.
>> .
>>
- [Qemu-devel] [PATCH v11 00/12] Add a netfilter object and netbuffer filter, Yang Hongyang, 2015/09/16
- [Qemu-devel] [PATCH v11 01/12] qmp: delete qemu opts when delete an object, Yang Hongyang, 2015/09/16
- Re: [Qemu-devel] [PATCH v11 01/12] qmp: delete qemu opts when delete an object, Markus Armbruster, 2015/09/24
- Re: [Qemu-devel] [PATCH v11 01/12] qmp: delete qemu opts when delete an object, Yang Hongyang, 2015/09/24
- Re: [Qemu-devel] [PATCH v11 01/12] qmp: delete qemu opts when delete an object, Markus Armbruster, 2015/09/24
- Re: [Qemu-devel] [PATCH v11 01/12] qmp: delete qemu opts when delete an object, Yang Hongyang, 2015/09/24
- Re: [Qemu-devel] [PATCH v11 01/12] qmp: delete qemu opts when delete an object, Markus Armbruster, 2015/09/24
- Re: [Qemu-devel] [PATCH v11 01/12] qmp: delete qemu opts when delete an object, Yang Hongyang, 2015/09/24
- Re: [Qemu-devel] [PATCH v11 01/12] qmp: delete qemu opts when delete an object, Yang Hongyang, 2015/09/24
- Re: [Qemu-devel] [PATCH v11 01/12] qmp: delete qemu opts when delete an object,
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH v11 01/12] qmp: delete qemu opts when delete an object, Yang Hongyang, 2015/09/24
- Re: [Qemu-devel] [PATCH v11 01/12] qmp: delete qemu opts when delete an object, Jason Wang, 2015/09/25
[Qemu-devel] [PATCH v11 03/12] netfilter: hook packets before net queue send, Yang Hongyang, 2015/09/16
[Qemu-devel] [PATCH v11 02/12] init/cleanup of netfilter object, Yang Hongyang, 2015/09/16
Re: [Qemu-devel] [PATCH v11 02/12] init/cleanup of netfilter object, Markus Armbruster, 2015/09/24