[PATCH v4 00/16] python: add mypy support to python/qemu

John Snow posted 16 patches 3 years, 9 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
python/qemu/accel.py          |   8 +-
python/qemu/machine.py        | 294 ++++++++++++++++++++--------------
python/qemu/qmp.py            | 111 +++++++++----
python/qemu/qtest.py          |  53 +++---
scripts/render_block_graph.py |   7 +-
tests/qemu-iotests/iotests.py |  11 +-
6 files changed, 300 insertions(+), 184 deletions(-)
[PATCH v4 00/16] python: add mypy support to python/qemu
Posted by John Snow 3 years, 9 months ago
Based-on: 20200626202350.11060-1-jsnow@redhat.com

This series modifies the python/qemu library to comply with mypy --strict.
This requires my "refactor shutdown" patch as a pre-requisite.

v4:
001/16:[----] [--] 'python/qmp.py: Define common types'
002/16:[----] [--] 'iotests.py: use qemu.qmp type aliases'
003/16:[----] [--] 'python/qmp.py: re-absorb MonitorResponseError'
004/16:[----] [--] 'python/qmp.py: Do not return None from cmd_obj'
005/16:[----] [--] 'python/qmp.py: add casts to JSON deserialization'
006/16:[----] [--] 'python/qmp.py: add QMPProtocolError'
007/16:[----] [--] 'python/machine.py: Fix monitor address typing'
008/16:[----] [--] 'python/machine.py: reorder __init__'
009/16:[----] [--] 'python/machine.py: Don't modify state in _base_args()'
010/16:[----] [--] 'python/machine.py: Handle None events in events_wait'
011/16:[----] [--] 'python/machine.py: use qmp.command'
012/16:[0010] [FC] 'python/machine.py: Add _qmp access shim'
013/16:[----] [-C] 'python/machine.py: fix _popen access'
014/16:[----] [--] 'python/qemu: make 'args' style arguments immutable'
015/16:[----] [--] 'iotests.py: Adjust HMP kwargs typing'
016/16:[----] [-C] 'python/qemu: Add mypy type annotations'

 - Rebased on "refactor shutdown" v4
 - Fixed _qmp access for scripts that disable QMP

v3:
005: Removed a cast, per Kevin Wolf's tip
010: Renamed with correct function name;
     Rewrote docstring and added comments
016: Use SocketAddrT instead of Union[Tuple[str,str],str]

"v2":
- This version supports iotests 297
- Many patches merged by Phil are removed
- Replaces iotests.py type aliases with centralized ones
  (See patch 2)
- Imports etc are reworked to use the non-installable
  package layout instead. (Mostly important for patch 3)

Testing this out:
- You'll need Python3.6+
- I encourage you to use a virtual environment!
- You don't necessarily need these exact versions, but I didn't test the
  lower bounds, use older versions at your peril:
  - pylint==2.5.0
  - mypy=0.770
  - flake8=3.7.8

> cd ~/src/qemu/python/
> flake8 qemu
> mypy --strict qemu
> cd qemu
> pylint *.py

These should all 100% pass.

---

Open RFC: What's the right way to hook this into make check to prevent
regressions?

John Snow (16):
  python/qmp.py: Define common types
  iotests.py: use qemu.qmp type aliases
  python/qmp.py: re-absorb MonitorResponseError
  python/qmp.py: Do not return None from cmd_obj
  python/qmp.py: add casts to JSON deserialization
  python/qmp.py: add QMPProtocolError
  python/machine.py: Fix monitor address typing
  python/machine.py: reorder __init__
  python/machine.py: Don't modify state in _base_args()
  python/machine.py: Handle None events in events_wait
  python/machine.py: use qmp.command
  python/machine.py: Add _qmp access shim
  python/machine.py: fix _popen access
  python/qemu: make 'args' style arguments immutable
  iotests.py: Adjust HMP kwargs typing
  python/qemu: Add mypy type annotations

 python/qemu/accel.py          |   8 +-
 python/qemu/machine.py        | 294 ++++++++++++++++++++--------------
 python/qemu/qmp.py            | 111 +++++++++----
 python/qemu/qtest.py          |  53 +++---
 scripts/render_block_graph.py |   7 +-
 tests/qemu-iotests/iotests.py |  11 +-
 6 files changed, 300 insertions(+), 184 deletions(-)

