[PATCH v2 0/5] python/qemu: qmp: Fix, delint and improvements

[PATCH v2 0/5] python/qemu: qmp: Fix, delint and improvements

[Wainer dos Santos Moschetta]

I started fixing an issue on exception handling which in some places
currently use the deprecated (in Python 3.3) `socket.error`. Then I
ended up delinting the module code and making some improvements.

Changes in v2:
- Rebased to master. No conflicts.
- Added docstring describing the allowed value of the new `timeout`
  option in accept() (patch 03) [jsnow]
- Set the new `timeout` option to 15.0 by default [philmd]

v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg667479.html

Git:
- Tree: https://github.com/wainersm/qemu
- Branch: python_qmp_sockets_error-v2

CI:
- Travis (FAIL): https://travis-ci.org/wainersm/qemu/builds/645583812
  Jobs failures aren't related with these changes


Wainer dos Santos Moschetta (5):
  python/qemu: qmp: Replace socket.error with OSError
  python/qemu: Delint the qmp module
  python/qemu: qmp: Make accept()'s timeout configurable
  python/qemu: qmp: Make QEMUMonitorProtocol a context manager
  python/qemu: qmp: Remove unnused attributes

 python/qemu/qmp.py | 97 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 71 insertions(+), 26 deletions(-)

-- 
2.23.0

[PATCH v2 1/5] python/qemu: qmp: Replace socket.error with OSError

[Wainer dos Santos Moschetta]

The socket.error is deprecated from Python 3.3, instead it is
made a link to OSError. This change replaces the occurences
of socket.error with OSError.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 python/qemu/qmp.py | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index 5c8cf6a056..8c6c9847d0 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -47,7 +47,7 @@ class QEMUMonitorProtocol(object):
                         or a tuple in the form ( address, port ) for a TCP
                         connection
         @param server: server mode listens on the socket (bool)
-        @raise socket.error on socket connection errors
+        @raise OSError on socket connection errors
         @note No connection is established, this is done by the connect() or
               accept() methods
         """
@@ -107,8 +107,8 @@ class QEMUMonitorProtocol(object):
         self.__sock.setblocking(0)
         try:
             self.__json_read()
-        except socket.error as err:
-            if err[0] == errno.EAGAIN:
+        except OSError as err:
+            if err.errno == errno.EAGAIN:
                 # No data available
                 pass
         self.__sock.setblocking(1)
@@ -133,7 +133,7 @@ class QEMUMonitorProtocol(object):
         Connect to the QMP Monitor and perform capabilities negotiation.
 
         @return QMP greeting dict
-        @raise socket.error on socket connection errors
+        @raise OSError on socket connection errors
         @raise QMPConnectError if the greeting is not received
         @raise QMPCapabilitiesError if fails to negotiate capabilities
         """
@@ -147,7 +147,7 @@ class QEMUMonitorProtocol(object):
         Await connection from QMP Monitor and perform capabilities negotiation.
 
         @return QMP greeting dict
