|
From: | Like Xu |
Subject: | Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion |
Date: | Tue, 23 Apr 2019 15:59:31 +0800 |
User-agent: | Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 |
On 2019/4/18 1:10, Eduardo Habkost wrote:
On Wed, Apr 17, 2019 at 07:14:10AM +0200, Markus Armbruster wrote:Eduardo Habkost <address@hidden> writes:On Mon, Apr 15, 2019 at 03:59:45PM +0800, Like Xu wrote:To avoid the misuse of qdev_get_machine() if machine hasn't been created yet, this patch uses qdev_get_machine_uncheck() for obj-common (share with user-only mode) and adds type assertion to qdev_get_machine() in system-emulation mode. Suggested-by: Igor Mammedov <address@hidden> Signed-off-by: Like Xu <address@hidden>Reviewed-by: Eduardo Habkost <address@hidden> I'm queueing the series on machine-next, thanks!Hold your horses, please. I dislike the name qdev_get_machine_uncheck(). I could live with qdev_get_machine_unchecked(). However, I doubt this is the right approach. The issue at hand is undisciplined creation of QOM object /machine. This patch adds an asseertion "undisciplined creation of /machine didn't create crap", but only in some places. I think we should never create /machine as (surprising!) side effect of qdev_get_machine(). Create it explicitly instead, and have qdev_get_machine() use object_resolve_path("/machine", NULL) to get it. Look ma, no side effects.OK, I'm dropping this one while we discuss it. I really miss a good explanation why qdev_get_machine_unchecked() needs to exist. When exactly do we want /machine to exist but not be TYPE_MACHINE? Why?
AFAICT, there is no such "/machine" that is not of type TYPE_MACHINE.The original qdev_get_machine() would always return a "/container" object in user-only mode and there is none TYPE_MACHINE object.
In system emulation mode, it returns the same "/container" object at the beginning, until we initialize and add a TYPE_MACHINE object to the "/container" as a child and it would return OBJECT(current_machine)
for later usages. The starting point is to avoid using the legacy qdev_get_machine() in system emulation mode when we haven't added the "/machine" object.As a result, we introduced type checking assertions to avoid premature invocations.
In this proposal, the qdev_get_machine_unchecked() is only used in user-only mode, part of which shares with system emulation mode(such as device_set_realized, cpu_common_realizefn). The new qdev_get_machine() is only used in system emulation mode and type checking assertion does reduce the irrational use of this function (if any in the future).
We all agree to use this qdev_get_machine() as little as possible and this patch could make future clean up work easier.
Once the expectations and use cases are explained, we can choose a better name for qdev_get_machine_unchecked() and document it properly.
[Prev in Thread] | Current Thread | [Next in Thread] |