-- 
2.21.3


Re: [PATCH v4 00/16] python: add mypy support to python/qemu
Posted by Philippe Mathieu-Daudé 3 years, 9 months ago
+Cleber/Wainer.

On 6/26/20 10:41 PM, John Snow wrote:
> Based-on: 20200626202350.11060-1-jsnow@redhat.com
> 
> This series modifies the python/qemu library to comply with mypy --strict.
> This requires my "refactor shutdown" patch as a pre-requisite.
> 
> v4:
> 001/16:[----] [--] 'python/qmp.py: Define common types'
> 002/16:[----] [--] 'iotests.py: use qemu.qmp type aliases'
> 003/16:[----] [--] 'python/qmp.py: re-absorb MonitorResponseError'
> 004/16:[----] [--] 'python/qmp.py: Do not return None from cmd_obj'
> 005/16:[----] [--] 'python/qmp.py: add casts to JSON deserialization'
> 006/16:[----] [--] 'python/qmp.py: add QMPProtocolError'
> 007/16:[----] [--] 'python/machine.py: Fix monitor address typing'
> 008/16:[----] [--] 'python/machine.py: reorder __init__'
> 009/16:[----] [--] 'python/machine.py: Don't modify state in _base_args()'
> 010/16:[----] [--] 'python/machine.py: Handle None events in events_wait'
> 011/16:[----] [--] 'python/machine.py: use qmp.command'
> 012/16:[0010] [FC] 'python/machine.py: Add _qmp access shim'
> 013/16:[----] [-C] 'python/machine.py: fix _popen access'
> 014/16:[----] [--] 'python/qemu: make 'args' style arguments immutable'
> 015/16:[----] [--] 'iotests.py: Adjust HMP kwargs typing'
> 016/16:[----] [-C] 'python/qemu: Add mypy type annotations'
> 
>  - Rebased on "refactor shutdown" v4
>  - Fixed _qmp access for scripts that disable QMP

See:

https://travis-ci.org/github/philmd/qemu/jobs/702507625#L5439

/ # uname -a
Linux buildroot 4.5.0-2-4kc-malta #1 Debian 4.5.5-1 (2016-05-29) mips
GNU/Linux
/ # reboot
/ # reboot: Restarting system
>>> {'execute': 'quit'}
qemu received signal 9; command: "mips-softmmu/qemu-system-mips -display
none -vga none -chardev
socket,id=mon,path=/tmp/tmpcojsc5u3/qemu-14540-monitor.sock -mon
chardev=mon,mode=control -machine malta -chardev
socket,id=console,path=/tmp/tmpcojsc5u3/qemu-14540-console.sock,server,nowait
-serial chardev:console -kernel
/tmp/avocado_xgmck2k5/avocado_job_83wkzs2f/12-tests_acceptance_boot_linux_console.py_BootLinuxConsole.test_mips_malta_cpio/boot/vmlinux-4.5.0-2-4kc-malta
-initrd
/tmp/avocado_xgmck2k5/avocado_job_83wkzs2f/12-tests_acceptance_boot_linux_console.py_BootLinuxConsole.test_mips_malta_cpiorootfs.cpio
-append printk.time=0 console=ttyS0 console=tty rdinit=/sbin/init
noreboot -no-reboot"

Reproduced traceback from:
/home/travis/build/philmd/qemu/build/tests/venv/lib/python3.6/site-packages/avocado/core/test.py:886
Traceback (most recent call last):
  File
"/home/travis/build/philmd/qemu/build/tests/acceptance/avocado_qemu/__init__.py",
line 195, in tearDown
    vm.shutdown()
  File "/home/travis/build/philmd/qemu/python/qemu/machine.py", line
