| Message ID | 20230213140103.1518173-15-vsementsov@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | pci hotplug tracking | expand |
On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote: > The device field is redundant, because QOM path always include device > ID when this ID exist. The flipside to that view is that applications configuring QEMU are specifying the device ID for -device (CLI) / device_add (QMP) and not the QOM path. IOW, the device ID is the more interesting field than QOM path, so feels like the wrong one to be dropping. Is there any real benefit to dropping this ? > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > --- > docs/about/deprecated.rst | 9 +++++++++ > qapi/qdev.json | 12 ++++++++++-- > 2 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index da2e6fe63d..b389934691 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -171,6 +171,15 @@ accepted incorrect commands will return an error. Users should make sure that > all arguments passed to ``device_add`` are consistent with the documented > property types. > > +QEMU Machine Protocol (QMP) events > +---------------------------------- > + > +``DEVICE_DELETED`` & ``DEVICE_UNPLUG_GUEST_ERROR`` field ``device`` (since 8.0) > +''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''' > + > +Devices that has ``ID`` always has QOM path `/machine/peripheral/ID`. So, the > +``device`` field is redundant and deprecated. Use the ``path`` field instead. > + > Host Architectures > ------------------ > > diff --git a/qapi/qdev.json b/qapi/qdev.json > index 2708fb4e99..325ef554f9 100644 > --- a/qapi/qdev.json > +++ b/qapi/qdev.json > @@ -124,6 +124,9 @@ > # > # @path: the device's QOM path > # > +# Features: > +# @deprecated: Member @device is deprecated as redundant. Use @path instead. > +# > # Since: 1.5 > # > # Example: > @@ -135,7 +138,8 @@ > # > ## > { 'event': 'DEVICE_DELETED', > - 'data': { '*device': 'str', 'path': 'str' } } > + 'data': { '*device': { 'type': 'str', 'features': [ 'deprecated' ] }, > + 'path': 'str' } } > > ## > # @DEVICE_UNPLUG_GUEST_ERROR: > @@ -146,6 +150,9 @@ > # > # @path: the device's QOM path > # > +# Features: > +# @deprecated: Member @device is deprecated as redundant. Use @path instead. > +# > # Since: 6.2 > # > # Example: > @@ -157,4 +164,5 @@ > # > ## > { 'event': 'DEVICE_UNPLUG_GUEST_ERROR', > - 'data': { '*device': 'str', 'path': 'str' } } > + 'data': { '*device': { 'type': 'str', 'features': [ 'deprecated' ] }, > + 'path': 'str' } } > -- > 2.34.1 > With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote: >> The device field is redundant, because QOM path always include device >> ID when this ID exist. > > The flipside to that view is that applications configuring QEMU are > specifying the device ID for -device (CLI) / device_add (QMP) and > not the QOM path. IOW, the device ID is the more interesting field > than QOM path, so feels like the wrong one to be dropping. QOM path is a reliable way to identify a device. Device ID isn't: devices need not have one. Therefore, dropping the QOM path would be wrong. > Is there any real benefit to dropping this ? The device ID is a trap for the unwary: relying on it is fine until you run into a scenario where you have to deal with devices lacking IDs. I suggested to deprecate it in review of "[PATCH v3 14/15] qapi: introduce DEVICE_ON event" (Message-ID: <873579x67l.fsf@pond.sub.org>). Quote: We commonly send both device ID and QOM path, mostly for historical reasons: the former precede the latter. There are exceptions, such as query-cpus-fast. Can't say offhand whether CPUs can be created with IDs. [...] I'd be in favour of deprecating and deleting redundant device IDs in QMP output.
On Tue, Feb 14, 2023 at 09:54:22 +0100, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote: > >> The device field is redundant, because QOM path always include device > >> ID when this ID exist. > > > > The flipside to that view is that applications configuring QEMU are > > specifying the device ID for -device (CLI) / device_add (QMP) and > > not the QOM path. IOW, the device ID is the more interesting field > > than QOM path, so feels like the wrong one to be dropping. > > QOM path is a reliable way to identify a device. Device ID isn't: > devices need not have one. Therefore, dropping the QOM path would be > wrong. > > > Is there any real benefit to dropping this ? > > The device ID is a trap for the unwary: relying on it is fine until you > run into a scenario where you have to deal with devices lacking IDs. Note that libvirt's code is still using the 'device' bit rather than QOM path and the fix might not be entirely trivial although should not be too hard.
On Tue, Feb 14, 2023 at 09:54:22AM +0100, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote: > >> The device field is redundant, because QOM path always include device > >> ID when this ID exist. > > > > The flipside to that view is that applications configuring QEMU are > > specifying the device ID for -device (CLI) / device_add (QMP) and > > not the QOM path. IOW, the device ID is the more interesting field > > than QOM path, so feels like the wrong one to be dropping. > > QOM path is a reliable way to identify a device. Device ID isn't: > devices need not have one. Therefore, dropping the QOM path would be > wrong. > > > Is there any real benefit to dropping this ? > > The device ID is a trap for the unwary: relying on it is fine until you > run into a scenario where you have to deal with devices lacking IDs. When a mgmt app is configuring QEMU though, it does it exclusively with device ID values. If I add a device "-device foo,id=dev0", and then later hot-unplug it "device_del dev0", it is pretty reasonable to then expect that the DEVICE_DELETED even will then include the ID value the app has been using elsewhere. If the mgmt app is using IDs everywhere when dealing with a device, then trap effectively doesn't exist for their usage scenario. With regards, Daniel
On Tue, Feb 14, 2023 at 10:25:22AM +0100, Peter Krempa wrote: > On Tue, Feb 14, 2023 at 09:54:22 +0100, Markus Armbruster wrote: > > Daniel P. Berrangé <berrange@redhat.com> writes: > > > > > On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote: > > >> The device field is redundant, because QOM path always include device > > >> ID when this ID exist. > > > > > > The flipside to that view is that applications configuring QEMU are > > > specifying the device ID for -device (CLI) / device_add (QMP) and > > > not the QOM path. IOW, the device ID is the more interesting field > > > than QOM path, so feels like the wrong one to be dropping. > > > > QOM path is a reliable way to identify a device. Device ID isn't: > > devices need not have one. Therefore, dropping the QOM path would be > > wrong. > > > > > Is there any real benefit to dropping this ? > > > > The device ID is a trap for the unwary: relying on it is fine until you > > run into a scenario where you have to deal with devices lacking IDs. > > Note that libvirt's code is still using the 'device' bit rather than QOM > path and the fix might not be entirely trivial although should not be > too hard. What's the documented way to construct a QOM path, given only an ID as input ? With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Tue, Feb 14, 2023 at 10:25:22AM +0100, Peter Krempa wrote: >> On Tue, Feb 14, 2023 at 09:54:22 +0100, Markus Armbruster wrote: >> > Daniel P. Berrangé <berrange@redhat.com> writes: >> > >> > > On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote: >> > >> The device field is redundant, because QOM path always include device >> > >> ID when this ID exist. >> > > >> > > The flipside to that view is that applications configuring QEMU are >> > > specifying the device ID for -device (CLI) / device_add (QMP) and >> > > not the QOM path. IOW, the device ID is the more interesting field >> > > than QOM path, so feels like the wrong one to be dropping. >> > >> > QOM path is a reliable way to identify a device. Device ID isn't: >> > devices need not have one. Therefore, dropping the QOM path would be >> > wrong. >> > >> > > Is there any real benefit to dropping this ? >> > >> > The device ID is a trap for the unwary: relying on it is fine until you >> > run into a scenario where you have to deal with devices lacking IDs. >> >> Note that libvirt's code is still using the 'device' bit rather than QOM >> path and the fix might not be entirely trivial although should not be >> too hard. > > What's the documented way to construct a QOM path, given only an ID as > input ? QOM paths a gap in our documentation, even though the composition tree structure has been stable since day one, and is de facto ABI. Short answer: "/machine/peripheral/ID". Long answer follows. We have three "containers" under /machine that serve as parents for devices: * /machine/peripheral/ Parent of user-created devices with ID. Children are named "ID". Put there by qdev_set_id(), called from qdev_device_add_from_qdict(). On "user-created": Nothing stops board code to abuse qdev_set_id() for onboard devices, directly or indirectly, but it really, really shouldn't. * /machine/peripheral-anon/ Parent of user-created devices without ID. Children are named "device[N]", where N counts up from zero. Put there by qdev_set_id(), called from qdev_device_add_from_qdict(). Again, abuse by board code is possible, but would be wrong. Beware: a particular device's N changes when the set of devices created before it grows or shrinks. Messing with the machine type can change it (different onboard devices). * /machine/unattached/ Surrogate parent of onboard devices created without a parent. Put there by device_set_realized() (general case), qdev_connect_gpio_out_named() (input pins) , memory_region_do_init() (memory regions), qemu_create_machine() (the main sysbus). I believe this container was created as a convenience, so we don't have to retrofit parents to existing code. Probably abused ever since.
On 14/2/23 12:49, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > >> On Tue, Feb 14, 2023 at 10:25:22AM +0100, Peter Krempa wrote: >>> On Tue, Feb 14, 2023 at 09:54:22 +0100, Markus Armbruster wrote: >>>> Daniel P. Berrangé <berrange@redhat.com> writes: >>>> >>>>> On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote: >>>>>> The device field is redundant, because QOM path always include device >>>>>> ID when this ID exist. >>>>> >>>>> The flipside to that view is that applications configuring QEMU are >>>>> specifying the device ID for -device (CLI) / device_add (QMP) and >>>>> not the QOM path. IOW, the device ID is the more interesting field >>>>> than QOM path, so feels like the wrong one to be dropping. >>>> >>>> QOM path is a reliable way to identify a device. Device ID isn't: >>>> devices need not have one. Therefore, dropping the QOM path would be >>>> wrong. >>>> >>>>> Is there any real benefit to dropping this ? >>>> >>>> The device ID is a trap for the unwary: relying on it is fine until you >>>> run into a scenario where you have to deal with devices lacking IDs. >>> >>> Note that libvirt's code is still using the 'device' bit rather than QOM >>> path and the fix might not be entirely trivial although should not be >>> too hard. >> >> What's the documented way to construct a QOM path, given only an ID as >> input ? > > QOM paths a gap in our documentation, even though the composition tree > structure has been stable since day one, and is de facto ABI. > > Short answer: "/machine/peripheral/ID". > > Long answer follows. > > We have three "containers" under /machine that serve as parents for > devices: > > * /machine/peripheral/ > > Parent of user-created devices with ID. Children are named "ID". > > Put there by qdev_set_id(), called from qdev_device_add_from_qdict(). > > On "user-created": Nothing stops board code to abuse qdev_set_id() for > onboard devices, directly or indirectly, but it really, really > shouldn't. > > * /machine/peripheral-anon/ > > Parent of user-created devices without ID. Children are named > "device[N]", where N counts up from zero. > > Put there by qdev_set_id(), called from qdev_device_add_from_qdict(). > > Again, abuse by board code is possible, but would be wrong. > > Beware: a particular device's N changes when the set of devices > created before it grows or shrinks. Messing with the machine type can > change it (different onboard devices). > > * /machine/unattached/ > > Surrogate parent of onboard devices created without a parent. > > Put there by device_set_realized() (general case), > qdev_connect_gpio_out_named() (input pins) , memory_region_do_init() > (memory regions), qemu_create_machine() (the main sysbus). > > I believe this container was created as a convenience, so we don't > have to retrofit parents to existing code. Probably abused ever > since. Are you suggesting this is a stable interface and we can not move devices (like from /machine/unattached/ to /machine/peripheral/) without going thru the deprecation process?
Markus Armbruster <armbru@redhat.com> writes: > Daniel P. Berrangé <berrange@redhat.com> writes: > >> On Tue, Feb 14, 2023 at 10:25:22AM +0100, Peter Krempa wrote: >>> On Tue, Feb 14, 2023 at 09:54:22 +0100, Markus Armbruster wrote: >>> > Daniel P. Berrangé <berrange@redhat.com> writes: >>> > >>> > > On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote: >>> > >> The device field is redundant, because QOM path always include device >>> > >> ID when this ID exist. >>> > > >>> > > The flipside to that view is that applications configuring QEMU are >>> > > specifying the device ID for -device (CLI) / device_add (QMP) and >>> > > not the QOM path. IOW, the device ID is the more interesting field >>> > > than QOM path, so feels like the wrong one to be dropping. >>> > >>> > QOM path is a reliable way to identify a device. Device ID isn't: >>> > devices need not have one. Therefore, dropping the QOM path would be >>> > wrong. >>> > >>> > > Is there any real benefit to dropping this ? >>> > >>> > The device ID is a trap for the unwary: relying on it is fine until you >>> > run into a scenario where you have to deal with devices lacking IDs. >>> >>> Note that libvirt's code is still using the 'device' bit rather than QOM >>> path and the fix might not be entirely trivial although should not be >>> too hard. >> >> What's the documented way to construct a QOM path, given only an ID as >> input ? > > QOM paths a gap in our documentation, even though the composition tree > structure has been stable since day one, and is de facto ABI. > > Short answer: "/machine/peripheral/ID". > > Long answer follows. > > We have three "containers" under /machine that serve as parents for > devices: > > * /machine/peripheral/ > > Parent of user-created devices with ID. Children are named "ID". > > Put there by qdev_set_id(), called from qdev_device_add_from_qdict(). > > On "user-created": Nothing stops board code to abuse qdev_set_id() for > onboard devices, directly or indirectly, but it really, really > shouldn't. > > * /machine/peripheral-anon/ > > Parent of user-created devices without ID. Children are named > "device[N]", where N counts up from zero. > > Put there by qdev_set_id(), called from qdev_device_add_from_qdict(). > > Again, abuse by board code is possible, but would be wrong. > > Beware: a particular device's N changes when the set of devices > created before it grows or shrinks. Messing with the machine type can > change it (different onboard devices). Correction: that should affect only /machine/unattached/. Messing with -device and such affects /machine/peripheral-anon/. > * /machine/unattached/ > > Surrogate parent of onboard devices created without a parent. Forgot to mention: Children are named "device[N]", where N counts up from zero. > Put there by device_set_realized() (general case), > qdev_connect_gpio_out_named() (input pins) , memory_region_do_init() > (memory regions), qemu_create_machine() (the main sysbus). > > I believe this container was created as a convenience, so we don't > have to retrofit parents to existing code. Probably abused ever > since.
Daniel P. Berrangé <berrange@redhat.com> writes: > On Tue, Feb 14, 2023 at 09:54:22AM +0100, Markus Armbruster wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> > On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote: >> >> The device field is redundant, because QOM path always include device >> >> ID when this ID exist. >> > >> > The flipside to that view is that applications configuring QEMU are >> > specifying the device ID for -device (CLI) / device_add (QMP) and >> > not the QOM path. IOW, the device ID is the more interesting field >> > than QOM path, so feels like the wrong one to be dropping. >> >> QOM path is a reliable way to identify a device. Device ID isn't: >> devices need not have one. Therefore, dropping the QOM path would be >> wrong. >> >> > Is there any real benefit to dropping this ? >> >> The device ID is a trap for the unwary: relying on it is fine until you >> run into a scenario where you have to deal with devices lacking IDs. > > When a mgmt app is configuring QEMU though, it does it exclusively > with device ID values. If I add a device "-device foo,id=dev0", > and then later hot-unplug it "device_del dev0", it is pretty > reasonable to then expect that the DEVICE_DELETED even will then > include the ID value the app has been using elsewhere. The management application would be well advised to use QOM paths with device_del, because only that works even for devices created by default (which have no ID), and devices the user created behind the management application's back. > If the mgmt app is using IDs everywhere when dealing with a device, > then trap effectively doesn't exist for their usage scenario.
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > On 14/2/23 12:49, Markus Armbruster wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: [...] >>> What's the documented way to construct a QOM path, given only an ID as >>> input ? >> >> QOM paths a gap in our documentation, even though the composition tree >> structure has been stable since day one, and is de facto ABI. >> >> Short answer: "/machine/peripheral/ID". >> >> Long answer follows. >> >> We have three "containers" under /machine that serve as parents for >> devices: >> >> * /machine/peripheral/ >> >> Parent of user-created devices with ID. Children are named "ID". >> >> Put there by qdev_set_id(), called from qdev_device_add_from_qdict(). >> >> On "user-created": Nothing stops board code to abuse qdev_set_id() for >> onboard devices, directly or indirectly, but it really, really >> shouldn't. >> >> * /machine/peripheral-anon/ >> >> Parent of user-created devices without ID. Children are named >> "device[N]", where N counts up from zero. >> >> Put there by qdev_set_id(), called from qdev_device_add_from_qdict(). >> >> Again, abuse by board code is possible, but would be wrong. >> >> Beware: a particular device's N changes when the set of devices >> created before it grows or shrinks. Messing with the machine type can >> change it (different onboard devices). >> >> * /machine/unattached/ >> >> Surrogate parent of onboard devices created without a parent. >> >> Put there by device_set_realized() (general case), >> qdev_connect_gpio_out_named() (input pins) , memory_region_do_init() >> (memory regions), qemu_create_machine() (the main sysbus). >> >> I believe this container was created as a convenience, so we don't >> have to retrofit parents to existing code. Probably abused ever >> since. > > Are you suggesting this is a stable interface and we can not move > devices (like from /machine/unattached/ to /machine/peripheral/) > without going thru the deprecation process? Difficult question! The point of not changing interfaces incompatibly without a grace period / deprecation process is not breaking users of the interface. When an interface has always worked a certain way, its users may well depend on it, whether it's documented or not. The question to ask is always "will this break users?" For documented aspects, we generally assume it will. Doesn't mean we can simply assume "won't" for undocumented aspects. Does this make sense?
On 14.02.23 14:57, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > >> On Tue, Feb 14, 2023 at 09:54:22AM +0100, Markus Armbruster wrote: >>> Daniel P. Berrangé <berrange@redhat.com> writes: >>> >>>> On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote: >>>>> The device field is redundant, because QOM path always include device >>>>> ID when this ID exist. >>>> >>>> The flipside to that view is that applications configuring QEMU are >>>> specifying the device ID for -device (CLI) / device_add (QMP) and >>>> not the QOM path. IOW, the device ID is the more interesting field >>>> than QOM path, so feels like the wrong one to be dropping. >>> >>> QOM path is a reliable way to identify a device. Device ID isn't: >>> devices need not have one. Therefore, dropping the QOM path would be >>> wrong. >>> >>>> Is there any real benefit to dropping this ? >>> >>> The device ID is a trap for the unwary: relying on it is fine until you >>> run into a scenario where you have to deal with devices lacking IDs. >> >> When a mgmt app is configuring QEMU though, it does it exclusively >> with device ID values. If I add a device "-device foo,id=dev0", >> and then later hot-unplug it "device_del dev0", it is pretty >> reasonable to then expect that the DEVICE_DELETED even will then >> include the ID value the app has been using elsewhere. > > The management application would be well advised to use QOM paths with > device_del, because only that works even for devices created by default > (which have no ID), and devices the user created behind the management > application's back. > >> If the mgmt app is using IDs everywhere when dealing with a device, >> then trap effectively doesn't exist for their usage scenario. > What if we go one step further and deprecate "id" in device_add / device_del as well? So that user will have to use qom path also in device_add. We may return an error if user don't specify "machine/peripheral/" prefix.. Or allow to create device with any QOM path?
On 14/2/23 13:17, Markus Armbruster wrote: > Philippe Mathieu-Daudé <philmd@linaro.org> writes: > >> On 14/2/23 12:49, Markus Armbruster wrote: >>> Daniel P. Berrangé <berrange@redhat.com> writes: > > [...] > >>>> What's the documented way to construct a QOM path, given only an ID as >>>> input ? >>> >>> QOM paths a gap in our documentation, even though the composition tree >>> structure has been stable since day one, and is de facto ABI. >>> >>> Short answer: "/machine/peripheral/ID". >>> >>> Long answer follows. >>> >>> We have three "containers" under /machine that serve as parents for >>> devices: >>> >>> * /machine/peripheral/ >>> >>> Parent of user-created devices with ID. Children are named "ID". >>> >>> Put there by qdev_set_id(), called from qdev_device_add_from_qdict(). >>> >>> On "user-created": Nothing stops board code to abuse qdev_set_id() for >>> onboard devices, directly or indirectly, but it really, really >>> shouldn't. >>> >>> * /machine/peripheral-anon/ >>> >>> Parent of user-created devices without ID. Children are named >>> "device[N]", where N counts up from zero. >>> >>> Put there by qdev_set_id(), called from qdev_device_add_from_qdict(). >>> >>> Again, abuse by board code is possible, but would be wrong. >>> >>> Beware: a particular device's N changes when the set of devices >>> created before it grows or shrinks. Messing with the machine type can >>> change it (different onboard devices). >>> >>> * /machine/unattached/ >>> >>> Surrogate parent of onboard devices created without a parent. >>> >>> Put there by device_set_realized() (general case), >>> qdev_connect_gpio_out_named() (input pins) , memory_region_do_init() >>> (memory regions), qemu_create_machine() (the main sysbus). >>> >>> I believe this container was created as a convenience, so we don't >>> have to retrofit parents to existing code. Probably abused ever >>> since. >> >> Are you suggesting this is a stable interface and we can not move >> devices (like from /machine/unattached/ to /machine/peripheral/) >> without going thru the deprecation process? > > Difficult question! > > The point of not changing interfaces incompatibly without a grace period > / deprecation process is not breaking users of the interface. > > When an interface has always worked a certain way, its users may well > depend on it, whether it's documented or not. > > The question to ask is always "will this break users?" > > For documented aspects, we generally assume it will. Doesn't mean we > can simply assume "won't" for undocumented aspects. > > Does this make sense? Yes, but I never considered the QOM paths as a stable interface... I'm very surprised. "Automatically assigned to /machine/unattached/" doesn't seem quite stable...
On Tue, Feb 14, 2023 at 12:57:28PM +0100, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Tue, Feb 14, 2023 at 09:54:22AM +0100, Markus Armbruster wrote: > >> Daniel P. Berrangé <berrange@redhat.com> writes: > >> > >> > On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote: > >> >> The device field is redundant, because QOM path always include device > >> >> ID when this ID exist. > >> > > >> > The flipside to that view is that applications configuring QEMU are > >> > specifying the device ID for -device (CLI) / device_add (QMP) and > >> > not the QOM path. IOW, the device ID is the more interesting field > >> > than QOM path, so feels like the wrong one to be dropping. > >> > >> QOM path is a reliable way to identify a device. Device ID isn't: > >> devices need not have one. Therefore, dropping the QOM path would be > >> wrong. > >> > >> > Is there any real benefit to dropping this ? > >> > >> The device ID is a trap for the unwary: relying on it is fine until you > >> run into a scenario where you have to deal with devices lacking IDs. > > > > When a mgmt app is configuring QEMU though, it does it exclusively > > with device ID values. If I add a device "-device foo,id=dev0", > > and then later hot-unplug it "device_del dev0", it is pretty > > reasonable to then expect that the DEVICE_DELETED even will then > > include the ID value the app has been using elsewhere. > > The management application would be well advised to use QOM paths with > device_del, because only that works even for devices created by default > (which have no ID), and devices the user created behind the management > application's back. If an application is using -nodefaults, then the only devices which exist will be those which are hardwired into the machine, and they can't be used with device_del anyway as they're hardwired. So the only reason is to cope with devices created secretly by the users, and that's a hairy enough problem that most apps won't even try to cope with it. At least in terms of the device hotplug area, it feels like we're adding an extra hurdle for apps to solve a problem that they don't actually face in practice. QOM paths are needed in some other QMP commands though, where there is definite need to refer to devices that are hardwired, most obviously qom-set/qom-get. With regards, Daniel
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > On 14/2/23 13:17, Markus Armbruster wrote: >> Philippe Mathieu-Daudé <philmd@linaro.org> writes: >> >>> On 14/2/23 12:49, Markus Armbruster wrote: >>>> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> [...] >> >>>>> What's the documented way to construct a QOM path, given only an ID as >>>>> input ? >>>> >>>> QOM paths a gap in our documentation, even though the composition tree >>>> structure has been stable since day one, and is de facto ABI. >>>> >>>> Short answer: "/machine/peripheral/ID". >>>> >>>> Long answer follows. >>>> >>>> We have three "containers" under /machine that serve as parents for >>>> devices: >>>> >>>> * /machine/peripheral/ >>>> >>>> Parent of user-created devices with ID. Children are named "ID". >>>> >>>> Put there by qdev_set_id(), called from qdev_device_add_from_qdict(). >>>> >>>> On "user-created": Nothing stops board code to abuse qdev_set_id() for >>>> onboard devices, directly or indirectly, but it really, really >>>> shouldn't. >>>> >>>> * /machine/peripheral-anon/ >>>> >>>> Parent of user-created devices without ID. Children are named >>>> "device[N]", where N counts up from zero. >>>> >>>> Put there by qdev_set_id(), called from qdev_device_add_from_qdict(). >>>> >>>> Again, abuse by board code is possible, but would be wrong. >>>> >>>> Beware: a particular device's N changes when the set of devices >>>> created before it grows or shrinks. Messing with the machine type can >>>> change it (different onboard devices). >>>> >>>> * /machine/unattached/ >>>> >>>> Surrogate parent of onboard devices created without a parent. >>>> >>>> Put there by device_set_realized() (general case), >>>> qdev_connect_gpio_out_named() (input pins) , memory_region_do_init() >>>> (memory regions), qemu_create_machine() (the main sysbus). >>>> >>>> I believe this container was created as a convenience, so we don't >>>> have to retrofit parents to existing code. Probably abused ever >>>> since. >>> >>> Are you suggesting this is a stable interface and we can not move >>> devices (like from /machine/unattached/ to /machine/peripheral/) >>> without going thru the deprecation process? >> >> Difficult question! >> >> The point of not changing interfaces incompatibly without a grace period >> / deprecation process is not breaking users of the interface. >> >> When an interface has always worked a certain way, its users may well >> depend on it, whether it's documented or not. >> >> The question to ask is always "will this break users?" >> >> For documented aspects, we generally assume it will. Doesn't mean we >> can simply assume "won't" for undocumented aspects. >> >> Does this make sense? > > Yes, but I never considered the QOM paths as a stable interface... > I'm very surprised. I think it's a gray area. For a good part of the QMP interface, we make an effort to review and document, and to spell out what is stable and what isn't. Sadly, QOM and qdev are exceptions. Properties are an essential part of the QMP interface, yet they are virtually undocumented: closest we have is output of "-device TYPENAME,help", which is utterly inadequate. There is no systematic review. We've never been quite clear on which properties are part of the stable interface. QOM paths are a much less prominent part of the QMP interface, but they are a part. The structure of the QOM composition tree is undocumented. Are they part of the stable interface? Anybody's guess. I figure they weren't intended to be stable interace. But then a QOM path is the only way to device_del a device without ID. Gray area. Moreover, Hyrum's law[*] can catch up with us any time. > "Automatically assigned to /machine/unattached/" doesn't seem > quite stable... The practical difficulties in (ab)using these push them towards the unstable end of the gray area. [*] "With a sufficient number of users of an API, it does not matter what you promise in the contract: all observable behaviors of your system will be depended on by somebody."
Daniel P. Berrangé <berrange@redhat.com> writes: > On Tue, Feb 14, 2023 at 12:57:28PM +0100, Markus Armbruster wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> > On Tue, Feb 14, 2023 at 09:54:22AM +0100, Markus Armbruster wrote: >> >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> >> >> > On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote: >> >> >> The device field is redundant, because QOM path always include device >> >> >> ID when this ID exist. >> >> > >> >> > The flipside to that view is that applications configuring QEMU are >> >> > specifying the device ID for -device (CLI) / device_add (QMP) and >> >> > not the QOM path. IOW, the device ID is the more interesting field >> >> > than QOM path, so feels like the wrong one to be dropping. >> >> >> >> QOM path is a reliable way to identify a device. Device ID isn't: >> >> devices need not have one. Therefore, dropping the QOM path would be >> >> wrong. >> >> >> >> > Is there any real benefit to dropping this ? >> >> >> >> The device ID is a trap for the unwary: relying on it is fine until you >> >> run into a scenario where you have to deal with devices lacking IDs. >> > >> > When a mgmt app is configuring QEMU though, it does it exclusively >> > with device ID values. If I add a device "-device foo,id=dev0", >> > and then later hot-unplug it "device_del dev0", it is pretty >> > reasonable to then expect that the DEVICE_DELETED even will then >> > include the ID value the app has been using elsewhere. >> >> The management application would be well advised to use QOM paths with >> device_del, because only that works even for devices created by default >> (which have no ID), and devices the user created behind the management >> application's back. > > If an application is using -nodefaults, then the only devices which > exist will be those which are hardwired into the machine, and they > can't be used with device_del anyway as they're hardwired. Your trust in the sanity of our board code is touching ;) > So the only reason is to cope with devices created secretly by > the users, and that's a hairy enough problem that most apps won't > even try to cope with it. Fair enough. > At least in terms of the device hotplug area, it feels like we're > adding an extra hurdle for apps to solve a problem that they don't > actually face in practice. > > QOM paths are needed in some other QMP commands though, where > there is definite need to refer to devices that are hardwired, > most obviously qom-set/qom-get. Also query-cpus-fast, query-hotpluggable-cpus, and possibly more I missed.
On 14.02.23 19:28, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > >> On Tue, Feb 14, 2023 at 12:57:28PM +0100, Markus Armbruster wrote: >>> Daniel P. Berrangé <berrange@redhat.com> writes: >>> >>>> On Tue, Feb 14, 2023 at 09:54:22AM +0100, Markus Armbruster wrote: >>>>> Daniel P. Berrangé <berrange@redhat.com> writes: >>>>> >>>>>> On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote: >>>>>>> The device field is redundant, because QOM path always include device >>>>>>> ID when this ID exist. >>>>>> >>>>>> The flipside to that view is that applications configuring QEMU are >>>>>> specifying the device ID for -device (CLI) / device_add (QMP) and >>>>>> not the QOM path. IOW, the device ID is the more interesting field >>>>>> than QOM path, so feels like the wrong one to be dropping. >>>>> >>>>> QOM path is a reliable way to identify a device. Device ID isn't: >>>>> devices need not have one. Therefore, dropping the QOM path would be >>>>> wrong. >>>>> >>>>>> Is there any real benefit to dropping this ? >>>>> >>>>> The device ID is a trap for the unwary: relying on it is fine until you >>>>> run into a scenario where you have to deal with devices lacking IDs. >>>> >>>> When a mgmt app is configuring QEMU though, it does it exclusively >>>> with device ID values. If I add a device "-device foo,id=dev0", >>>> and then later hot-unplug it "device_del dev0", it is pretty >>>> reasonable to then expect that the DEVICE_DELETED even will then >>>> include the ID value the app has been using elsewhere. >>> >>> The management application would be well advised to use QOM paths with >>> device_del, because only that works even for devices created by default >>> (which have no ID), and devices the user created behind the management >>> application's back. >> >> If an application is using -nodefaults, then the only devices which >> exist will be those which are hardwired into the machine, and they >> can't be used with device_del anyway as they're hardwired. > > Your trust in the sanity of our board code is touching ;) > >> So the only reason is to cope with devices created secretly by >> the users, and that's a hairy enough problem that most apps won't >> even try to cope with it. > > Fair enough. > >> At least in terms of the device hotplug area, it feels like we're >> adding an extra hurdle for apps to solve a problem that they don't >> actually face in practice. >> >> QOM paths are needed in some other QMP commands though, where >> there is definite need to refer to devices that are hardwired, >> most obviously qom-set/qom-get. > > Also query-cpus-fast, query-hotpluggable-cpus, and possibly more I > missed. > So, finally, we don't have consensus on deprecating ids? For me the most strong argument is that if user specify id in device_add, user should get exactly that id in DEVICE_DELETED and other events. So if deprecate something, we'd better deprecate ids altogether, making users specify full QOM path even in device_add. But that seems quite painful for existing users with no visible benefit. So, if no objections, I plan to resend with old "optional id & qom_path" designation for devices. We still can do a deprecation in future.
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
[...]
> So, if no objections, I plan to resend with old "optional id & qom_path" designation for devices. We still can do a deprecation in future.
Yes, please.
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index da2e6fe63d..b389934691 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -171,6 +171,15 @@ accepted incorrect commands will return an error. Users should make sure that all arguments passed to ``device_add`` are consistent with the documented property types. +QEMU Machine Protocol (QMP) events +---------------------------------- + +``DEVICE_DELETED`` & ``DEVICE_UNPLUG_GUEST_ERROR`` field ``device`` (since 8.0) +''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''' + +Devices that has ``ID`` always has QOM path `/machine/peripheral/ID`. So, the +``device`` field is redundant and deprecated. Use the ``path`` field instead. + Host Architectures ------------------ diff --git a/qapi/qdev.json b/qapi/qdev.json index 2708fb4e99..325ef554f9 100644 --- a/qapi/qdev.json +++ b/qapi/qdev.json @@ -124,6 +124,9 @@ # # @path: the device's QOM path # +# Features: +# @deprecated: Member @device is deprecated as redundant. Use @path instead. +# # Since: 1.5 # # Example: @@ -135,7 +138,8 @@ # ## { 'event': 'DEVICE_DELETED', - 'data': { '*device': 'str', 'path': 'str' } } + 'data': { '*device': { 'type': 'str', 'features': [ 'deprecated' ] }, + 'path': 'str' } } ## # @DEVICE_UNPLUG_GUEST_ERROR: @@ -146,6 +150,9 @@ # # @path: the device's QOM path # +# Features: +# @deprecated: Member @device is deprecated as redundant. Use @path instead. +# # Since: 6.2 # # Example: @@ -157,4 +164,5 @@ # ## { 'event': 'DEVICE_UNPLUG_GUEST_ERROR', - 'data': { '*device': 'str', 'path': 'str' } } + 'data': { '*device': { 'type': 'str', 'features': [ 'deprecated' ] }, + 'path': 'str' } }
The device field is redundant, because QOM path always include device ID when this ID exist. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- docs/about/deprecated.rst | 9 +++++++++ qapi/qdev.json | 12 ++++++++++-- 2 files changed, 19 insertions(+), 2 deletions(-)