-        @raise socket.error on socket connection errors
+        @raise OSError on socket connection errors
         @raise QMPConnectError if the greeting is not received
         @raise QMPCapabilitiesError if fails to negotiate capabilities
         """
@@ -167,10 +167,10 @@ class QEMUMonitorProtocol(object):
         self.logger.debug(">>> %s", qmp_cmd)
         try:
             self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8'))
-        except socket.error as err:
-            if err[0] == errno.EPIPE:
+        except OSError as err:
+            if err.errno == errno.EPIPE:
                 return
-            raise socket.error(err)
+            raise err
         resp = self.__json_read()
         self.logger.debug("<<< %s", resp)
         return resp
-- 
2.23.0

[PATCH v2 2/5] python/qemu: Delint the qmp module

[Wainer dos Santos Moschetta]

This clean up the pylint-3 report on qmp:

************* Module qemu.qmp
python/qemu/qmp.py:1:0: C0111: Missing module docstring (missing-docstring)
python/qemu/qmp.py:17:0: C0111: Missing class docstring (missing-docstring)
python/qemu/qmp.py:21:0: C0111: Missing class docstring (missing-docstring)
python/qemu/qmp.py:25:0: C0111: Missing class docstring (missing-docstring)
python/qemu/qmp.py:29:0: C0111: Missing class docstring (missing-docstring)
python/qemu/qmp.py:33:0: C0111: Missing class docstring (missing-docstring)
python/qemu/qmp.py:33:0: R0205: Class 'QEMUMonitorProtocol' inherits from object, can be safely removed from bases in python3 (useless-object-inheritance)
python/qemu/qmp.py:80:4: R1710: Either all return statements in a function should return an expression, or none of them should. (inconsistent-return-statements)
python/qemu/qmp.py:131:4: R1710: Either all return statements in a function should return an expression, or none of them should. (inconsistent-return-statements)
python/qemu/qmp.py:159:4: R1710: Either all return statements in a function should return an expression, or none of them should. (inconsistent-return-statements)
python/qemu/qmp.py:245:4: C0111: Missing method docstring (missing-docstring)
python/qemu/qmp.py:249:4: C0111: Missing method docstring (missing-docstring)
python/qemu/qmp.py:252:4: C0111: Missing method docstring (missing-docstring)
python/qemu/qmp.py:255:4: C0111: Missing method docstring (missing-docstring)

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 python/qemu/qmp.py | 51 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 41 insertions(+), 10 deletions(-)

diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index 8c6c9847d0..f4e04a6683 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -1,5 +1,4 @@
-# QEMU Monitor Protocol Python class
-#
+""" QEMU Monitor Protocol Python class """
 # Copyright (C) 2009, 2010 Red Hat Inc.
 #
 # Authors:
@@ -15,22 +14,34 @@ import logging
 
 
 class QMPError(Exception):
-    pass
+    """
+    QMP base exception
+    """
 
 
 class QMPConnectError(QMPError):
-    pass
+    """
+    QMP connection exception
+    """
 
 
 class QMPCapabilitiesError(QMPError):
-    pass
+    """
+    QMP negotiate capabilities exception
+    """
 
 
 class QMPTimeoutError(QMPError):
-    pass
+    """
+    QMP timeout exception
+    """
 
 
-class QEMUMonitorProtocol(object):
+class QEMUMonitorProtocol:
+    """
+    Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP) and then
+    allow to handle commands and events.
+    """
 
     #: Logger object for debugging messages
     logger = logging.getLogger('QMP')
@@ -81,7 +92,7 @@ class QEMUMonitorProtocol(object):
         while True:
             data = self.__sockfile.readline()
             if not data:
-                return
+                return None
             resp = json.loads(data)
             if 'event' in resp:
                 self.logger.debug("<<< %s", resp)
@@ -132,7 +143,7 @@ class QEMUMonitorProtocol(object):
         """
         Connect to the QMP Monitor and perform capabilities negotiation.
 
-        @return QMP greeting dict
+        @return QMP greeting dict, or None if negotiate is false
         @raise OSError on socket connection errors
         @raise QMPConnectError if the greeting is not received
         @raise QMPCapabilitiesError if fails to negotiate capabilities
@@ -141,6 +152,7 @@ class QEMUMonitorProtocol(object):
         self.__sockfile = self.__sock.makefile()
         if negotiate:
             return self.__negotiate_capabilities()
+        return None
 
     def accept(self):
         """
@@ -169,7 +181,7 @@ class QEMUMonitorProtocol(object):
             self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8'))
         except OSError as err:
             if err.errno == errno.EPIPE:
-                return
+                return None
             raise err
         resp = self.__json_read()
         self.logger.debug("<<< %s", resp)
@@ -243,14 +255,33 @@ class QEMUMonitorProtocol(object):
         self.__events = []
 
     def close(self):
+        """
+        Close the socket and socket file.
+        """
         self.__sock.close()
         self.__sockfile.close()
 
     def settimeout(self, timeout):
+        """
+        Set the socket timeout.
+
+        @param timeout (float): timeout in seconds, or None.
+        @note This is a wrap around socket.settimeout
+        """
         self.__sock.settimeout(timeout)
 
     def get_sock_fd(self):
+        """
+        Get the socket file descriptor.
+
+        @return The file descriptor number.
+        """
         return self.__sock.fileno()
 
     def is_scm_available(self):
+        """
+        Check if the socket allows for SCM_RIGHTS.
+
+        @return True if SCM_RIGHTS is available, otherwise False.
+        """
         return self.__sock.family == socket.AF_UNIX
-- 
2.23.0

[PATCH v2 3/5] python/qemu: qmp: Make accept()'s timeout configurable

[Wainer dos Santos Moschetta]

Currently the timeout of QEMUMonitorProtocol.accept() is
hard-coded to 15.0 seconds. This added the parameter `timeout`
so the value can be configured by the user.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 python/qemu/qmp.py | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index f4e04a6683..0e07d80e2a 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -154,16 +154,23 @@ class QEMUMonitorProtocol:
             return self.__negotiate_capabilities()
         return None
 
-    def accept(self):
+    def accept(self, timeout=15.0):
         """
         Await connection from QMP Monitor and perform capabilities negotiation.
 
