Re: [PATCH] tests/avocado/hotplug_blk: Fix addr in device_add command

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> pci_devfn properties accept both integer and string values, but
> integer 1 and string '1' have different meanings: The integer value
> means device 0, function 1 whereas the string value '1' is short for
> '1.0' and means device 1, function 0.

This is a terrible interface.  Goes back to

commit 768a9ebe188bd0a6172a9a4e64777d21fff7f014
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Thu Feb 9 09:53:32 2012 +0100

    qdev: accept both strings and integers for PCI addresses
    
    Visitors allow a limited form of polymorphism.  Exploit it to support
    setting the non-legacy PCI address property both as a DD.F string
    and as an 8-bit integer.
    
Less than clear.  Before the patch, only strings where accepted.
Afterwards, integers are accepted, too.

    The 8-bit integer form is just too clumsy, it is unlikely that we will
    ever drop it.

No idea what that means.
    
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> This test wants the string version so that the device actually becomes
> visible for the guest. device_add hides the problem because it goes
> through QemuOpts, which turns all properties into strings - this is a
> QEMU bug that we want to fix, but that cancelled out the bug in this
> test.

The integer half of the terrible interface is inaccessible with
device_add.  This is fixable for QMP device_add: avoid the ill-advised
detour that turns all values into strings.

It will remain inaccessible with HMP device_add and -device with dotted
key arguments: scalar values can only be strings there.

> Fix the test first so that device_add can be fixed afterwards.

Your patch fixes a misuse of the terrible interface.

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/avocado/hotplug_blk.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/avocado/hotplug_blk.py b/tests/avocado/hotplug_blk.py
> index d55ded1c1d..b36bca02ec 100644
> --- a/tests/avocado/hotplug_blk.py
> +++ b/tests/avocado/hotplug_blk.py
> @@ -33,7 +33,7 @@ def plug(self) -> None:
>              'drive': 'disk',
>              'id': 'virtio-disk0',
>              'bus': 'pci.1',
> -            'addr': 1
> +            'addr': '1',
>          }
>  
>          self.assert_no_vda()

Reviewed-by: Markus Armbruster <armbru@redhat.com>