| Message ID | 1555315185-16414-3-git-send-email-like.xu@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | vl.c: make current_machine as non-global variable | expand |
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!
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.
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? Once the expectations and use cases are explained, we can choose a better name for qdev_get_machine_unchecked() and document it properly.
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. >
On Tue, Apr 23, 2019 at 03:59:31PM +0800, Like Xu wrote: > 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. I'm confused. Both qdev_get_machine() and qdev_get_machine_unchecked() still return the object at "/machine". On softmmu, /machine will be of type TYPE_MACHINE. On user-only, /machine will be of type "container". > > 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. I believe Markus is suggesting that avoiding unwanted side effects is even better than doing type checking after it's already too late. In other words, it doesn't make sense to call container_get("/machine") on system emulation mode. > > 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). This part confuses me as well. qdev_get_machine_unchecked() is used in both user-only and softmmu, isn't? Thus we can't say it is only used in user-only mode. I think we all agree that qdev_get_machine() should eventually be available in softmmu only. But I don't think we agree when it would be appropriate to call qdev_get_machine_unchecked() instead of qdev_get_machine(). On both examples in your patch, the code checks for TYPE_MACHINE immediately after calling qdev_get_machine_unchecked(). If that code is only useful in softmmu mode, when would anybody want to call qdev_get_machine_unchecked() in user-only mode? > > 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. > > >
On 2019/4/25 1:21, Eduardo Habkost wrote: > On Tue, Apr 23, 2019 at 03:59:31PM +0800, Like Xu wrote: >> 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. > > I'm confused. Both qdev_get_machine() and > qdev_get_machine_unchecked() still return the object at > "/machine". On softmmu, /machine will be of type TYPE_MACHINE. > On user-only, /machine will be of type "container". > > >> >> 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. > > I believe Markus is suggesting that avoiding unwanted side > effects is even better than doing type checking after it's > already too late. In other words, it doesn't make sense to call > container_get("/machine") on system emulation mode. I agree. > > >> >> 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). > > This part confuses me as well. qdev_get_machine_unchecked() is > used in both user-only and softmmu, isn't? Thus we can't say it > is only used in user-only mode. You're right about this. > > I think we all agree that qdev_get_machine() should eventually be > available in softmmu only. I think we need to make it happen to avoid calling qdev_get_machine() in user-only mode. > > But I don't think we agree when it would be appropriate to call > qdev_get_machine_unchecked() instead of qdev_get_machine(). > > On both examples in your patch, the code checks for TYPE_MACHINE > immediately after calling qdev_get_machine_unchecked(). If that > code is only useful in softmmu mode, when would anybody want to > call qdev_get_machine_unchecked() in user-only mode? > > >> >> 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. >>> >> >
On Thu, Apr 25, 2019 at 11:12:29AM +0800, Like Xu wrote: > On 2019/4/25 1:21, Eduardo Habkost wrote: [...] > > > > I think we all agree that qdev_get_machine() should eventually be > > available in softmmu only. > > I think we need to make it happen to avoid calling qdev_get_machine() > in user-only mode. Agreed. My point is that we we shouldn't need a qdev_get_machine_unchecked() function at all if we first get rid of all user-only qdev_get_machine() calls. > > > > But I don't think we agree when it would be appropriate to call > > qdev_get_machine_unchecked() instead of qdev_get_machine(). > > > > On both examples in your patch, the code checks for TYPE_MACHINE > > immediately after calling qdev_get_machine_unchecked(). If that > > code is only useful in softmmu mode, when would anybody want to > > call qdev_get_machine_unchecked() in user-only mode? > > > >
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. >>
Eduardo Habkost <ehabkost@redhat.com> writes: > On Thu, Apr 25, 2019 at 11:12:29AM +0800, Like Xu wrote: >> On 2019/4/25 1:21, Eduardo Habkost wrote: > [...] >> > >> > I think we all agree that qdev_get_machine() should eventually be >> > available in softmmu only. >> >> I think we need to make it happen to avoid calling qdev_get_machine() >> in user-only mode. That would be ideal. > Agreed. My point is that we we shouldn't need a > qdev_get_machine_unchecked() function at all if we first get rid > of all user-only qdev_get_machine() calls. Concur.
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index f9b6efe..8232216 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -223,7 +223,7 @@ HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev) { MachineState *machine; MachineClass *mc; - Object *m_obj = qdev_get_machine(); + Object *m_obj = qdev_get_machine_uncheck(); if (object_dynamic_cast(m_obj, TYPE_MACHINE)) { machine = MACHINE(m_obj); @@ -815,7 +815,7 @@ 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(), + object_property_add_child(container_get(qdev_get_machine_uncheck(), "/unattached"), name, obj, &error_abort); unattached_parent = true; @@ -1095,7 +1095,7 @@ void device_reset(DeviceState *dev) } } -Object *qdev_get_machine(void) +Object *qdev_get_machine_uncheck(void) { static Object *dev; @@ -1106,6 +1106,16 @@ Object *qdev_get_machine(void) return dev; } +Object *qdev_get_machine(void) +{ + static Object *dev; + + dev = qdev_get_machine_uncheck(); + assert(object_dynamic_cast(dev, TYPE_MACHINE) != NULL); + + return dev; +} + static const TypeInfo device_type_info = { .name = TYPE_DEVICE, .parent = TYPE_OBJECT, diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 33ed3b8..e7c6a5a 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -429,6 +429,7 @@ const struct VMStateDescription *qdev_get_vmsd(DeviceState *dev); const char *qdev_fw_name(DeviceState *dev); +Object *qdev_get_machine_uncheck(void); Object *qdev_get_machine(void); /* FIXME: make this a link<> */ diff --git a/qom/cpu.c b/qom/cpu.c index a8d2958..bb877d5 100644 --- a/qom/cpu.c +++ b/qom/cpu.c @@ -325,9 +325,10 @@ static void cpu_common_parse_features(const char *typename, char *features, static void cpu_common_realizefn(DeviceState *dev, Error **errp) { CPUState *cpu = CPU(dev); - Object *machine = qdev_get_machine(); + Object *machine = qdev_get_machine_uncheck(); - /* qdev_get_machine() can return something that's not TYPE_MACHINE + /* + * qdev_get_machine_uncheck() can return something that's not TYPE_MACHINE * if this is one of the user-only emulators; in that case there's * no need to check the ignore_memory_transaction_failures board flag. */
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> --- hw/core/qdev.c | 16 +++++++++++++--- include/hw/qdev-core.h | 1 + qom/cpu.c | 5 +++-- 3 files changed, 17 insertions(+), 5 deletions(-)