[PATCH 00/10] Python: delint iotests, machine.py and console_socket.py

[PATCH 00/10] Python: delint iotests, machine.py and console_socket.py

[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

[PATCH 01/10] python/console_socket: avoid one-letter variable

[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

[PATCH 02/10] python/machine: use subprocess.DEVNULL instead of open(os.path.devnull)

[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

[PATCH 03/10] python/machine: use subprocess.run instead of subprocess.Popen

[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

[PATCH 04/10] python/console_socket: Add a pylint ignore

[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

[PATCH 05/10] python/machine: Disable pylint warning for open() in _pre_launch

[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

[PATCH 06/10] python/machine: disable warning for Popen in _launch()

[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

[PATCH 07/10] iotests: use subprocess.run where possible

[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

[PATCH 08/10] iotests: use 'with open()' where applicable

[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

[PATCH 09/10] iotests: silence spurious consider-using-with warnings

[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

[PATCH 10/10] iotests: ensure that QemuIoInteractive definitely closes

[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

Re: [PATCH 01/10] python/console_socket: avoid one-letter variable

[Philippe Mathieu-Daudé]

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>

Re: [PATCH 02/10] python/machine: use subprocess.DEVNULL instead of open(os.path.devnull)

[Philippe Mathieu-Daudé]

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>

Re: [PATCH 08/10] iotests: use 'with open()' where applicable

[Philippe Mathieu-Daudé]

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>

Re: [PATCH 03/10] python/machine: use subprocess.run instead of subprocess.Popen

[Wainer dos Santos Moschetta]

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
>   

Re: [PATCH 05/10] python/machine: Disable pylint warning for open() in _pre_launch

[Wainer dos Santos Moschetta]

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()

Re: [PATCH 03/10] python/machine: use subprocess.run instead of subprocess.Popen

[John Snow]

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!

Re: [PATCH 05/10] python/machine: Disable pylint warning for open() in _pre_launch

[John Snow]

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()

Re: [PATCH 03/10] python/machine: use subprocess.run instead of subprocess.Popen

[John Snow]

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

Re: [PATCH 07/10] iotests: use subprocess.run where possible

[John Snow]

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():
> 

Re: [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py

[John Snow]

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

Re: [PATCH 00/10] Python: delint iotests, machine.py and console_socket.py

[Emanuele Giuseppe Esposito]

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