Re: [PATCH v2 08/13] qdev: Make qdev_get_machine() not use container_get()

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 20/12/24 18:24, Peter Xu wrote:
> On Fri, Dec 20, 2024 at 12:25:44PM +0100, Philippe Mathieu-Daudé wrote:
>> On 19/12/24 19:27, Philippe Mathieu-Daudé wrote:
>>> On 19/12/24 19:20, Philippe Mathieu-Daudé wrote:
>>>> On 21/11/24 20:21, Peter Xu wrote:
>>>>> Currently, qdev_get_machine() has a slight misuse on container_get(), as
>>>>> the helper says "get a container" but in reality the goal is to get the
>>>>> machine object.  It is still a "container" but not strictly.
>>>>>
>>>>> Note that it _may_ get a container (at "/machine") in our
>>>>> current unit test
>>>>> of test-qdev-global-props.c before all these changes, but it's probably
>>>>> unexpected and worked by accident.
>>>>>
>>>>> Switch to an explicit object_resolve_path_component(), with a
>>>>> side benefit
>>>>> that qdev_get_machine() can happen a lot, and we don't need to split the
>>>>> string ("/machine") every time.  This also paves way for making
>>>>> the helper
>>>>> container_get() never try to return a non-container at all.
>>>>>
>>>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>>>> ---
>>>>>    hw/core/qdev.c | 7 ++++++-
>>>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>>>> index 5f13111b77..b622be15ee 100644
>>>>> --- a/hw/core/qdev.c
>>>>> +++ b/hw/core/qdev.c
>>>>> @@ -817,7 +817,12 @@ Object *qdev_get_machine(void)
>>>>>        static Object *dev;
>>>>>        if (dev == NULL) {
>>>>> -        dev = container_get(object_get_root(), "/machine");
>>>>> +        dev = object_resolve_path_component(object_get_root(),
>>>>> "machine");
>>>>> +        /*
>>>>> +         * Any call to this function before machine is created
>>>>> is treated
>>>>> +         * as a programming error as of now.
>>>>> +         */
>>>>> +        assert(dev);
>>>>
>>>> This fails for user-emulation:
>>>>
>>>> ./qemu-x86_64 /bin/echo foo
>>>> qemu-x86_64: ../../hw/core/qdev.c:825: qdev_get_machine: Assertion
>>>> `dev' failed.
>>
>> OK so I guess I might have found a "fix" which is to simply not
>> call qdev_get_machine() for user emulation, but this involves some
>> invasive refactoring -- so will take time --.
> 
> Thanks for taking a look, Phil.  Yes this sounds clean.
> 
>>
>> I'm dropping this series for now, planning to merge it again on top
>> of my refactor once it is ready. Any clever / simpler fix is
>> obviously welcomed first.
> 
> I initially thought about this, which could also be clean but I then
> noticed LINUX_USER is poisoned..
> 
> ===8<===
> diff --git a/qom/object.c b/qom/object.c
> index 58897a79a7..da26e8d69b 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1729,7 +1729,19 @@ const char *object_property_get_type(Object *obj, const char *name, Error **errp
>       return prop->type;
>   }
> 
> +/*
> + * Create all QEMU default containers.
> + *
> + * For system emulations, "machine" and its sub-containers are only created
> + * when machine initializes (qemu_create_machine()).
> + *
> + * For user emulations, create "machine" before hand to make qdev realize()
> + * work by default.
> + */
>   static const char *const root_containers[] = {
> +#ifdef CONFIG_LINUX_USER
> +    "machine",
> +#endif
>       "chardevs",
>       "objects",
>       "backend"
> @@ -1740,10 +1752,6 @@ static Object *object_root_initialize(void)
>       Object *root = object_new(TYPE_CONTAINER);
>       int i;
> 
> -    /*
> -     * Create all QEMU system containers.  "machine" and its sub-containers
> -     * are only created when machine initializes (qemu_create_machine()).
> -     */
>       for (i = 0; i < ARRAY_SIZE(root_containers); i++) {
>           object_property_add_new_container(root, root_containers[i]);
>       }
> ===8<===
> 
> Maybe we could still move it somewhere that LINUX_USER is not poisoned
> (plus "unattached" be created too, more below)?
> 
> OTOH, this works for me:
> 
> ===8<===
> diff --git a/linux-user/main.c b/linux-user/main.c
> index b09af8d436..009b7695f2 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -819,6 +819,11 @@ int main(int argc, char **argv, char **envp)
>       set_preferred_target_page_bits(ctz32(host_page_size));
>       finalize_target_page_bits();
>   
> +    Object *fake_obj = object_property_add_new_container(object_get_root(),
> +                                                         "machine");
> +    object_property_add_new_container(fake_obj, "unattached");
> +
>       cpu = cpu_create(cpu_type);
>       env = cpu_env(cpu);
>       cpu_reset(cpu);
> ===8<===

I like it, simple enough, allowing to remove container_get() now.

> 
> So we need both "/machine" and "/machine/unattached" so far to make
> linux-user work.  Not sure if bsd-user/main.c needs similar care, but none
> of these look as clean.

Maybe add a common method in hw/core/qdev-user.c?
qemu_create_machine() or qdev_create_fake_machine()?