[PATCH] python/qom: Fix qom-set failure

[PATCH] python/qom: Fix qom-set failure

[Wang Bing-hua]

Fix the following failure by interpreting 'value' argument as 'int'.

$ scripts/qmp/qom-set -s /tmp/qmp-socket /machine/unattached/device[6].temperature 0
QMPResponseError: Invalid parameter type for 'temperature', expected: integer

Fixes: c750c02891a8 ("python/qmp: Add qom script rewrites")
Signed-off-by: Wang Bing-hua <louiswpf@gmail.com>
---
 python/qemu/qmp/qom.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/python/qemu/qmp/qom.py b/python/qemu/qmp/qom.py
index 8ff28a8343..0b77dc6aa3 100644
--- a/python/qemu/qmp/qom.py
+++ b/python/qemu/qmp/qom.py
@@ -72,6 +72,7 @@ def configure_parser(cls, parser: argparse.ArgumentParser) -> None:
         cls.add_path_prop_arg(parser)
         parser.add_argument(
             'value',
+            type=int,
             metavar='<value>',
             action='store',
             help='new QOM property value'
-- 
2.34.1

Re: [PATCH] python/qom: Fix qom-set failure

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 2094 bytes --]

On Mon, Dec 20, 2021 at 12:46 PM Wang Bing-hua <louiswpf@gmail.com> wrote:

> Fix the following failure by interpreting 'value' argument as 'int'.
>
>
Thanks for the patch. Do you use the qom tools often? I wasn't sure anybody
did ...