457, in shutdown
    self._do_shutdown(has_quit)
  File "/home/travis/build/philmd/qemu/python/qemu/machine.py", line
434, in _do_shutdown
    self._soft_shutdown(has_quit, timeout)
  File "/home/travis/build/philmd/qemu/python/qemu/machine.py", line
414, in _soft_shutdown
    self._qmp.cmd('quit')
  File "/home/travis/build/philmd/qemu/python/qemu/qmp.py", line 271, in cmd
    return self.cmd_obj(qmp_cmd)
  File "/home/travis/build/philmd/qemu/python/qemu/qmp.py", line 249, in
cmd_obj
    self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8'))
BrokenPipeError: [Errno 32] Broken pipe


Re: [PATCH v4 00/16] python: add mypy support to python/qemu
Posted by John Snow 3 years, 9 months ago

On 6/29/20 4:26 AM, Philippe Mathieu-Daudé wrote:
> +Cleber/Wainer.
> 
> On 6/26/20 10:41 PM, John Snow wrote:
>> Based-on: 20200626202350.11060-1-jsnow@redhat.com
>>
>> This series modifies the python/qemu library to comply with mypy --strict.
>> This requires my "refactor shutdown" patch as a pre-requisite.
>>
>> v4:
>> 001/16:[----] [--] 'python/qmp.py: Define common types'
>> 002/16:[----] [--] 'iotests.py: use qemu.qmp type aliases'
>> 003/16:[----] [--] 'python/qmp.py: re-absorb MonitorResponseError'
>> 004/16:[----] [--] 'python/qmp.py: Do not return None from cmd_obj'
>> 005/16:[----] [--] 'python/qmp.py: add casts to JSON deserialization'
>> 006/16:[----] [--] 'python/qmp.py: add QMPProtocolError'
>> 007/16:[----] [--] 'python/machine.py: Fix monitor address typing'
>> 008/16:[----] [--] 'python/machine.py: reorder __init__'
>> 009/16:[----] [--] 'python/machine.py: Don't modify state in _base_args()'
>> 010/16:[----] [--] 'python/machine.py: Handle None events in events_wait'
>> 011/16:[----] [--] 'python/machine.py: use qmp.command'
>> 012/16:[0010] [FC] 'python/machine.py: Add _qmp access shim'
>> 013/16:[----] [-C] 'python/machine.py: fix _popen access'
>> 014/16:[----] [--] 'python/qemu: make 'args' style arguments immutable'
>> 015/16:[----] [--] 'iotests.py: Adjust HMP kwargs typing'
>> 016/16:[----] [-C] 'python/qemu: Add mypy type annotations'
>>
>>  - Rebased on "refactor shutdown" v4
>>  - Fixed _qmp access for scripts that disable QMP
> 
> See:
> 
> https://travis-ci.org/github/philmd/qemu/jobs/702507625#L5439
> 
> / # uname -a
> Linux buildroot 4.5.0-2-4kc-malta #1 Debian 4.5.5-1 (2016-05-29) mips
> GNU/Linux
> / # reboot
> / # reboot: Restarting system
>>>> {'execute': 'quit'}
> qemu received signal 9; command: "mips-softmmu/qemu-system-mips -display
> none -vga none -chardev
> socket,id=mon,path=/tmp/tmpcojsc5u3/qemu-14540-monitor.sock -mon
> chardev=mon,mode=control -machine malta -chardev
> socket,id=console,path=/tmp/tmpcojsc5u3/qemu-14540-console.sock,server,nowait
> -serial chardev:console -kernel
> /tmp/avocado_xgmck2k5/avocado_job_83wkzs2f/12-tests_acceptance_boot_linux_console.py_BootLinuxConsole.test_mips_malta_cpio/boot/vmlinux-4.5.0-2-4kc-malta
> -initrd
> /tmp/avocado_xgmck2k5/avocado_job_83wkzs2f/12-tests_acceptance_boot_linux_console.py_BootLinuxConsole.test_mips_malta_cpiorootfs.cpio
> -append printk.time=0 console=ttyS0 console=tty rdinit=/sbin/init
> noreboot -no-reboot"
> 
> Reproduced traceback from:
> /home/travis/build/philmd/qemu/build/tests/venv/lib/python3.6/site-packages/avocado/core/test.py:886
> Traceback (most recent call last):
>   File
> "/home/travis/build/philmd/qemu/build/tests/acceptance/avocado_qemu/__init__.py",
> line 195, in tearDown
>     vm.shutdown()
>   File "/home/travis/build/philmd/qemu/python/qemu/machine.py", line
> 457, in shutdown
>     self._do_shutdown(has_quit)
>   File "/home/travis/build/philmd/qemu/python/qemu/machine.py", line
> 434, in _do_shutdown
>     self._soft_shutdown(has_quit, timeout)
>   File "/home/travis/build/philmd/qemu/python/qemu/machine.py", line
> 414, in _soft_shutdown
>     self._qmp.cmd('quit')
>   File "/home/travis/build/philmd/qemu/python/qemu/qmp.py", line 271, in cmd
>     return self.cmd_obj(qmp_cmd)
>   File "/home/travis/build/philmd/qemu/python/qemu/qmp.py", line 249, in
> cmd_obj
>     self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8'))
> BrokenPipeError: [Errno 32] Broken pipe
> 

