[PATCH v2 0/6] Python: delint python library

[PATCH v2 0/6] Python: delint python library

[John Snow]

gitlab CI: https://gitlab.com/jsnow/qemu/-/pipelines/304224309
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 ones fix a new warning
that was recently added to pylint 2.8.x.

V2:
 - iotest bits already handled, dropped here.
 - Changed patch #3 based on feedback from Wainer.

John Snow (6):
  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()

 python/qemu/console_socket.py | 11 ++++++-----
 python/qemu/machine.py        | 28 ++++++++++++++++++----------
 2 files changed, 24 insertions(+), 15 deletions(-)

-- 
2.30.2

[PATCH v2 1/6] 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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@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 v2 2/6] 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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@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

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

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 464 bytes --]

On Mon, May 17, 2021 at 02:48:04PM -0400, 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>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  python/qemu/machine.py | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 3/6] 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..2e55e2c8bd8 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=False,
+            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

Re: [PATCH v2 3/6] python/machine: use subprocess.run instead of subprocess.Popen

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 474 bytes --]

On Mon, May 17, 2021 at 02:48:05PM -0400, 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(-)
>

Reviewed-by: Cleber Rosa <crosa@redhat.com>

qemu-iotests 045 and 147 are happy, so:

Tested-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 4/6] 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

Re: [PATCH v2 4/6] python/console_socket: Add a pylint ignore

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 314 bytes --]

On Mon, May 17, 2021 at 02:48:06PM -0400, John Snow wrote:
> 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(+)
>

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 5/6] 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>
Reviewed-by: Wainer dos Santos Moschetta <wainersm@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 2e55e2c8bd8..f021127f0fc 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

Re: [PATCH v2 5/6] python/machine: Disable pylint warning for open() in _pre_launch

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]

On Mon, May 17, 2021 at 02:48:07PM -0400, 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().
> 
> _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>
> Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> ---
>  python/qemu/machine.py | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 6/6] 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 f021127f0fc..06058d89e83 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

Re: [PATCH v2 6/6] python/machine: disable warning for Popen in _launch()

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 421 bytes --]

On Mon, May 17, 2021 at 02:48:08PM -0400, John Snow wrote:
> 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(+)
>

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/6] Python: delint python library

[John Snow]

On 5/17/21 2:48 PM, John Snow wrote:
> gitlab CI: https://gitlab.com/jsnow/qemu/-/pipelines/304224309
> 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 ones fix a new warning
> that was recently added to pylint 2.8.x.
> 
> V2:
>   - iotest bits already handled, dropped here.
>   - Changed patch #3 based on feedback from Wainer.
> 
> John Snow (6):
>    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()
> 
>   python/qemu/console_socket.py | 11 ++++++-----
>   python/qemu/machine.py        | 28 ++++++++++++++++++----------
>   2 files changed, 24 insertions(+), 15 deletions(-)
> 

Thank you for reviews! Staged to my Python branch.

--js