[Top][All Lists]

[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: Yang Hongyang
Subject: Re: [Qemu-devel] [PATCH v11 01/12] qmp: delete qemu opts when delete an object
Date: Fri, 25 Sep 2015 09:12:16 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0

On 09/24/2015 07:36 PM, Markus Armbruster wrote:
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".

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);

         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

      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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]