[PATCH for-6.2] docs: Deprecate incorrectly typed device_add arguments

[PATCH for-6.2] docs: Deprecate incorrectly typed device_add arguments

[Kevin Wolf]

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(+)

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
 -------------------
 
-- 
2.31.1

Re: [PATCH for-6.2] docs: Deprecate incorrectly typed device_add arguments

[Daniel P. Berrangé]

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 :|

Re: [PATCH for-6.2] docs: Deprecate incorrectly typed device_add arguments

[Markus Armbruster]

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>