Might be racing between QMP going away and Python not being aware that
the process has closed yet. It's the only explanation I have left.

I wish I could reproduce this, though. When I submit jobs to Travis I am
not seeing these failures.

I'll see what I can do, but at this point I'll not re-send the patch
series until I can conclusively fix your build testing.

--js


Re: [PATCH v4 00/16] python: add mypy support to python/qemu
Posted by John Snow 3 years, 9 months ago

On 6/29/20 10:30 AM, John Snow wrote:
> 
> 
> On 6/29/20 4:26 AM, Philippe Mathieu-Daudé wrote:
>> +Cleber/Wainer.
>>
>> On 6/26/20 10:41 PM, John Snow wrote:
>>> Based-on: 20200626202350.11060-1-jsnow@redhat.com
>>>
>>> This series modifies the python/qemu library to comply with mypy --strict.
>>> This requires my "refactor shutdown" patch as a pre-requisite.
>>>
>>> v4:
>>> 001/16:[----] [--] 'python/qmp.py: Define common types'
>>> 002/16:[----] [--] 'iotests.py: use qemu.qmp type aliases'
>>> 003/16:[----] [--] 'python/qmp.py: re-absorb MonitorResponseError'
>>> 004/16:[----] [--] 'python/qmp.py: Do not return None from cmd_obj'
>>> 005/16:[----] [--] 'python/qmp.py: add casts to JSON deserialization'
>>> 006/16:[----] [--] 'python/qmp.py: add QMPProtocolError'
>>> 007/16:[----] [--] 'python/machine.py: Fix monitor address typing'
>>> 008/16:[----] [--] 'python/machine.py: reorder __init__'
>>> 009/16:[----] [--] 'python/machine.py: Don't modify state in _base_args()'
>>> 010/16:[----] [--] 'python/machine.py: Handle None events in events_wait'
>>> 011/16:[----] [--] 'python/machine.py: use qmp.command'
>>> 012/16:[0010] [FC] 'python/machine.py: Add _qmp access shim'
>>> 013/16:[----] [-C] 'python/machine.py: fix _popen access'
>>> 014/16:[----] [--] 'python/qemu: make 'args' style arguments immutable'
>>> 015/16:[----] [--] 'iotests.py: Adjust HMP kwargs typing'
>>> 016/16:[----] [-C] 'python/qemu: Add mypy type annotations'
>>>
>>>  - Rebased on "refactor shutdown" v4
>>>  - Fixed _qmp access for scripts that disable QMP
>>
>> See:
>>
>> https://travis-ci.org/github/philmd/qemu/jobs/702507625#L5439
>>
>> / # uname -a
>> Linux buildroot 4.5.0-2-4kc-malta #1 Debian 4.5.5-1 (2016-05-29) mips
>> GNU/Linux
>> / # reboot
>> / # reboot: Restarting system
>>>>> {'execute': 'quit'}
>> qemu received signal 9; command: "mips-softmmu/qemu-system-mips -display
>> none -vga none -chardev
>> socket,id=mon,path=/tmp/tmpcojsc5u3/qemu-14540-monitor.sock -mon
>> chardev=mon,mode=control -machine malta -chardev
>> socket,id=console,path=/tmp/tmpcojsc5u3/qemu-14540-console.sock,server,nowait
>> -serial chardev:console -kernel
>> /tmp/avocado_xgmck2k5/avocado_job_83wkzs2f/12-tests_acceptance_boot_linux_console.py_BootLinuxConsole.test_mips_malta_cpio/boot/vmlinux-4.5.0-2-4kc-malta
>> -initrd
>> /tmp/avocado_xgmck2k5/avocado_job_83wkzs2f/12-tests_acceptance_boot_linux_console.py_BootLinuxConsole.test_mips_malta_cpiorootfs.cpio
>> -append printk.time=0 console=ttyS0 console=tty rdinit=/sbin/init
>> noreboot -no-reboot"
>>
>> Reproduced traceback from:
>> /home/travis/build/philmd/qemu/build/tests/venv/lib/python3.6/site-packages/avocado/core/test.py:886
>> Traceback (most recent call last):
>>   File
>> "/home/travis/build/philmd/qemu/build/tests/acceptance/avocado_qemu/__init__.py",
>> line 195, in tearDown
>>     vm.shutdown()
>>   File "/home/travis/build/philmd/qemu/python/qemu/machine.py", line
>> 457, in shutdown
>>     self._do_shutdown(has_quit)
>>   File "/home/travis/build/philmd/qemu/python/qemu/machine.py", line
>> 434, in _do_shutdown
>>     self._soft_shutdown(has_quit, timeout)
>>   File "/home/travis/build/philmd/qemu/python/qemu/machine.py", line
>> 414, in _soft_shutdown
>>     self._qmp.cmd('quit')
>>   File "/home/travis/build/philmd/qemu/python/qemu/qmp.py", line 271, in cmd
>>     return self.cmd_obj(qmp_cmd)
>>   File "/home/travis/build/philmd/qemu/python/qemu/qmp.py", line 249, in
>> cmd_obj
>>     self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8'))
>> BrokenPipeError: [Errno 32] Broken pipe
>>
> 
> Might be racing between QMP going away and Python not being aware that
> the process has closed yet. It's the only explanation I have left.
> 
> I wish I could reproduce this, though. When I submit jobs to Travis I am
> not seeing these failures.
> 
> I'll see what I can do, but at this point I'll not re-send the patch
> series until I can conclusively fix your build testing.
> 
> --js
> 

OK, this is a very big mea culpa. There are two problems.

1. I should catch ConnectionReset and not ConnectionResetError; one is a
catch-all for socket problems and the other is a very specific errno
that does not include BrokenPipeError.

2. Even if I do, it can still race, actually. QEMU might be in the
middle of shutting down, but has already lost the control channel.

Solving the second problem as the interface is currently designed is
hard. You can wait, but then if the wait failed, you need to re-raise
the control channel error instead of the wait failure. IOW, you need to
wait in order to learn if the control channel error is "important" or not.

So, the architecture of this is starting to look wrong; _soft_shutdown
should keep clarity of purpose. Either it was able to to a nice, soft
shutdown -- or it wasn't.

I'm starting to think that the real problem is that machine.py shouldn't
try to hide connection errors on shutdown at all if we expected to be
able to issue a quit command.

Changing my mind rapidly that the right thing to do is to actually just
fix the test to understand that it should not try to issue a shutdown
command, but instead issue a wait command; and removing the race
condition from machine.py by simply reporting *all* shutdown errors.

(If callers expect shutdown errors, they can -- and should -- catch
those exceptions as desired!)

--js