On 09/24/2015 07:35 PM, Markus Armbruster wrote:
Yang Hongyang <yan...@cn.fujitsu.com> writes:
On 09/24/2015 05:42 PM, Markus Armbruster wrote:
Yang Hongyang <yan...@cn.fujitsu.com> 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.
