qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v2 5/6] iotests: implement QemuIoInteractive cla


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v2 5/6] iotests: implement QemuIoInteractive class
Date: Tue, 9 Jan 2018 14:34:59 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0

On 12/07/2017 09:51 AM, Vladimir Sementsov-Ogievskiy wrote:

The subject says what, but there is no commit body that says why.

Presumably this makes writing the test in the next patch easier, but
some details in the commit message would make this obvious.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  tests/qemu-iotests/iotests.py | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 

My python is not strong; it looks good overall, although I have a few
questions that may warrant a v3 before I give R-b.

> +class QemuIoInteractive:
> +    def __init__(self, *args):
> +        self.args = qemu_io_args + list(args)
> +        self._p = subprocess.Popen(self.args, stdin=subprocess.PIPE,
> +                                   stdout=subprocess.PIPE,
> +                                   stderr=subprocess.STDOUT)

Why STDOUT instead of STDERR?  Is the redirection intentional?


> +    def _read_output(self):
> +        pattern = 'qemu-io> '
> +        n = len(pattern)
> +        pos = 0
> +        s = []
> +        while pos != n:
> +            c = self._p.stdout.read(1)
> +            #check unexpected EOF

pep8 doesn't like this comment style (it wants space after #).  The file
is already unclean under pep8, but you shouldn't make it worse.

> +            assert c != ''

Is assert really the nicest control flow when early EOF is present? Or
is it because we are only using this in tests, where we don't expect
early EOF, so asserting is just fine for our usage?

> +            s.append(c)
> +            if c == pattern[pos]:
> +                pos += 1
> +            else:
> +                pos = 0
> +
> +        return ''.join(s[:-n])

Is it necessary to use join() on the empty string here, compared to just
returning the array slice directly?

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

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

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