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

[Yang Hongyang]

On 09/24/2015 07:35 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.
> >
> > 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.
> .

--
Thanks,
Yang.