* Re: [PATCH for-6.2] docs: Deprecate incorrectly typed device_add arguments
2021-11-11 14:35 [PATCH for-6.2] docs: Deprecate incorrectly typed device_add arguments Kevin Wolf
@ 2021-11-11 14:37 ` Daniel P. Berrangé
2021-11-11 14:52 ` Markus Armbruster
1 sibling, 0 replies; 3+ messages in thread
From: Daniel P. Berrangé @ 2021-11-11 14:37 UTC (permalink / raw)
To: Kevin Wolf; +Cc: libvir-list, pbonzini, eblake, qemu-devel, armbru
On Thu, Nov 11, 2021 at 03:35:30PM +0100, Kevin Wolf wrote:
> While introducing a non-QemuOpts code path for device creation for JSON
> -device, we noticed that QMP device_add doesn't check its input
> correctly (accepting arguments that should have been rejected), and that
> users may be relying on this behaviour (libvirt did until it was fixed
> recently).
>
> Let's use a deprecation period before we fix this bug in QEMU to avoid
> nasty surprises for users.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> docs/about/deprecated.rst | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH for-6.2] docs: Deprecate incorrectly typed device_add arguments
2021-11-11 14:35 [PATCH for-6.2] docs: Deprecate incorrectly typed device_add arguments Kevin Wolf
2021-11-11 14:37 ` Daniel P. Berrangé
@ 2021-11-11 14:52 ` Markus Armbruster
1 sibling, 0 replies; 3+ messages in thread
From: Markus Armbruster @ 2021-11-11 14:52 UTC (permalink / raw)
To: Kevin Wolf; +Cc: libvir-list, berrange, eblake, qemu-devel, pbonzini
Kevin Wolf <kwolf@redhat.com> writes:
> While introducing a non-QemuOpts code path for device creation for JSON
> -device, we noticed that QMP device_add doesn't check its input
> correctly (accepting arguments that should have been rejected), and that
> users may be relying on this behaviour (libvirt did until it was fixed
> recently).
Actually, we've known this for years. netdev_add had the same bug, and
we delayed its QAPIfication for fear of breaking stuff relying on the
bug.
commit db2a380c84574d8c76d7193b8af8535234fe5156
Author: Eric Blake <eblake@redhat.com>
Date: Tue Mar 17 15:17:10 2020 -0500
net: Complete qapi-fication of netdev_add
We've had all the required pieces for doing a type-safe representation
of netdev_add as a flat union for quite some time now (since
0e55c381f6 in v2.7.0, released in 2016), but did not make the final
switch to using it because of concern about whether a command-line
regression in accepting "1" in place of 1 for integer arguments would
be problematic. Back then, we did not have the deprecation cycle to
allow us to make progress. But now that we have waited so long, other
problems have crept in: for example, our desire to add
qemu-storage-daemon is hampered by the inability to express net
objects, and we are unable to introspect what we actually accept.
Additionally, our round-trip through QemuOpts silently eats any
argument that expands to an array, rendering dnssearch, hostfwd, and
guestfwd useless through QMP:
{"execute": "netdev_add", "arguments": { "id": "netdev0",
"type": "user", "dnssearch": [
{ "str": "8.8.8.8" }, { "str": "8.8.4.4" }
]}}
So without further ado, let's turn on proper QAPI. netdev_add() was a
[...]
Release note for 5.0:
1.28.1 QMP
[...]
* netdev_add improvements:
- Don't ignore arguments \dnssearch, hostfwd, and guestfwd
- Drop undocumented and unwanted argument ipv6-net
- Stricter argument type checking
This is not a demand for a commit message change.
>
> Let's use a deprecation period before we fix this bug in QEMU to avoid
> nasty surprises for users.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> docs/about/deprecated.rst | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 600031210d..c03fcf951f 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -250,6 +250,20 @@ options are removed in favor of using explicit ``blockdev-create`` and
> ``blockdev-add`` calls. See :doc:`/interop/live-block-operations` for
> details.
>
> +Incorrectly typed ``device_add`` arguments (since 6.2)
> +''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +Due to shortcomings in the internal implementation of ``device_add``, QEMU
> +incorrectly accepts certain invalid arguments: Any object or list arguments are
> +silently ignored. Other argument types are not checked, but an implicit
> +conversion happens, so that e.g. string values can be assigned to integer
> +device properties or vice versa.
> +
> +This is a bug in QEMU that will be fixed in the future so that previously
> +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.
> +
> System accelerators
> -------------------
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 3+ messages in thread