> $ scripts/qmp/qom-set -s /tmp/qmp-socket
> /machine/unattached/device[6].temperature 0
> QMPResponseError: Invalid parameter type for 'temperature', expected:
> integer
>
> Fixes: c750c02891a8 ("python/qmp: Add qom script rewrites")
> Signed-off-by: Wang Bing-hua <louiswpf@gmail.com>
> ---
>  python/qemu/qmp/qom.py | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/python/qemu/qmp/qom.py b/python/qemu/qmp/qom.py
> index 8ff28a8343..0b77dc6aa3 100644
> --- a/python/qemu/qmp/qom.py
> +++ b/python/qemu/qmp/qom.py
> @@ -72,6 +72,7 @@ def configure_parser(cls, parser:
> argparse.ArgumentParser) -> None:
>          cls.add_path_prop_arg(parser)
>          parser.add_argument(
>              'value',
> +            type=int,
>              metavar='<value>',
>              action='store',
>              help='new QOM property value'
> --
> 2.34.1
>
>
Is this always going to be correct, though? QOM property values aren't
*always* integers. Won't this break other cases?

The old qom-set script did this [1]:
> print(srv.command('qom-set', path=path, property=prop, value=value))

which looks an awful lot like the old qom-set just passed a string along,
too.

Two ideas:

(1) try qom-get on the same property and just take note of what type it is
that you get back from the server. e.g.

rsp = self.qmp.command('qom-get', path=self.path, property=self.prop)
if isinstance(rsp, int):
    # Property we are setting must be an int
else:
    # It's something else.

(2) use a query to just determine the type. qom-list with
path=/tmp/qmp-socket /machine/unattached/device[6] will return a list of
dicts; filter out for the one where "name" is "temperature", then use the
"type" value to know what type we should expect from the user.

--js

[1]
https://gitlab.com/jsnow/qemu/-/blob/553032db17440f8de011390e5a1cfddd13751b0b/scripts/qmp/qom-set#L66

[-- Attachment #2: Type: text/html, Size: 3577 bytes --]

Re: [PATCH] python/qom: Fix qom-set failure

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Mon, Dec 20, 2021 at 12:46 PM Wang Bing-hua <louiswpf@gmail.com> wrote:
>
>> Fix the following failure by interpreting 'value' argument as 'int'.
>>
>>
> Thanks for the patch. Do you use the qom tools often? I wasn't sure anybody
> did ...
>
>
>> $ scripts/qmp/qom-set -s /tmp/qmp-socket
>> /machine/unattached/device[6].temperature 0
>> QMPResponseError: Invalid parameter type for 'temperature', expected:
>> integer
>>
>> Fixes: c750c02891a8 ("python/qmp: Add qom script rewrites")
>> Signed-off-by: Wang Bing-hua <louiswpf@gmail.com>
>> ---
>>  python/qemu/qmp/qom.py | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/python/qemu/qmp/qom.py b/python/qemu/qmp/qom.py
>> index 8ff28a8343..0b77dc6aa3 100644
>> --- a/python/qemu/qmp/qom.py
>> +++ b/python/qemu/qmp/qom.py
>> @@ -72,6 +72,7 @@ def configure_parser(cls, parser:
>> argparse.ArgumentParser) -> None:
>>          cls.add_path_prop_arg(parser)
>>          parser.add_argument(
>>              'value',
>> +            type=int,
>>              metavar='<value>',
>>              action='store',
>>              help='new QOM property value'
>> --
>> 2.34.1
>>
>>
> Is this always going to be correct, though? QOM property values aren't
> *always* integers. Won't this break other cases?

I believe it will.

The QMP core parses arguments (which are JSON values) into QObjects.
JSON strings become QString, JSON booleans becomes QBool, and so forth.

qmp_qom_set() feeds its value argument to the property's .set() method
together with a QObject input visitor.  Fails when its the wrong kind of
QObject.

The problem is figuring out what the right kind is.

QAPI schema introspection can't tell you, because we declare: 'value':
'any'.

QOM introspection is (in my educated opinion) crap.  See below.

> The old qom-set script did this [1]:
>> print(srv.command('qom-set', path=path, property=prop, value=value))
>
> which looks an awful lot like the old qom-set just passed a string along,
> too.
>
> Two ideas:
>
> (1) try qom-get on the same property and just take note of what type it is
> that you get back from the server. e.g.
>
> rsp = self.qmp.command('qom-get', path=self.path, property=self.prop)
> if isinstance(rsp, int):
>     # Property we are setting must be an int
> else:
>     # It's something else.

Assumes .set() accepts the kind of object .get() returns, which should
be the case.

However, .set() could accept more.  And the kind of object .get()
returns could depend on state.  Which kind is the right one to use for
the string we get from the CLI?  We can't know.  What we can is guess.
There will always be cases where we guess wrong.

My advice is to stay away from this program.

> (2) use a query to just determine the type. qom-list with
> path=/tmp/qmp-socket /machine/unattached/device[6] will return a list of
> dicts; filter out for the one where "name" is "temperature", then use the
> "type" value to know what type we should expect from the user.

This is QOM introspection.  Possible "type" values are documented in
qom.json:

    # @type: the type of the property.  This will typically come in one of four
    #        forms:
    #
    #        1) A primitive type such as 'u8', 'u16', 'bool', 'str', or 'double'.
    #           These types are mapped to the appropriate JSON type.
    #
    #        2) A child type in the form 'child<subtype>' where subtype is a qdev
    #           device type name.  Child properties create the composition tree.
    #
    #        3) A link type in the form 'link<subtype>' where subtype is a qdev
    #           device type name.  Link properties form the device model graph.

I like that it says "one of four", then lists three.  Fair warning to
the reader not to trust this.

In fact, 1) is aspirational.  The value is whatever C code passes to
object_property_add().  Actually values include "bool", "int", "int32",
"size", "string", "uint16", "uint32", "uint64", "uint8",
"GuestPanicInformation", "QemuUUID", "X86CPUFeatureWordInfo", my
favorites "container", "guest statistics", "struct tm", and my special
favorite "struct".

Again, all we can do is guess.

>
> --js
>
> [1]
> https://gitlab.com/jsnow/qemu/-/blob/553032db17440f8de011390e5a1cfddd13751b0b/scripts/qmp/qom-set#L66