Re: [PATCH] iotests.py: Do not wait() before communicate()

2020-07-03 Thread Kevin Wolf
Am 30.06.2020 um 10:37 hat Max Reitz geschrieben:
> Waiting on a process for which we have a pipe will stall if the process
> outputs more data than fits into the OS-provided buffer.  We must use
> communicate() before wait(), and in fact, communicate() perfectly
> replaces wait() already.
> 
> We have to drop the stderr=subprocess.STDOUT parameter from
> subprocess.Popen() in qemu_nbd_early_pipe(), because stderr is passed on
> to the child process, so if we do not drop this parameter, communicate()
> will hang (because the pipe is not closed).
> 
> Signed-off-by: Max Reitz 

Thanks, applied to the block branch.

Kevin




[PATCH] iotests.py: Do not wait() before communicate()

2020-06-30 Thread Max Reitz
Waiting on a process for which we have a pipe will stall if the process
outputs more data than fits into the OS-provided buffer.  We must use
communicate() before wait(), and in fact, communicate() perfectly
replaces wait() already.

We have to drop the stderr=subprocess.STDOUT parameter from
subprocess.Popen() in qemu_nbd_early_pipe(), because stderr is passed on
to the child process, so if we do not drop this parameter, communicate()
will hang (because the pipe is not closed).

Signed-off-by: Max Reitz 
---
I hit this at some point with some test when writing my dirty bitmap
migration mapping series, but I can't find the test I had problems with
any more (at least not on master).
Either way, I still think this is the right thing to do.
---
 tests/qemu-iotests/iotests.py | 34 +-
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 5ea4c4df8b..ef739dd1e3 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -146,11 +146,12 @@ def qemu_img_pipe(*args):
 stdout=subprocess.PIPE,
 stderr=subprocess.STDOUT,
 universal_newlines=True)
-exitcode = subp.wait()
-if exitcode < 0:
+output = subp.communicate()[0]
+if subp.returncode < 0:
 sys.stderr.write('qemu-img received signal %i: %s\n'
- % (-exitcode, ' '.join(qemu_img_args + list(args
-return subp.communicate()[0]
+ % (-subp.returncode,
+' '.join(qemu_img_args + list(args
+return output
 
 def qemu_img_log(*args):
 result = qemu_img_pipe(*args)
@@ -177,11 +178,11 @@ def qemu_io(*args):
 subp = subprocess.Popen(args, stdout=subprocess.PIPE,
 stderr=subprocess.STDOUT,
 universal_newlines=True)
-exitcode = subp.wait()
-if exitcode < 0:
+output = subp.communicate()[0]
+if subp.returncode < 0:
 sys.stderr.write('qemu-io received signal %i: %s\n'
- % (-exitcode, ' '.join(args)))
-return subp.communicate()[0]
+ % (-subp.returncode, ' '.join(args)))
+return output
 
 def qemu_io_log(*args):
 result = qemu_io(*args)
@@ -257,15 +258,14 @@ def qemu_nbd_early_pipe(*args):
and its output in case of an error'''
 subp = subprocess.Popen(qemu_nbd_args + ['--fork'] + list(args),
 stdout=subprocess.PIPE,
-stderr=subprocess.STDOUT,
 universal_newlines=True)
-exitcode = subp.wait()
-if exitcode < 0:
+output = subp.communicate()[0]
+if subp.returncode < 0:
 sys.stderr.write('qemu-nbd received signal %i: %s\n' %
- (-exitcode,
+ (-subp.returncode,
   ' '.join(qemu_nbd_args + ['--fork'] + list(args
 
-return exitcode, subp.communicate()[0] if exitcode else ''
+return subp.returncode, output if subp.returncode else ''
 
 def qemu_nbd_popen(*args):
 '''Run qemu-nbd in daemon mode and return the parent's exit code'''
@@ -1062,11 +1062,11 @@ def qemu_pipe(*args):
 subp = subprocess.Popen(args, stdout=subprocess.PIPE,
 stderr=subprocess.STDOUT,
 universal_newlines=True)
-exitcode = subp.wait()
-if exitcode < 0:
+output = subp.communicate()[0]
+if subp.returncode < 0:
 sys.stderr.write('qemu received signal %i: %s\n' %
- (-exitcode, ' '.join(args)))
-return subp.communicate()[0]
+ (-subp.returncode, ' '.join(args)))
+return output
 
 def supported_formats(read_only=False):
 '''Set 'read_only' to True to check ro-whitelist
-- 
2.26.2