Like Xu <like...@linux.intel.com> writes:
> On 2019/4/18 1:10, Eduardo Habkost wrote:
>> On Wed, Apr 17, 2019 at 07:14:10AM +0200, Markus Armbruster wrote:
>>> Eduardo Habkost <ehabk...@redhat.com> 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 <imamm...@redhat.com>
>>>>> Signed-off-by: Like Xu <like...@linux.intel.com>
>>>>
>>>> Reviewed-by: Eduardo Habkost <ehabk...@redhat.com>
>>>>
>>>> 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.
I don't think so.
If you ever call qdev_get_machine() before creating "/machine", you not
only get a bogus "container" object, you *also* set "/machine" to that
object. When main() later attempts to create the real "/machine", it
fails with "attempt to add duplicate property 'machine' to object (type
'container')", and aborts. See commit 1a3ec8c1564 and e2fb3fbbf9c.
> 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.
I don't think qdev_get_machine() per se is the problem. Its side effect
is. Quoting myself:
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.
>> Once the expectations and use cases are explained, we can choose
>> a better name for qdev_get_machine_unchecked() and document it
>> properly.
>>
