qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 07/25] python/qmp: switch qemu-ga-client to AQMP


From: John Snow
Subject: Re: [PATCH v2 07/25] python/qmp: switch qemu-ga-client to AQMP
Date: Thu, 16 Dec 2021 12:43:12 -0500



On Thu, Dec 16, 2021 at 5:31 AM Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
15.12.2021 22:39, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>

Not simple to check, how much new behavior is equal to the old one.. And impossible to check, is everything updated that should be )


Agree, it's hard to tell. Mostly: iotests has been using the new interface for the 6.2 release testing cycle and we didn't find many bugs, so I'm assuming we can largely switch these, too.

Big assumption! but, I actually doubt people use this script much ...
 

> ---
>   python/qemu/qmp/qemu_ga_client.py | 22 +++++++++++-----------
>   1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/python/qemu/qmp/qemu_ga_client.py b/python/qemu/qmp/qemu_ga_client.py
> index b3e1d98c9e..15ed430c61 100644
> --- a/python/qemu/qmp/qemu_ga_client.py
> +++ b/python/qemu/qmp/qemu_ga_client.py

[..]

>   
>       try:
>           client = QemuGuestAgentClient(address)
> -    except OSError as err:
> +    except ConnectError as err:
>           print(err)
> -        if err.errno == errno.ECONNREFUSED:
> -            print('Hint: qemu is not running?')
> +        if isinstance(err.exc, ConnectionError):
> +            print('(Is QEMU running?)')

It at least a bit changed from checking errno to checking the class, I'd note it in commit message. And anyway commit message may be more informative. Still, I just don't care too much about testing framework.


OK, I'll update the commit message. The problem is that the backing errors originally emitted by a blocking socket may no longer be exactly those raised by the async core, so it's not 1:1.

In this case, the new QMP library always raises qemu.[a]qmp.ConnectError, and that houses a backing exception with the "real reason". that "real reason" may be the built-in ConnectionError, which just indicates some kind of connection problem, including ECONNREFUSED. So the scope of this exception has expanded a little.
 
Nothing seems wrong to me, so weak:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

--
Best regards,
Vladimir


reply via email to

[Prev in Thread] Current Thread [Next in Thread]