From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53625) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZfHYq-0001Ti-UD for qemu-devel@nongnu.org; Thu, 24 Sep 2015 21:12:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZfHYo-0000Du-9o for qemu-devel@nongnu.org; Thu, 24 Sep 2015 21:12:28 -0400 Received: from [59.151.112.132] (port=31558 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZfHYn-0000Cj-Aq for qemu-devel@nongnu.org; Thu, 24 Sep 2015 21:12:26 -0400 Message-ID: <56049F70.20402@cn.fujitsu.com> Date: Fri, 25 Sep 2015 09:12:16 +0800 From: Yang Hongyang MIME-Version: 1.0 References: <1442405768-23019-1-git-send-email-yanghy@cn.fujitsu.com> <1442405768-23019-2-git-send-email-yanghy@cn.fujitsu.com> <87a8sctg74.fsf@blackfin.pond.sub.org> <5603B5C8.7080201@cn.fujitsu.com> <87eghop2zt.fsf@blackfin.pond.sub.org> <5603CB33.7070104@cn.fujitsu.com> <878u7wnj56.fsf@blackfin.pond.sub.org> In-Reply-To: <878u7wnj56.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v11 01/12] qmp: delete qemu opts when delete an object List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: thuth@redhat.com, zhang.zhanghailiang@huawei.com, lizhijian@cn.fujitsu.com, jasowang@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, Paolo Bonzini On 09/24/2015 07:36 PM, Markus Armbruster wrote: > Yang Hongyang writes: > >> On 09/24/2015 05:42 PM, Markus Armbruster wrote: >>> Yang Hongyang 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.