Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion

[Markus Armbruster <armbru@redhat.com>]

Like Xu <like.xu@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 <ehabkost@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 <imammedo@redhat.com>
>>>>> Signed-off-by: Like Xu <like.xu@linux.intel.com>
>>>>
>>>> Reviewed-by: Eduardo Habkost <ehabkost@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.
>>