* [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py
@ 2021-05-12 21:46 John Snow
2021-05-12 21:46 ` [PATCH 01/10] python/console_socket: avoid one-letter variable John Snow
` (10 more replies)
0 siblings, 11 replies; 22+ messages in thread
From: John Snow @ 2021-05-12 21:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
gitlab CI: https://gitlab.com/jsnow/qemu/-/pipelines/301924893
branch: https://gitlab.com/jsnow/qemu/-/commits/python-package-pre-cleanup
This series serves as a pre-requisite for packaging the python series
and getting the linters running via CI. The first patch fixes a linter
error we've had for a while now; the subsequent 9 fix a new warning that
was recently added to pylint 2.8.x.
If there's nobody opposed, I'll take it through my Python queue,
including the iotests bits.
John Snow (10):
python/console_socket: avoid one-letter variable
python/machine: use subprocess.DEVNULL instead of
open(os.path.devnull)
python/machine: use subprocess.run instead of subprocess.Popen
python/console_socket: Add a pylint ignore
python/machine: Disable pylint warning for open() in _pre_launch
python/machine: disable warning for Popen in _launch()
iotests: use subprocess.run where possible
iotests: use 'with open()' where applicable
iotests: silence spurious consider-using-with warnings
iotests: ensure that QemuIoInteractive definitely closes
python/qemu/console_socket.py | 11 ++++---
python/qemu/machine.py | 28 ++++++++++------
tests/qemu-iotests/iotests.py | 55 +++++++++++++++++++-------------
tests/qemu-iotests/testrunner.py | 1 +
4 files changed, 57 insertions(+), 38 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 01/10] python/console_socket: avoid one-letter variable
2021-05-12 21:46 [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py John Snow
@ 2021-05-12 21:46 ` John Snow
2021-05-13 9:39 ` Philippe Mathieu-Daudé
2021-05-12 21:46 ` [PATCH 02/10] python/machine: use subprocess.DEVNULL instead of open(os.path.devnull) John Snow
` (9 subsequent siblings)
10 siblings, 1 reply; 22+ messages in thread
From: John Snow @ 2021-05-12 21:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
Fixes pylint warnings.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
---
python/qemu/console_socket.py | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
index ac21130e446..87237bebef7 100644
--- a/python/qemu/console_socket.py
+++ b/python/qemu/console_socket.py
@@ -46,11 +46,11 @@ def __init__(self, address: str, file: Optional[str] = None,
self._drain_thread = self._thread_start()
def __repr__(self) -> str:
- s = super().__repr__()
- s = s.rstrip(">")
- s = "%s, logfile=%s, drain_thread=%s>" % (s, self._logfile,
- self._drain_thread)
- return s
+ tmp = super().__repr__()
+ tmp = tmp.rstrip(">")
+ tmp = "%s, logfile=%s, drain_thread=%s>" % (tmp, self._logfile,
+ self._drain_thread)
+ return tmp
def _drain_fn(self) -> None:
"""Drains the socket and runs while the socket is open."""
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 02/10] python/machine: use subprocess.DEVNULL instead of open(os.path.devnull)
2021-05-12 21:46 [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py John Snow
2021-05-12 21:46 ` [PATCH 01/10] python/console_socket: avoid one-letter variable John Snow
@ 2021-05-12 21:46 ` John Snow
2021-05-13 9:39 ` Philippe Mathieu-Daudé
2021-05-12 21:46 ` [PATCH 03/10] python/machine: use subprocess.run instead of subprocess.Popen John Snow
` (8 subsequent siblings)
10 siblings, 1 reply; 22+ messages in thread
From: John Snow @ 2021-05-12 21:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
One less file resource to manage, and it helps quiet some pylint >=
2.8.0 warnings about not using a with-context manager for the open call.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/machine.py | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 6e44bda337e..41f51bd27d0 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -223,9 +223,8 @@ def send_fd_scm(self, fd: Optional[int] = None,
assert fd is not None
fd_param.append(str(fd))
- devnull = open(os.path.devnull, 'rb')
proc = subprocess.Popen(
- fd_param, stdin=devnull, stdout=subprocess.PIPE,
+ fd_param, stdin=subprocess.DEVNULL, stdout=subprocess.PIPE,
stderr=subprocess.STDOUT, close_fds=False
)
output = proc.communicate()[0]
@@ -393,7 +392,6 @@ def _launch(self) -> None:
"""
Launch the VM and establish a QMP connection
"""
- devnull = open(os.path.devnull, 'rb')
self._pre_launch()
self._qemu_full_args = tuple(
chain(self._wrapper,
@@ -403,7 +401,7 @@ def _launch(self) -> None:
)
LOG.debug('VM launch command: %r', ' '.join(self._qemu_full_args))
self._popen = subprocess.Popen(self._qemu_full_args,
- stdin=devnull,
+ stdin=subprocess.DEVNULL,
stdout=self._qemu_log_file,
stderr=subprocess.STDOUT,
shell=False,
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 03/10] python/machine: use subprocess.run instead of subprocess.Popen
2021-05-12 21:46 [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py John Snow
2021-05-12 21:46 ` [PATCH 01/10] python/console_socket: avoid one-letter variable John Snow
2021-05-12 21:46 ` [PATCH 02/10] python/machine: use subprocess.DEVNULL instead of open(os.path.devnull) John Snow
@ 2021-05-12 21:46 ` John Snow
2021-05-14 14:08 ` Wainer dos Santos Moschetta
2021-05-12 21:46 ` [PATCH 04/10] python/console_socket: Add a pylint ignore John Snow
` (7 subsequent siblings)
10 siblings, 1 reply; 22+ messages in thread
From: John Snow @ 2021-05-12 21:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
use run() instead of Popen() -- to assert to pylint that we are not
forgetting to close a long-running program.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/machine.py | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 41f51bd27d0..c13ff9b32bf 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -223,13 +223,16 @@ def send_fd_scm(self, fd: Optional[int] = None,
assert fd is not None
fd_param.append(str(fd))
- proc = subprocess.Popen(
- fd_param, stdin=subprocess.DEVNULL, stdout=subprocess.PIPE,
- stderr=subprocess.STDOUT, close_fds=False
+ proc = subprocess.run(
+ fd_param,
+ stdin=subprocess.DEVNULL,
+ stdout=subprocess.PIPE,
+ stderr=subprocess.STDOUT,
+ check=True,
+ close_fds=False,
)
- output = proc.communicate()[0]
- if output:
- LOG.debug(output)
+ if proc.stdout:
+ LOG.debug(proc.stdout)
return proc.returncode
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 04/10] python/console_socket: Add a pylint ignore
2021-05-12 21:46 [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py John Snow
` (2 preceding siblings ...)
2021-05-12 21:46 ` [PATCH 03/10] python/machine: use subprocess.run instead of subprocess.Popen John Snow
@ 2021-05-12 21:46 ` John Snow
2021-05-12 21:46 ` [PATCH 05/10] python/machine: Disable pylint warning for open() in _pre_launch John Snow
` (6 subsequent siblings)
10 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2021-05-12 21:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
We manage cleaning up this resource ourselves. Pylint should shush.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/console_socket.py | 1 +
1 file changed, 1 insertion(+)
diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
index 87237bebef7..8c4ff598ad7 100644
--- a/python/qemu/console_socket.py
+++ b/python/qemu/console_socket.py
@@ -39,6 +39,7 @@ def __init__(self, address: str, file: Optional[str] = None,
self.connect(address)
self._logfile = None
if file:
+ # pylint: disable=consider-using-with
self._logfile = open(file, "bw")
self._open = True
self._drain_thread = None
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 05/10] python/machine: Disable pylint warning for open() in _pre_launch
2021-05-12 21:46 [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py John Snow
` (3 preceding siblings ...)
2021-05-12 21:46 ` [PATCH 04/10] python/console_socket: Add a pylint ignore John Snow
@ 2021-05-12 21:46 ` John Snow
2021-05-14 14:42 ` Wainer dos Santos Moschetta
2021-05-12 21:46 ` [PATCH 06/10] python/machine: disable warning for Popen in _launch() John Snow
` (5 subsequent siblings)
10 siblings, 1 reply; 22+ messages in thread
From: John Snow @ 2021-05-12 21:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
Shift the open() call later so that the pylint pragma applies *only* to
that one open() call. Add a note that suggests why this is safe: the
resource is unconditionally cleaned up in _post_shutdown().
_post_shutdown is called after failed launches (see launch()), and
unconditionally after every call to shutdown(), and therefore also on
__exit__.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/machine.py | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index c13ff9b32bf..8f86303b48f 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -308,7 +308,6 @@ def _pre_launch(self) -> None:
self._temp_dir = tempfile.mkdtemp(prefix="qemu-machine-",
dir=self._test_dir)
self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log")
- self._qemu_log_file = open(self._qemu_log_path, 'wb')
if self._console_set:
self._remove_files.append(self._console_address)
@@ -323,6 +322,11 @@ def _pre_launch(self) -> None:
nickname=self._name
)
+ # NOTE: Make sure any opened resources are *definitely* freed in
+ # _post_shutdown()!
+ # pylint: disable=consider-using-with
+ self._qemu_log_file = open(self._qemu_log_path, 'wb')
+
def _post_launch(self) -> None:
if self._qmp_connection:
self._qmp.accept()
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 06/10] python/machine: disable warning for Popen in _launch()
2021-05-12 21:46 [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py John Snow
` (4 preceding siblings ...)
2021-05-12 21:46 ` [PATCH 05/10] python/machine: Disable pylint warning for open() in _pre_launch John Snow
@ 2021-05-12 21:46 ` John Snow
2021-05-12 21:46 ` [PATCH 07/10] iotests: use subprocess.run where possible John Snow
` (4 subsequent siblings)
10 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2021-05-12 21:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
We handle this resource rather meticulously in
shutdown/kill/wait/__exit__ et al, through the laborious mechanisms in
_do_shutdown().
Quiet this pylint warning here.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/machine.py | 3 +++
1 file changed, 3 insertions(+)
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 8f86303b48f..0df5b2f386f 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -407,6 +407,9 @@ def _launch(self) -> None:
self._args)
)
LOG.debug('VM launch command: %r', ' '.join(self._qemu_full_args))
+
+ # Cleaning up of this subprocess is guaranteed by _do_shutdown.
+ # pylint: disable=consider-using-with
self._popen = subprocess.Popen(self._qemu_full_args,
stdin=subprocess.DEVNULL,
stdout=self._qemu_log_file,
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 07/10] iotests: use subprocess.run where possible
2021-05-12 21:46 [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py John Snow
` (5 preceding siblings ...)
2021-05-12 21:46 ` [PATCH 06/10] python/machine: disable warning for Popen in _launch() John Snow
@ 2021-05-12 21:46 ` John Snow
2021-05-14 19:22 ` John Snow
2021-05-12 21:46 ` [PATCH 08/10] iotests: use 'with open()' where applicable John Snow
` (3 subsequent siblings)
10 siblings, 1 reply; 22+ messages in thread
From: John Snow @ 2021-05-12 21:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
pylint 2.8.x adds warnings whenever we use Popen calls without using
'with', so it's desirable to convert synchronous calls to run()
invocations where applicable.
(Though, this trades one pylint warning for another due to a pylint bug,
which I've silenced with a pragma and a link to the bug.)
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/iotests.py | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 5af01828951..46deb7f4dd4 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -113,15 +113,16 @@ 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:
+ res = subprocess.run(args,
+ stdout=subprocess.PIPE,
+ stderr=stderr,
+ universal_newlines=True,
+ check=False)
+ output = res.stdout
+ if res.returncode < 0:
cmd = ' '.join(args)
- sys.stderr.write(f'{tool} received signal {-subp.returncode}: {cmd}\n')
- return (output, subp.returncode)
+ sys.stderr.write(f'{tool} received signal {-res.returncode}: {cmd}\n')
+ return (output, res.returncode)
def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]:
"""
@@ -1153,6 +1154,8 @@ def _verify_virtio_scsi_pci_or_ccw() -> None:
def supports_quorum():
+ # https://github.com/PyCQA/astroid/issues/689
+ # pylint: disable=unsupported-membership-test
return 'quorum' in qemu_img_pipe('--help')
def verify_quorum():
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 08/10] iotests: use 'with open()' where applicable
2021-05-12 21:46 [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py John Snow
` (6 preceding siblings ...)
2021-05-12 21:46 ` [PATCH 07/10] iotests: use subprocess.run where possible John Snow
@ 2021-05-12 21:46 ` John Snow
2021-05-13 9:40 ` Philippe Mathieu-Daudé
2021-05-12 21:46 ` [PATCH 09/10] iotests: silence spurious consider-using-with warnings John Snow
` (2 subsequent siblings)
10 siblings, 1 reply; 22+ messages in thread
From: John Snow @ 2021-05-12 21:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
More pylint 2.8.x warning hushing: use open's context manager where it's
applicable to do so to avoid a warning.
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/iotests.py | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 46deb7f4dd4..5d5ec40429b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -335,13 +335,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 outfile:
+ i = 0
+ while i < size:
+ sector = struct.pack('>l504xl', i // 512, i // 512)
+ outfile.write(sector)
+ i = i + 512
def image_size(img):
'''Return image's virtual size'''
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 09/10] iotests: silence spurious consider-using-with warnings
2021-05-12 21:46 [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py John Snow
` (7 preceding siblings ...)
2021-05-12 21:46 ` [PATCH 08/10] iotests: use 'with open()' where applicable John Snow
@ 2021-05-12 21:46 ` John Snow
2021-05-12 21:46 ` [PATCH 10/10] iotests: ensure that QemuIoInteractive definitely closes John Snow
2021-05-17 16:11 ` [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py John Snow
10 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2021-05-12 21:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
In a few cases, we can't use 'with ...' because they belong to
long-running classes that need those items to stay open at the end of
the block. We're handling it, so tell pylint to shush.
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/iotests.py | 2 +-
tests/qemu-iotests/testrunner.py | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 5d5ec40429b..e09c991b84e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -311,7 +311,7 @@ def qemu_nbd_popen(*args):
cmd.extend(args)
log('Start NBD server')
- p = subprocess.Popen(cmd)
+ p = subprocess.Popen(cmd) # pylint: disable=consider-using-with
try:
while not os.path.exists(pid_file):
if p.poll() is not None:
diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 1fc61fcaa34..34fb551c01b 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -258,6 +258,7 @@ def do_run_test(self, test: str) -> TestResult:
t0 = time.time()
with f_bad.open('w', encoding="utf-8") as f:
+ # pylint: disable=consider-using-with
proc = subprocess.Popen(args, cwd=str(f_test.parent), env=env,
stdout=f, stderr=subprocess.STDOUT)
try:
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 10/10] iotests: ensure that QemuIoInteractive definitely closes
2021-05-12 21:46 [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py John Snow
` (8 preceding siblings ...)
2021-05-12 21:46 ` [PATCH 09/10] iotests: silence spurious consider-using-with warnings John Snow
@ 2021-05-12 21:46 ` John Snow
2021-05-17 16:11 ` [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py John Snow
10 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2021-05-12 21:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Max Reitz, Cleber Rosa,
John Snow
More on the lines of quieting pylint 2.8.x, though to make it obvious
that we definitely handle the cleanup here, I elected to bolster the
close() method here.
1. Check for the process having terminated early more rigorously by
checking poll() directly.
2. Change the prompt read into an assertion.
3. Ensure that the final communicate() call *definitely* closes the
socket, adding a timeout and a final kill just in case.
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/iotests.py | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e09c991b84e..12e876fa67d 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -238,20 +238,27 @@ def qemu_io_silent_check(*args):
class QemuIoInteractive:
def __init__(self, *args):
self.args = qemu_io_args_no_fmt + list(args)
+
+ # Resource cleaned up via close()
+ # pylint: disable=consider-using-with
self._p = subprocess.Popen(self.args, stdin=subprocess.PIPE,
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
universal_newlines=True)
+ if self._p.poll():
+ # Failed to start.
+ out = self._p.stdout.read()
+ raise subprocess.SubprocessError(out, self._p.poll())
+
+ # Eat the first prompt
out = self._p.stdout.read(9)
- if out != 'qemu-io> ':
- # Most probably qemu-io just failed to start.
- # Let's collect the whole output and exit.
- out += self._p.stdout.read()
- self._p.wait(timeout=1)
- raise ValueError(out)
+ assert out == 'qemu-io> ', "Did not understand qemu-io prompt"
def close(self):
- self._p.communicate('q\n')
+ try:
+ self._p.communicate('q\n', timeout=5)
+ except subprocess.TimeoutExpired:
+ self._p.kill()
def _read_output(self):
pattern = 'qemu-io> '
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 01/10] python/console_socket: avoid one-letter variable
2021-05-12 21:46 ` [PATCH 01/10] python/console_socket: avoid one-letter variable John Snow
@ 2021-05-13 9:39 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-13 9:39 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Cleber Rosa, Eduardo Habkost, qemu-block, Max Reitz
On 5/12/21 11:46 PM, John Snow wrote:
> Fixes pylint warnings.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> ---
> python/qemu/console_socket.py | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 02/10] python/machine: use subprocess.DEVNULL instead of open(os.path.devnull)
2021-05-12 21:46 ` [PATCH 02/10] python/machine: use subprocess.DEVNULL instead of open(os.path.devnull) John Snow
@ 2021-05-13 9:39 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-13 9:39 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Cleber Rosa, Eduardo Habkost, qemu-block, Max Reitz
On 5/12/21 11:46 PM, John Snow wrote:
> One less file resource to manage, and it helps quiet some pylint >=
> 2.8.0 warnings about not using a with-context manager for the open call.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> python/qemu/machine.py | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 08/10] iotests: use 'with open()' where applicable
2021-05-12 21:46 ` [PATCH 08/10] iotests: use 'with open()' where applicable John Snow
@ 2021-05-13 9:40 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-13 9:40 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Cleber Rosa, Eduardo Habkost, qemu-block, Max Reitz
On 5/12/21 11:46 PM, John Snow wrote:
> More pylint 2.8.x warning hushing: use open's context manager where it's
> applicable to do so to avoid a warning.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/iotests.py | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 03/10] python/machine: use subprocess.run instead of subprocess.Popen
2021-05-12 21:46 ` [PATCH 03/10] python/machine: use subprocess.run instead of subprocess.Popen John Snow
@ 2021-05-14 14:08 ` Wainer dos Santos Moschetta
2021-05-14 19:00 ` John Snow
2021-05-14 19:13 ` John Snow
0 siblings, 2 replies; 22+ messages in thread
From: Wainer dos Santos Moschetta @ 2021-05-14 14:08 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Cleber Rosa, Eduardo Habkost, qemu-block, Max Reitz
Hi,
On 5/12/21 6:46 PM, John Snow wrote:
> use run() instead of Popen() -- to assert to pylint that we are not
> forgetting to close a long-running program.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> python/qemu/machine.py | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index 41f51bd27d0..c13ff9b32bf 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -223,13 +223,16 @@ def send_fd_scm(self, fd: Optional[int] = None,
> assert fd is not None
> fd_param.append(str(fd))
>
> - proc = subprocess.Popen(
> - fd_param, stdin=subprocess.DEVNULL, stdout=subprocess.PIPE,
> - stderr=subprocess.STDOUT, close_fds=False
> + proc = subprocess.run(
> + fd_param,
> + stdin=subprocess.DEVNULL,
> + stdout=subprocess.PIPE,
> + stderr=subprocess.STDOUT,
> + check=True,
> + close_fds=False,
> )
Now it might throw a CalledProcessError given that `check=True`.
Shouldn't it capture the exception and (possible) re-throw as an
QEMUMachineError?
- Wainer
> - output = proc.communicate()[0]
> - if output:
> - LOG.debug(output)
> + if proc.stdout:
> + LOG.debug(proc.stdout)
>
> return proc.returncode
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 05/10] python/machine: Disable pylint warning for open() in _pre_launch
2021-05-12 21:46 ` [PATCH 05/10] python/machine: Disable pylint warning for open() in _pre_launch John Snow
@ 2021-05-14 14:42 ` Wainer dos Santos Moschetta
2021-05-14 19:03 ` John Snow
0 siblings, 1 reply; 22+ messages in thread
From: Wainer dos Santos Moschetta @ 2021-05-14 14:42 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Cleber Rosa, Eduardo Habkost, qemu-block, Max Reitz
Hi,
On 5/12/21 6:46 PM, John Snow wrote:
> Shift the open() call later so that the pylint pragma applies *only* to
> that one open() call. Add a note that suggests why this is safe: the
> resource is unconditionally cleaned up in _post_shutdown().
You can also put it in a pylint disable/enable block. E.g.:
# pylint: disable=consider-using-with
self._qemu_log_file = open(self._qemu_log_path, 'wb')
# pylint: enable=consider-using-with
However I don't know if this is bad practice. :)
Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>
> _post_shutdown is called after failed launches (see launch()), and
> unconditionally after every call to shutdown(), and therefore also on
> __exit__.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> python/qemu/machine.py | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index c13ff9b32bf..8f86303b48f 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -308,7 +308,6 @@ def _pre_launch(self) -> None:
> self._temp_dir = tempfile.mkdtemp(prefix="qemu-machine-",
> dir=self._test_dir)
> self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log")
> - self._qemu_log_file = open(self._qemu_log_path, 'wb')
>
> if self._console_set:
> self._remove_files.append(self._console_address)
> @@ -323,6 +322,11 @@ def _pre_launch(self) -> None:
> nickname=self._name
> )
>
> + # NOTE: Make sure any opened resources are *definitely* freed in
> + # _post_shutdown()!
> + # pylint: disable=consider-using-with
> + self._qemu_log_file = open(self._qemu_log_path, 'wb')
> +
> def _post_launch(self) -> None:
> if self._qmp_connection:
> self._qmp.accept()
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 03/10] python/machine: use subprocess.run instead of subprocess.Popen
2021-05-14 14:08 ` Wainer dos Santos Moschetta
@ 2021-05-14 19:00 ` John Snow
2021-05-14 19:13 ` John Snow
1 sibling, 0 replies; 22+ messages in thread
From: John Snow @ 2021-05-14 19:00 UTC (permalink / raw)
To: Wainer dos Santos Moschetta, qemu-devel
Cc: Kevin Wolf, Cleber Rosa, Eduardo Habkost, qemu-block, Max Reitz
On 5/14/21 10:08 AM, Wainer dos Santos Moschetta wrote:
> Hi,
>
> On 5/12/21 6:46 PM, John Snow wrote:
>> use run() instead of Popen() -- to assert to pylint that we are not
>> forgetting to close a long-running program.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> python/qemu/machine.py | 15 +++++++++------
>> 1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>> index 41f51bd27d0..c13ff9b32bf 100644
>> --- a/python/qemu/machine.py
>> +++ b/python/qemu/machine.py
>> @@ -223,13 +223,16 @@ def send_fd_scm(self, fd: Optional[int] = None,
>> assert fd is not None
>> fd_param.append(str(fd))
>> - proc = subprocess.Popen(
>> - fd_param, stdin=subprocess.DEVNULL, stdout=subprocess.PIPE,
>> - stderr=subprocess.STDOUT, close_fds=False
>> + proc = subprocess.run(
>> + fd_param,
>> + stdin=subprocess.DEVNULL,
>> + stdout=subprocess.PIPE,
>> + stderr=subprocess.STDOUT,
>> + check=True,
>> + close_fds=False,
>> )
>
> Now it might throw a CalledProcessError given that `check=True`.
> Shouldn't it capture the exception and (possible) re-throw as an
> QEMUMachineError?
>
> - Wainer
>
I suppose I ought to so that it matches the other errors of this method,
yes.
Setting it to false and checking manually may be less code, but yeah.
I'll change this.
Thanks!
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 05/10] python/machine: Disable pylint warning for open() in _pre_launch
2021-05-14 14:42 ` Wainer dos Santos Moschetta
@ 2021-05-14 19:03 ` John Snow
0 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2021-05-14 19:03 UTC (permalink / raw)
To: Wainer dos Santos Moschetta, qemu-devel
Cc: Kevin Wolf, Cleber Rosa, Eduardo Habkost, qemu-block, Max Reitz
On 5/14/21 10:42 AM, Wainer dos Santos Moschetta wrote:
> Hi,
>
> On 5/12/21 6:46 PM, John Snow wrote:
>> Shift the open() call later so that the pylint pragma applies *only* to
>> that one open() call. Add a note that suggests why this is safe: the
>> resource is unconditionally cleaned up in _post_shutdown().
>
>
> You can also put it in a pylint disable/enable block. E.g.:
>
> # pylint: disable=consider-using-with
>
> self._qemu_log_file = open(self._qemu_log_path, 'wb')
>
> # pylint: enable=consider-using-with
>
> However I don't know if this is bad practice. :)
>
I learned a new trick!
> Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>
Thanks. In this case I will probably leave this alone unless someone
else voices a strong opinion. I figure the comment protects us against
future oopses well enough.
>>
>> _post_shutdown is called after failed launches (see launch()), and
>> unconditionally after every call to shutdown(), and therefore also on
>> __exit__.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> python/qemu/machine.py | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>> index c13ff9b32bf..8f86303b48f 100644
>> --- a/python/qemu/machine.py
>> +++ b/python/qemu/machine.py
>> @@ -308,7 +308,6 @@ def _pre_launch(self) -> None:
>> self._temp_dir = tempfile.mkdtemp(prefix="qemu-machine-",
>> dir=self._test_dir)
>> self._qemu_log_path = os.path.join(self._temp_dir,
>> self._name + ".log")
>> - self._qemu_log_file = open(self._qemu_log_path, 'wb')
>> if self._console_set:
>> self._remove_files.append(self._console_address)
>> @@ -323,6 +322,11 @@ def _pre_launch(self) -> None:
>> nickname=self._name
>> )
>> + # NOTE: Make sure any opened resources are *definitely* freed in
>> + # _post_shutdown()!
>> + # pylint: disable=consider-using-with
>> + self._qemu_log_file = open(self._qemu_log_path, 'wb')
>> +
>> def _post_launch(self) -> None:
>> if self._qmp_connection:
>> self._qmp.accept()
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 03/10] python/machine: use subprocess.run instead of subprocess.Popen
2021-05-14 14:08 ` Wainer dos Santos Moschetta
2021-05-14 19:00 ` John Snow
@ 2021-05-14 19:13 ` John Snow
1 sibling, 0 replies; 22+ messages in thread
From: John Snow @ 2021-05-14 19:13 UTC (permalink / raw)
To: Wainer dos Santos Moschetta, qemu-devel
Cc: Kevin Wolf, Cleber Rosa, Eduardo Habkost, qemu-block, Max Reitz
On 5/14/21 10:08 AM, Wainer dos Santos Moschetta wrote:
> Now it might throw a CalledProcessError given that `check=True`.
> Shouldn't it capture the exception and (possible) re-throw as an
> QEMUMachineError?
I lied to you again. The existing callers all check for failure
explicitly, so in the interest of avoiding an API change, I'm just going
to set check=False here.
We can improve the interface separately some other time.
--js
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 07/10] iotests: use subprocess.run where possible
2021-05-12 21:46 ` [PATCH 07/10] iotests: use subprocess.run where possible John Snow
@ 2021-05-14 19:22 ` John Snow
0 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2021-05-14 19:22 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Max Reitz, Eduardo Habkost, qemu-block, Cleber Rosa
On 5/12/21 5:46 PM, John Snow wrote:
> pylint 2.8.x adds warnings whenever we use Popen calls without using
> 'with', so it's desirable to convert synchronous calls to run()
> invocations where applicable.
>
> (Though, this trades one pylint warning for another due to a pylint bug,
> which I've silenced with a pragma and a link to the bug.)
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/iotests.py | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 5af01828951..46deb7f4dd4 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -113,15 +113,16 @@ 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:
> + res = subprocess.run(args,
> + stdout=subprocess.PIPE,
> + stderr=stderr,
> + universal_newlines=True,
> + check=False)
> + output = res.stdout
> + if res.returncode < 0:
> cmd = ' '.join(args)
> - sys.stderr.write(f'{tool} received signal {-subp.returncode}: {cmd}\n')
> - return (output, subp.returncode)
> + sys.stderr.write(f'{tool} received signal {-res.returncode}: {cmd}\n')
> + return (output, res.returncode)
>
> def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]:
> """
> @@ -1153,6 +1154,8 @@ def _verify_virtio_scsi_pci_or_ccw() -> None:
>
>
> def supports_quorum():
> + # https://github.com/PyCQA/astroid/issues/689
Oh, realizing this bug was closed, so this is something
similar-but-different.
Bah. I'll delete this comment and change the commit message.
> + # pylint: disable=unsupported-membership-test
> return 'quorum' in qemu_img_pipe('--help')
>
> def verify_quorum():
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py
2021-05-12 21:46 [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py John Snow
` (9 preceding siblings ...)
2021-05-12 21:46 ` [PATCH 10/10] iotests: ensure that QemuIoInteractive definitely closes John Snow
@ 2021-05-17 16:11 ` John Snow
2021-05-17 17:02 ` Emanuele Giuseppe Esposito
10 siblings, 1 reply; 22+ messages in thread
From: John Snow @ 2021-05-17 16:11 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Emanuele Giuseppe Esposito, Eduardo Habkost,
qemu-block, Max Reitz, Cleber Rosa
On 5/12/21 5:46 PM, John Snow wrote:
> gitlab CI: https://gitlab.com/jsnow/qemu/-/pipelines/301924893
> branch: https://gitlab.com/jsnow/qemu/-/commits/python-package-pre-cleanup
>
> This series serves as a pre-requisite for packaging the python series
> and getting the linters running via CI. The first patch fixes a linter
> error we've had for a while now; the subsequent 9 fix a new warning that
> was recently added to pylint 2.8.x.
>
> If there's nobody opposed, I'll take it through my Python queue,
> including the iotests bits.
>
> John Snow (10):
> python/console_socket: avoid one-letter variable
> python/machine: use subprocess.DEVNULL instead of
> open(os.path.devnull)
> python/machine: use subprocess.run instead of subprocess.Popen
> python/console_socket: Add a pylint ignore
> python/machine: Disable pylint warning for open() in _pre_launch
> python/machine: disable warning for Popen in _launch()
> iotests: use subprocess.run where possible
> iotests: use 'with open()' where applicable
> iotests: silence spurious consider-using-with warnings
> iotests: ensure that QemuIoInteractive definitely closes
>
> python/qemu/console_socket.py | 11 ++++---
> python/qemu/machine.py | 28 ++++++++++------
> tests/qemu-iotests/iotests.py | 55 +++++++++++++++++++-------------
> tests/qemu-iotests/testrunner.py | 1 +
> 4 files changed, 57 insertions(+), 38 deletions(-)
>
The iotests stuff was handled by Emanuele Giuseppe Esposito instead, and
-- I must admit -- better than I did. Dropping patches 7-10.
--js
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py
2021-05-17 16:11 ` [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py John Snow
@ 2021-05-17 17:02 ` Emanuele Giuseppe Esposito
0 siblings, 0 replies; 22+ messages in thread
From: Emanuele Giuseppe Esposito @ 2021-05-17 17:02 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Max Reitz, Eduardo Habkost, qemu-block, Cleber Rosa
On 17/05/2021 18:11, John Snow wrote:
> On 5/12/21 5:46 PM, John Snow wrote:
>> gitlab CI: https://gitlab.com/jsnow/qemu/-/pipelines/301924893
>> branch:
>> https://gitlab.com/jsnow/qemu/-/commits/python-package-pre-cleanup
>>
>> This series serves as a pre-requisite for packaging the python series
>> and getting the linters running via CI. The first patch fixes a linter
>> error we've had for a while now; the subsequent 9 fix a new warning that
>> was recently added to pylint 2.8.x.
>>
>> If there's nobody opposed, I'll take it through my Python queue,
>> including the iotests bits.
>>
>> John Snow (10):
>> python/console_socket: avoid one-letter variable
>> python/machine: use subprocess.DEVNULL instead of
>> open(os.path.devnull)
>> python/machine: use subprocess.run instead of subprocess.Popen
>> python/console_socket: Add a pylint ignore
>> python/machine: Disable pylint warning for open() in _pre_launch
>> python/machine: disable warning for Popen in _launch()
>> iotests: use subprocess.run where possible
>> iotests: use 'with open()' where applicable
>> iotests: silence spurious consider-using-with warnings
>> iotests: ensure that QemuIoInteractive definitely closes
>>
>> python/qemu/console_socket.py | 11 ++++---
>> python/qemu/machine.py | 28 ++++++++++------
>> tests/qemu-iotests/iotests.py | 55 +++++++++++++++++++-------------
>> tests/qemu-iotests/testrunner.py | 1 +
>> 4 files changed, 57 insertions(+), 38 deletions(-)
>>
>
> The iotests stuff was handled by Emanuele Giuseppe Esposito instead, and
> -- I must admit -- better than I did. Dropping patches 7-10.
Yes, patch 7-9 + the #pylint: disable= in patch 10 are covered in
"qemu-iotests: fix pylint 2.8 consider-using-with error"
https://patchew.org/QEMU/20210510190449.65948-1-eesposit@redhat.com/
that is merged.
Just wanted to point that maybe you want to keep part of patch 10, if
you think that it is important :)
Emanuele
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2021-05-17 17:08 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-12 21:46 [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py John Snow
2021-05-12 21:46 ` [PATCH 01/10] python/console_socket: avoid one-letter variable John Snow
2021-05-13 9:39 ` Philippe Mathieu-Daudé
2021-05-12 21:46 ` [PATCH 02/10] python/machine: use subprocess.DEVNULL instead of open(os.path.devnull) John Snow
2021-05-13 9:39 ` Philippe Mathieu-Daudé
2021-05-12 21:46 ` [PATCH 03/10] python/machine: use subprocess.run instead of subprocess.Popen John Snow
2021-05-14 14:08 ` Wainer dos Santos Moschetta
2021-05-14 19:00 ` John Snow
2021-05-14 19:13 ` John Snow
2021-05-12 21:46 ` [PATCH 04/10] python/console_socket: Add a pylint ignore John Snow
2021-05-12 21:46 ` [PATCH 05/10] python/machine: Disable pylint warning for open() in _pre_launch John Snow
2021-05-14 14:42 ` Wainer dos Santos Moschetta
2021-05-14 19:03 ` John Snow
2021-05-12 21:46 ` [PATCH 06/10] python/machine: disable warning for Popen in _launch() John Snow
2021-05-12 21:46 ` [PATCH 07/10] iotests: use subprocess.run where possible John Snow
2021-05-14 19:22 ` John Snow
2021-05-12 21:46 ` [PATCH 08/10] iotests: use 'with open()' where applicable John Snow
2021-05-13 9:40 ` Philippe Mathieu-Daudé
2021-05-12 21:46 ` [PATCH 09/10] iotests: silence spurious consider-using-with warnings John Snow
2021-05-12 21:46 ` [PATCH 10/10] iotests: ensure that QemuIoInteractive definitely closes John Snow
2021-05-17 16:11 ` [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py John Snow
2021-05-17 17:02 ` Emanuele Giuseppe Esposito
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).