+        @param timeout: timeout in seconds (nonnegative float number, or
+                        None). The value passed will set the behavior of the
+                        underneath QMP socket as described in [1]. Default value
+                        is set to 15.0.
         @return QMP greeting dict
         @raise OSError on socket connection errors
         @raise QMPConnectError if the greeting is not received
         @raise QMPCapabilitiesError if fails to negotiate capabilities
+
+        [1]
+        https://docs.python.org/3/library/socket.html#socket.socket.settimeout
         """
-        self.__sock.settimeout(15)
+        self.__sock.settimeout(timeout)
         self.__sock, _ = self.__sock.accept()
         self.__sockfile = self.__sock.makefile()
         return self.__negotiate_capabilities()
-- 
2.23.0

[PATCH v2 4/5] python/qemu: qmp: Make QEMUMonitorProtocol a context manager

[Wainer dos Santos Moschetta]

This implement the __enter__ and __exit__ functions on
QEMUMonitorProtocol class so that it can be used on 'with'
statement and the resources will be free up on block end:

with QEMUMonitorProtocol(socket_path) as qmp:
    qmp.connect()
    qmp.command('query-status')

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
---
 python/qemu/qmp.py | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index 0e07d80e2a..486a566da0 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -139,6 +139,15 @@ class QEMUMonitorProtocol:
                 raise QMPConnectError("Error while reading from socket")
             self.__sock.settimeout(None)
 
+    def __enter__(self):
+        # Implement context manager enter function.
+        return self
+
+    def __exit__(self, exc_type, exc_value, exc_traceback):
+        # Implement context manager exit function.
+        self.close()
+        return False
+
     def connect(self, negotiate=True):
         """
         Connect to the QMP Monitor and perform capabilities negotiation.
@@ -265,8 +274,10 @@ class QEMUMonitorProtocol:
         """
         Close the socket and socket file.
         """
-        self.__sock.close()
-        self.__sockfile.close()
+        if self.__sock:
+            self.__sock.close()
+        if self.__sockfile:
+            self.__sockfile.close()
 
     def settimeout(self, timeout):
         """
-- 
2.23.0

[PATCH v2 5/5] python/qemu: qmp: Remove unnused attributes

[Wainer dos Santos Moschetta]

The `error` and `timeout` attributes in QEMUMonitorProtocol are
not used, so this delete them.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 python/qemu/qmp.py | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index 486a566da0..4b9a362240 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -45,10 +45,6 @@ class QEMUMonitorProtocol:
 
     #: Logger object for debugging messages
     logger = logging.getLogger('QMP')
-    #: Socket's error class
-    error = socket.error
-    #: Socket's timeout
-    timeout = socket.timeout
 
     def __init__(self, address, server=False):
         """
-- 
2.23.0

Re: [PATCH v2 3/5] python/qemu: qmp: Make accept()'s timeout configurable

[John Snow]

On 2/4/20 9:11 AM, Wainer dos Santos Moschetta wrote:
> Currently the timeout of QEMUMonitorProtocol.accept() is
> hard-coded to 15.0 seconds. This added the parameter `timeout`
> so the value can be configured by the user.
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: John Snow <jsnow@redhat.com>

> ---
>  python/qemu/qmp.py | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
> index f4e04a6683..0e07d80e2a 100644
> --- a/python/qemu/qmp.py
> +++ b/python/qemu/qmp.py
> @@ -154,16 +154,23 @@ class QEMUMonitorProtocol:
>              return self.__negotiate_capabilities()
>          return None
>  
> -    def accept(self):
> +    def accept(self, timeout=15.0):
>          """
>          Await connection from QMP Monitor and perform capabilities negotiation.
>  
> +        @param timeout: timeout in seconds (nonnegative float number, or
> +                        None). The value passed will set the behavior of the
> +                        underneath QMP socket as described in [1]. Default value
> +                        is set to 15.0.
>          @return QMP greeting dict
>          @raise OSError on socket connection errors
>          @raise QMPConnectError if the greeting is not received
>          @raise QMPCapabilitiesError if fails to negotiate capabilities
> +
> +        [1]
> +        https://docs.python.org/3/library/socket.html#socket.socket.settimeout
>          """
> -        self.__sock.settimeout(15)
> +        self.__sock.settimeout(timeout)
>          self.__sock, _ = self.__sock.accept()
>          self.__sockfile = self.__sock.makefile()
>          return self.__negotiate_capabilities()
> 

-- 
—js

