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

[Like Xu <like.xu@linux.intel.com>]

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.

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.
>