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

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

[Kevin Wolf]

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

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

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

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>

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

[Daniel P. Berrangé]

On Fri, Nov 22, 2024 at 11:40:42PM +0100, Kevin Wolf wrote:
> 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 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.
> 
> Fix the test first so that device_add can be fixed afterwards.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/avocado/hotplug_blk.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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