Re: [PATCH v2 4/5] python/qemu: qmp: Make QEMUMonitorProtocol a context manager

[John Snow]

On 2/4/20 9:11 AM, Wainer dos Santos Moschetta wrote:
> This implement the __enter__ and __exit__ functions on
> QEMUMonitorProtocol class so that it can be used on 'with'
> statement and the resources will be free up on block end:
> 
> with QEMUMonitorProtocol(socket_path) as qmp:
>     qmp.connect()
>     qmp.command('query-status')
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> ---
>  python/qemu/qmp.py | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
> index 0e07d80e2a..486a566da0 100644
> --- a/python/qemu/qmp.py
> +++ b/python/qemu/qmp.py
> @@ -139,6 +139,15 @@ class QEMUMonitorProtocol:
>                  raise QMPConnectError("Error while reading from socket")
>              self.__sock.settimeout(None)
>  
> +    def __enter__(self):
> +        # Implement context manager enter function.
> +        return self
> +
> +    def __exit__(self, exc_type, exc_value, exc_traceback):
> +        # Implement context manager exit function.
> +        self.close()
> +        return False
> +
>      def connect(self, negotiate=True):
>          """
>          Connect to the QMP Monitor and perform capabilities negotiation.
> @@ -265,8 +274,10 @@ class QEMUMonitorProtocol:

Can we teach git to give better context for python?

What function is this part of?
(Rhetorical, do not shame me with answers.)

Default git configuration for python is bad. Can it be less bad?

> git diff
diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index 20e1e6ce86..aa73fc0b71 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -246,8 +246,10 @@ class QEMUMonitorProtocol(object):
         """
         foo
         """
-        self.__sock.close()
-        self.__sockfile.close()
+        if self.__sock:
+            self.__sock.close()
+        if self.__sockfile:
+            self.__sockfile.close()


Apparently if you edit .git/info/attributes to add this:
`*.py diff=python`

The diffs will look like this instead:

> git diff
diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index 20e1e6ce86..aa73fc0b71 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -246,8 +246,10 @@ def close(self):
         """
         foo
         """
-        self.__sock.close()
-        self.__sockfile.close()
+        if self.__sock:
+            self.__sock.close()
+        if self.__sockfile:
+            self.__sockfile.close()

     def settimeout(self, timeout):
         self.__sock.settimeout(timeout)


That's... better, but now it doesn't show the class anymore :(


>          """
>          Close the socket and socket file.
>          """
> -        self.__sock.close()
> -        self.__sockfile.close()
> +        if self.__sock:
> +            self.__sock.close()
> +        if self.__sockfile:
> +            self.__sockfile.close()
>  
>      def settimeout(self, timeout):
>          """
> 

You've designed it to be thrown away as a context object, but close() is
left as a public method you *could* call multiple times if you wanted to.

"Well, don't do that", but that was the nature of me asking why you were
guarding these values; and under what circumstances you expected to need
to guard them.

"Because they aren't defined on __enter__ and we need to not try to
close then on __exit__" is valid.

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [PATCH v2 0/5] python/qemu: qmp: Fix, delint and improvements

[Philippe Mathieu-Daudé]

On 2/4/20 3:11 PM, Wainer dos Santos Moschetta wrote:
> I started fixing an issue on exception handling which in some places
> currently use the deprecated (in Python 3.3) `socket.error`. Then I
> ended up delinting the module code and making some improvements.
> 
> Changes in v2:
> - Rebased to master. No conflicts.
> - Added docstring describing the allowed value of the new `timeout`
>    option in accept() (patch 03) [jsnow]
> - Set the new `timeout` option to 15.0 by default [philmd]
> 
> v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg667479.html
> 
> Git:
> - Tree: https://github.com/wainersm/qemu
> - Branch: python_qmp_sockets_error-v2
> 
> CI:
> - Travis (FAIL): https://travis-ci.org/wainersm/qemu/builds/645583812
>    Jobs failures aren't related with these changes
> 
> 
> Wainer dos Santos Moschetta (5):
>    python/qemu: qmp: Replace socket.error with OSError
>    python/qemu: Delint the qmp module
>    python/qemu: qmp: Make accept()'s timeout configurable
>    python/qemu: qmp: Make QEMUMonitorProtocol a context manager
>    python/qemu: qmp: Remove unnused attributes
> 
>   python/qemu/qmp.py | 97 +++++++++++++++++++++++++++++++++-------------
>   1 file changed, 71 insertions(+), 26 deletions(-)
> 

Patches 3 and 4 applied to my python-next tree:
https://gitlab.com/philmd/qemu/commits/python-next