[v2,5/6] iotests: implement QemuIoInteractive class

Message ID 20171207155102.66622-6-vsementsov@virtuozzo.com
State New
Headers show
Series
  • nbd export qmp interface
Related show

Commit Message

Vladimir Sementsov-Ogievskiy Dec. 7, 2017, 3:51 p.m.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/iotests.py | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

Comments

Eric Blake Jan. 9, 2018, 8:34 p.m. | #1
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 <vsementsov@virtuozzo.com>
> ---
>  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?
Vladimir Sementsov-Ogievskiy Jan. 12, 2018, 11:56 a.m. | #2
09.01.2018 23:34, Eric Blake wrote:
> 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 <vsementsov@virtuozzo.com>
>> ---
>>   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?

this special flag means: "send subprocess stderr to the same place as 
stdout", so,
I'll have both stdout and stderr in one .PIPE. I don't print these 
outputs, but return
them to the user.

>
>
>> +    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?

The only usage for now is a test in the following patch. I think assert 
is OK for now,
we can change it in future if needed.

>
>> +            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?

It seems more usual for such function to return string, not a list. 
Without join here,
I'll have to join in caller.

>
Eric Blake Jan. 12, 2018, 4:48 p.m. | #3
On 01/12/2018 05:56 AM, Vladimir Sementsov-Ogievskiy wrote:

>> 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?
> 
> this special flag means: "send subprocess stderr to the same place as
> stdout", so,
> I'll have both stdout and stderr in one .PIPE. I don't print these
> outputs, but return
> them to the user.

Okay, makes sense (my python inexperience is showing ;)

I'd welcome a review from anyone else, but since the resulting test
passes, I'm not opposed to taking the patch as-is.  If it goes through
my NBD queue, it will get my S-o-b (because I'll have to touch it); but
if it goes through anyone else's queue, that maintainer can add this
(weaker than R-b, but at least says I'm okay with it):

Acked-by: Eric Blake <eblake@redhat.com>

Patch

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6f057904a9..a9f312db83 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -93,6 +93,44 @@  def qemu_io(*args):
         sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' '.join(args)))
     return subp.communicate()[0]
 
+
+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)
+        assert self._p.stdout.read(9) == 'qemu-io> '
+
+    def close(self):
+        self._p.communicate('q\n')
+
+    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
+            assert c != ''
+            s.append(c)
+            if c == pattern[pos]:
+                pos += 1
+            else:
+                pos = 0
+
+        return ''.join(s[:-n])
+
+    def cmd(self, cmd):
+        # quit command is in close(), '\n' is added automatically
+        assert '\n' not in cmd
+        cmd = cmd.strip()
+        assert cmd != 'q' and cmd != 'quit'
+        self._p.stdin.write(cmd + '\n')
+        return self._read_output()
+
+
 def qemu_nbd(*args):
     '''Run qemu-nbd in daemon mode and return the parent's exit code'''
     return subprocess.call(qemu_nbd_args + ['--fork'] + list(args))