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

[Eric Blake]

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