[Top][All Lists]

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

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

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-block] [PATCH v2 5/6] iotests: implement QemuIoInteractive class
Date: Fri, 12 Jan 2018 14:56:13 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.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 <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?

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.

Best regards,

reply via email to

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