Message ID | 20210510190449.65948-1-eesposit@redhat.com |
---|---|
State | New |
Headers | show |
Series | [v2] qemu-iotests: fix pylint 2.8 consider-using-with error | expand |
On 10.05.21 21:04, Emanuele Giuseppe Esposito wrote: > pylint 2.8 introduces consider-using-with error, suggesting > to use the 'with' block statement when possible. > > Modify all subprocess.Popen call to use the 'with' statement, > except the one in __init__ of QemuIoInteractive class, since > it is assigned to a class field and used in other methods. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > v1 -> v2: > * fix indentation [Max] > * explain why we disabled the check in QemuIoInteractive's __init__ [Max] Thanks! Applied to my block branch: https://github.com/XanClic/qemu/commits/block Max
On 12.05.21 19:04, Max Reitz wrote: > On 10.05.21 21:04, Emanuele Giuseppe Esposito wrote: >> pylint 2.8 introduces consider-using-with error, suggesting >> to use the 'with' block statement when possible. >> >> Modify all subprocess.Popen call to use the 'with' statement, >> except the one in __init__ of QemuIoInteractive class, since >> it is assigned to a class field and used in other methods. >> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> --- >> v1 -> v2: >> * fix indentation [Max] >> * explain why we disabled the check in QemuIoInteractive's __init__ [Max] > > Thanks! > > Applied to my block branch: > > https://github.com/XanClic/qemu/commits/block I’ve just seen that the “# pylint: disable=consider-using-with” line causes a warning in pylint versions that don’t know that warning… I’d like to squash this in: > diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc > index 7a6c0a9474..f2c0b522ac 100644 > --- a/tests/qemu-iotests/pylintrc > +++ b/tests/qemu-iotests/pylintrc > @@ -19,6 +19,9 @@ disable=invalid-name, > too-many-public-methods, > # pylint warns about Optional[] etc. as unsubscriptable in 3.9 > unsubscriptable-object, > + # Sometimes we need to disable a newly introduced pylint warning. > + # Doing so should not produce a warning in older versions of pylint. > + bad-option-value, > # These are temporary, and should be removed: > missing-docstring, > too-many-return-statements, Would that be OK for you? Max
On 14/05/2021 16:09, Max Reitz wrote: > On 12.05.21 19:04, Max Reitz wrote: >> On 10.05.21 21:04, Emanuele Giuseppe Esposito wrote: >>> pylint 2.8 introduces consider-using-with error, suggesting >>> to use the 'with' block statement when possible. >>> >>> Modify all subprocess.Popen call to use the 'with' statement, >>> except the one in __init__ of QemuIoInteractive class, since >>> it is assigned to a class field and used in other methods. >>> >>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >>> --- >>> v1 -> v2: >>> * fix indentation [Max] >>> * explain why we disabled the check in QemuIoInteractive's __init__ >>> [Max] >> >> Thanks! >> >> Applied to my block branch: >> >> https://github.com/XanClic/qemu/commits/block > > I’ve just seen that the “# pylint: disable=consider-using-with” line > causes a warning in pylint versions that don’t know that warning… > > I’d like to squash this in: > >> diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc >> index 7a6c0a9474..f2c0b522ac 100644 >> --- a/tests/qemu-iotests/pylintrc >> +++ b/tests/qemu-iotests/pylintrc >> @@ -19,6 +19,9 @@ disable=invalid-name, >> too-many-public-methods, >> # pylint warns about Optional[] etc. as unsubscriptable in 3.9 >> unsubscriptable-object, >> + # Sometimes we need to disable a newly introduced pylint >> warning. >> + # Doing so should not produce a warning in older versions of >> pylint. >> + bad-option-value, >> # These are temporary, and should be removed: >> missing-docstring, >> too-many-return-statements, > > Would that be OK for you? I see... Looks good to me, this also prevents the same problem with other pylint options that can be introduced in the future. Thank you, Emanuele
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 5af0182895..ec3c69daf1 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -113,15 +113,14 @@ def qemu_tool_pipe_and_status(tool: str, args: Sequence[str], Run a tool and return both its output and its exit code """ stderr = subprocess.STDOUT if connect_stderr else None - subp = subprocess.Popen(args, - stdout=subprocess.PIPE, - stderr=stderr, - universal_newlines=True) - output = subp.communicate()[0] - if subp.returncode < 0: - cmd = ' '.join(args) - sys.stderr.write(f'{tool} received signal {-subp.returncode}: {cmd}\n') - return (output, subp.returncode) + with subprocess.Popen(args, stdout=subprocess.PIPE, + stderr=stderr, universal_newlines=True) as subp: + output = subp.communicate()[0] + if subp.returncode < 0: + cmd = ' '.join(args) + sys.stderr.write(f'{tool} received signal \ + {-subp.returncode}: {cmd}\n') + return (output, subp.returncode) def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]: """ @@ -237,6 +236,9 @@ def qemu_io_silent_check(*args): class QemuIoInteractive: def __init__(self, *args): self.args = qemu_io_args_no_fmt + list(args) + # We need to keep the Popen objext around, and not + # close it immediately. Therefore, disable the pylint check: + # pylint: disable=consider-using-with self._p = subprocess.Popen(self.args, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, @@ -310,22 +312,22 @@ def qemu_nbd_popen(*args): 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: - if os.path.exists(pid_file): - os.remove(pid_file) - log('Kill NBD server') - p.kill() - p.wait() + with subprocess.Popen(cmd) as p: + 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: + if os.path.exists(pid_file): + os.remove(pid_file) + log('Kill NBD server') + p.kill() + p.wait() def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt): '''Return True if two image files are identical''' @@ -334,13 +336,12 @@ def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt): def create_image(name, size): '''Create a fully-allocated raw image with sector markers''' - file = open(name, 'wb') - i = 0 - while i < size: - sector = struct.pack('>l504xl', i // 512, i // 512) - file.write(sector) - i = i + 512 - file.close() + with open(name, 'wb') as file: + i = 0 + while i < size: + sector = struct.pack('>l504xl', i // 512, i // 512) + file.write(sector) + i = i + 512 def image_size(img): '''Return image's virtual size''' diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py index 1fc61fcaa3..eddceeb4ae 100644 --- a/tests/qemu-iotests/testrunner.py +++ b/tests/qemu-iotests/testrunner.py @@ -258,17 +258,17 @@ def do_run_test(self, test: str) -> TestResult: t0 = time.time() with f_bad.open('w', encoding="utf-8") as f: - proc = subprocess.Popen(args, cwd=str(f_test.parent), env=env, - stdout=f, stderr=subprocess.STDOUT) - try: - proc.wait() - except KeyboardInterrupt: - proc.terminate() - proc.wait() - return TestResult(status='not run', - description='Interrupted by user', - interrupted=True) - ret = proc.returncode + with subprocess.Popen(args, cwd=str(f_test.parent), env=env, + stdout=f, stderr=subprocess.STDOUT) as proc: + try: + proc.wait() + except KeyboardInterrupt: + proc.terminate() + proc.wait() + return TestResult(status='not run', + description='Interrupted by user', + interrupted=True) + ret = proc.returncode elapsed = round(time.time() - t0, 1)
pylint 2.8 introduces consider-using-with error, suggesting to use the 'with' block statement when possible. Modify all subprocess.Popen call to use the 'with' statement, except the one in __init__ of QemuIoInteractive class, since it is assigned to a class field and used in other methods. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- v1 -> v2: * fix indentation [Max] * explain why we disabled the check in QemuIoInteractive's __init__ [Max] tests/qemu-iotests/iotests.py | 65 ++++++++++++++++---------------- tests/qemu-iotests/testrunner.py | 22 +++++------ 2 files changed, 44 insertions(+), 43 deletions(-)