Re: [PATCH v2 2/4] iotests: Make qemu_nbd_popen() a contextmanager

[Eric Blake <eblake@redhat.com>]

On 7/28/20 8:42 AM, Vladimir Sementsov-Ogievskiy wrote:
> 28.07.2020 00:58, Nir Soffer wrote:
>> Instead of duplicating the code to wait until the server is ready and
>> remember to terminate the server and wait for it, make it possible to
>> use like this:
>>
>>      with qemu_nbd_popen('-k', sock, image):
>>          # Access image via qemu-nbd socket...
>>
>> Only test 264 used this helper, but I had to modify the output since it
>> did not consistently when starting and stopping qemu-nbd.
>>
>> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
>> ---

>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -28,10 +28,13 @@ import signal
>>   import struct
>>   import subprocess
>>   import sys
>> +import time
>>   from typing import (Any, Callable, Dict, Iterable,
>>                       List, Optional, Sequence, Tuple, TypeVar)
>>   import unittest
>> +from contextlib import contextmanager
>> +
>>   # pylint: disable=import-error, wrong-import-position
>>   sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 
>> 'python'))
>>   from qemu import qtest
>> @@ -270,9 +273,30 @@ def qemu_nbd_early_pipe(*args):
>>       return subp.returncode, output if subp.returncode else ''
>> +@contextmanager
>>   def qemu_nbd_popen(*args):
>> -    '''Run qemu-nbd in daemon mode and return the parent's exit code'''
>> -    return subprocess.Popen(qemu_nbd_args + ['--persistent'] + 
>> list(args))
>> +    '''Context manager running qemu-nbd within the context'''
> 
> PEP8 (or some another PEP referenced in PEP8) asks to use """ for 
> doc-strings

But at least './check 297' still passes, so we are consistent enough 
with our current set of syntax checks to be acceptable.

> 
>> +    pid_file = file_path("pid")
>> +
>> +    cmd = list(qemu_nbd_args)
>> +    cmd.extend(('--persistent', '--pid-file', pid_file))
>> +    cmd.extend(args)
>> +
>> +    log('Start NBD server')
>> +    p = subprocess.Popen(cmd)
>> +    try:
>> +        while not os.path.exists(pid_file):
>> +            if p.poll() is not None:
>> +                raise RuntimeError(
>> +                    "qemu-nbd terminated with exit code {}: {}"
>> +                    .format(p.returncode, ' '.join(cmd)))
>> +
>> +            time.sleep(0.01)
>> +        yield
>> +    finally:
>> +        log('Kill NBD server')
>> +        p.kill()
>> +        p.wait()
> 
> why do we need try-finally? I think, the only possible exception is your 
> "raise RuntimeError", and in this case the process is alredy dead, no 
> need to kill it (and print the log message)
> 
>>   def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
>>       '''Return True if two image files are identical'''
>>
> 
> anyway:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 

Thanks; I trust your python review more than mine.  Queuing for -rc2.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org