Re: [Qemu-devel] [PATCH 0/4] Remove some qdev_get_machine() calls from CONFIG_USER_ONLY

[Eduardo Habkost]

On Fri, Apr 26, 2019 at 04:55:17PM +0800, Like Xu wrote:
> On 2019/4/26 4:00, Eduardo Habkost wrote:
> > This series moves some qdev code outside qdev.o, so it can be
> > compiled only in CONFIG_SOFTMMU.
> > 
> > The code being moved includes two qdev_get_machine() calls, so
> > this will make it easier to move qdev_get_machine() to
> > CONFIG_SOFTMMU later.
> > 
> > After this series, there's one remaining qdev_get_machine() call
> > that seems more difficult to remove:
> > 
> >      static void device_set_realized(Object *obj, bool value, Error **errp)
> >      {
> >          /* [...] */
> >          if (!obj->parent) {
> >              gchar *name = g_strdup_printf("device[%d]", 
> > unattached_count++);
> > 
> >              object_property_add_child(container_get(qdev_get_machine(),
> >                                                      "/unattached"),
> >                                        name, obj, &error_abort);
> >              unattached_parent = true;
> >              g_free(name);
> >          }
> >          /* [...] */
> >      }
> > 
> 
> I may have an experimental patch to fix device_set_realized issue:
> 
> 1. in qdev_get_machine():
> replace
>       dev = container_get(object_get_root(), "/machine");
> with
>       dev = object_resolve_path("/machine", NULL);
> 
> 2. in device_set_realized():
> 
> Using
>       Object *container = qdev_get_machine() ?
>               qdev_get_machine() : object_get_root();
> and pass it to
>       object_property_add_child(
>               container_get(container, "/unattached"),
>               name, obj, &error_abort);

I wouldn't like to call qdev_get_machine() here (see below[1]),
but trying "/machine" first and falling back to object_get_root()
sounds good, for two reasons:

* It won't require "/machine" to exist at all, on *-user;
* It will allow machines to create unattached devices on
  instance_init without crashing (which is not possible today).

> 
> With this fix, we could say the qdev_get_machine() does
> return the "/machine" object (or null) not a confused "/container".
> 
> We could continue to use qdev_get_machine() in system emulation mode,
> getting rid of its surprising side effect as Markus said.
> 
> The return value of qdev_get_machine() in user-only mode
> is the same object returned by object_get_root(),
> so no semantic changes.

[1] I wouldn't like to have a different qdev_get_machine()
function in user-only mode.  I would like to simply not call
qdev_get_machine() at all unless we're in system emulation mode.

-- 
Eduardo