On Tue, Apr 23, 2019 at 03:59:31PM +0800, Like Xu wrote:
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.
I'm confused. Both qdev_get_machine() and
qdev_get_machine_unchecked() still return the object at
"/machine". On softmmu, /machine will be of type TYPE_MACHINE.
On user-only, /machine will be of type "container".
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.
I believe Markus is suggesting that avoiding unwanted side
effects is even better than doing type checking after it's
already too late. In other words, it doesn't make sense to call
container_get("/machine") on system emulation mode.
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).
This part confuses me as well. qdev_get_machine_unchecked() is
used in both user-only and softmmu, isn't? Thus we can't say it
is only used in user-only mode.
But I don't think we agree when it would be appropriate to call
qdev_get_machine_unchecked() instead of qdev_get_machine().
On both examples in your patch, the code checks for TYPE_MACHINE
immediately after calling qdev_get_machine_unchecked(). If that
code is only useful in softmmu mode, when would anybody want to
call qdev_get_machine_unchecked() in user-only mode?
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.