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

[John Snow]

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