QEMU versions prior to the "oob" capability *also* can't accept the
"enable" keyword argument at all. Fix the handshake process with older
QEMU versions.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
---
 python/qemu/aqmp/qmp_client.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
index f1a845cc82..90a8737f03 100644
--- a/python/qemu/aqmp/qmp_client.py
+++ b/python/qemu/aqmp/qmp_client.py
@@ -292,9 +292,9 @@ async def _negotiate(self) -> None:
         """
         self.logger.debug("Negotiating capabilities ...")
 
-        arguments: Dict[str, List[str]] = {'enable': []}
+        arguments: Dict[str, List[str]] = {}
         if self._greeting and 'oob' in self._greeting.QMP.capabilities:
-            arguments['enable'].append('oob')
+            arguments.setdefault('enable', []).append('oob')
         msg = self.make_execute_msg('qmp_capabilities', arguments=arguments)
 
         # It's not safe to use execute() here, because the reader/writers
-- 
2.31.1

Am 01.02.2022 um 05:11 hat John Snow geschrieben:
> QEMU versions prior to the "oob" capability *also* can't accept the
> "enable" keyword argument at all. Fix the handshake process with older
> QEMU versions.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  python/qemu/aqmp/qmp_client.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
> index f1a845cc82..90a8737f03 100644
> --- a/python/qemu/aqmp/qmp_client.py
> +++ b/python/qemu/aqmp/qmp_client.py
> @@ -292,9 +292,9 @@ async def _negotiate(self) -> None:
>          """
>          self.logger.debug("Negotiating capabilities ...")
>  
> -        arguments: Dict[str, List[str]] = {'enable': []}
> +        arguments: Dict[str, List[str]] = {}
>          if self._greeting and 'oob' in self._greeting.QMP.capabilities:
> -            arguments['enable'].append('oob')
> +            arguments.setdefault('enable', []).append('oob')
>          msg = self.make_execute_msg('qmp_capabilities', arguments=arguments)

In case you have some interest in bike sheds:

As long as we only ever append a single capability, it doesn't really
make a difference and an explicit setdefault() when adding it is fine.
But if we had more than one, maybe making arguments a defaultdict(list)
would be nicer.

Not worth respinning, of course, if you don't for another reason.

Kevin

On Tue, Feb 1, 2022 at 7:59 AM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 01.02.2022 um 05:11 hat John Snow geschrieben:
> > QEMU versions prior to the "oob" capability *also* can't accept the
> > "enable" keyword argument at all. Fix the handshake process with older
> > QEMU versions.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > Reviewed-by: Hanna Reitz <hreitz@redhat.com>
> > ---
> >  python/qemu/aqmp/qmp_client.py | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
> > index f1a845cc82..90a8737f03 100644
> > --- a/python/qemu/aqmp/qmp_client.py
> > +++ b/python/qemu/aqmp/qmp_client.py
> > @@ -292,9 +292,9 @@ async def _negotiate(self) -> None:
> >          """
> >          self.logger.debug("Negotiating capabilities ...")
> >
> > -        arguments: Dict[str, List[str]] = {'enable': []}
> > +        arguments: Dict[str, List[str]] = {}
> >          if self._greeting and 'oob' in self._greeting.QMP.capabilities:
> > -            arguments['enable'].append('oob')
> > +            arguments.setdefault('enable', []).append('oob')
> >          msg = self.make_execute_msg('qmp_capabilities', arguments=arguments)
>
> In case you have some interest in bike sheds:
>
> As long as we only ever append a single capability, it doesn't really
> make a difference and an explicit setdefault() when adding it is fine.
> But if we had more than one, maybe making arguments a defaultdict(list)
> would be nicer.
>
> Not worth respinning, of course, if you don't for another reason.
>
> Kevin
>

Nope, no reason. I just forget that there's a fancier doodad. I'll add
a patch to a less-important series to shine this up.

--js