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

[Markus Armbruster]

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.

       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.