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

[PATCH 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.

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

CI:
- Travis (FAIL): https://travis-ci.org/wainersm/qemu/builds/629772066
  Failure not related with this series. Even QEMU master branch is
  failing to build when `--without-default-devices --disable-user`.

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 | 91 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 65 insertions(+), 26 deletions(-)

-- 
2.23.0

[PATCH 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>
---
 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 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>
---
 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 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 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>
---
 python/qemu/qmp.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index f4e04a6683..914b8c6774 100644
--- a/python/qemu/qmp.py
+++ b/python/qemu/qmp.py
@@ -154,16 +154,17 @@ class QEMUMonitorProtocol:
             return self.__negotiate_capabilities()
         return None
 
-    def accept(self):
+    def accept(self, timeout=15):
         """
         Await connection from QMP Monitor and perform capabilities negotiation.
 
+        @param timeout (float): timeout in seconds. Default is 15.
         @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
         """
-        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 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 914b8c6774..6d55f53595 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.
@@ -259,8 +268,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 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>
---
 python/qemu/qmp.py | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
index 6d55f53595..cddb94bb3c 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 3/5] python/qemu: qmp: Make accept()'s timeout configurable

[Philippe Mathieu-Daudé]

On 12/27/19 2:40 PM, Wainer dos Santos Moschetta wrote:
> Currently the timeout of QEMUMonitorProtocol.accept() is
> hard-coded to 15 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>
> ---
>   python/qemu/qmp.py | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
> index f4e04a6683..914b8c6774 100644
> --- a/python/qemu/qmp.py
> +++ b/python/qemu/qmp.py
> @@ -154,16 +154,17 @@ class QEMUMonitorProtocol:
>               return self.__negotiate_capabilities()
>           return None
>   
> -    def accept(self):
> +    def accept(self, timeout=15):
>           """
>           Await connection from QMP Monitor and perform capabilities negotiation.
>   
> +        @param timeout (float): timeout in seconds. Default is 15.

Maybe name with unit: 'timeout_s'.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>           @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
>           """
> -        self.__sock.settimeout(15)
> +        self.__sock.settimeout(timeout)
>           self.__sock, _ = self.__sock.accept()
>           self.__sockfile = self.__sock.makefile()
>           return self.__negotiate_capabilities()
> 

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

[John Snow]

On 12/27/19 8:40 AM, Wainer dos Santos Moschetta wrote:
> 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>

(Are there other clients that of this code that would still be using the
old exception?)

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

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

[John Snow]

On 12/27/19 8:40 AM, Wainer dos Santos Moschetta wrote:
> 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>

ACK, thanks for the delinting work here. It will be nice to get our
python code in such a shape that it *could* be packaged if we want to.

Like, if we wanted to publish a standalone QMP client, for instance...

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
> 

-- 
—js

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

[John Snow]

On 12/27/19 8:40 AM, Wainer dos Santos Moschetta wrote:
> Currently the timeout of QEMUMonitorProtocol.accept() is
> hard-coded to 15 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>
> ---
>  python/qemu/qmp.py | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
> index f4e04a6683..914b8c6774 100644
> --- a/python/qemu/qmp.py
> +++ b/python/qemu/qmp.py
> @@ -154,16 +154,17 @@ class QEMUMonitorProtocol:
>              return self.__negotiate_capabilities()
>          return None
>  
> -    def accept(self):
> +    def accept(self, timeout=15):
>          """
>          Await connection from QMP Monitor and perform capabilities negotiation.
>  
> +        @param timeout (float): timeout in seconds. Default is 15.

What happens if I pass -1 or 0? Please document the valid range for this
parameter.

>          @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
>          """
> -        self.__sock.settimeout(15)
> +        self.__sock.settimeout(timeout)
>          self.__sock, _ = self.__sock.accept()
>          self.__sockfile = self.__sock.makefile()
>          return self.__negotiate_capabilities()
> 

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

[John Snow]

On 12/27/19 8:41 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 914b8c6774..6d55f53595 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.
> @@ -259,8 +268,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()

Not evident on cold read: does self.close() change self.__sock and
self.__sockfile such that they are false-ish?

close() I suspect might need to actually unset the __sock and __sockfile
fields.

>  
>      def settimeout(self, timeout):
>          """
> 

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

[John Snow]

On 12/27/19 8:41 AM, Wainer dos Santos Moschetta wrote:
> 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 6d55f53595..cddb94bb3c 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):
>          """
> 

-- 
—js

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

[Wainer dos Santos Moschetta]

On 1/8/20 10:23 PM, John Snow wrote:
>
> On 12/27/19 8:41 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 914b8c6774..6d55f53595 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.
>> @@ -259,8 +268,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()
> Not evident on cold read: does self.close() change self.__sock and
> self.__sockfile such that they are false-ish?


Because self.__exit__() calls self.close() even when a runtime exception 
raises, there isn't any guarantee that self.__sockfile and self.__sock 
were initialized. That's the reason why I added those guards.


>
> close() I suspect might need to actually unset the __sock and __sockfile
> fields.

The QEMUMonitorProtocol object is designed to be disposed after close() 
being called. So I don't see any reason to unset those fields. Unless I 
am missing something here...

Thanks for the comments!

- Wainer

>
>>   
>>       def settimeout(self, timeout):
>>           """
>>

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

[Philippe Mathieu-Daudé]

On 1/9/20 1:18 AM, John Snow wrote:
> On 12/27/19 8:40 AM, Wainer dos Santos Moschetta wrote:
>> Currently the timeout of QEMUMonitorProtocol.accept() is
>> hard-coded to 15 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>
>> ---
>>   python/qemu/qmp.py | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py
>> index f4e04a6683..914b8c6774 100644
>> --- a/python/qemu/qmp.py
>> +++ b/python/qemu/qmp.py
>> @@ -154,16 +154,17 @@ class QEMUMonitorProtocol:
>>               return self.__negotiate_capabilities()
>>           return None
>>   
>> -    def accept(self):
>> +    def accept(self, timeout=15):

Since it is a float, it is clearer to use 15.0 as default IMHO.

>>           """
>>           Await connection from QMP Monitor and perform capabilities negotiation.
>>   
>> +        @param timeout (float): timeout in seconds. Default is 15.
> 
> What happens if I pass -1 or 0? Please document the valid range for this
> parameter.
> 
>>           @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
>>           """
>> -        self.__sock.settimeout(15)
>> +        self.__sock.settimeout(timeout)
>>           self.__sock, _ = self.__sock.accept()
>>           self.__sockfile = self.__sock.makefile()
>>           return self.__negotiate_capabilities()
>>
> 
> 

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

[Philippe Mathieu-Daudé]

On 12/27/19 2:40 PM, Wainer dos Santos Moschetta wrote:
> 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>
> ---
>   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
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

[Philippe Mathieu-Daudé]

On 12/27/19 2:41 PM, Wainer dos Santos Moschetta wrote:
> The `error` and `timeout` attributes in QEMUMonitorProtocol are
> not used, so this delete them.
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@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 6d55f53595..cddb94bb3c 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):
>           """
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

[Philippe Mathieu-Daudé]

On 12/27/19 2:40 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.
> 
> Git:
> - Tree: https://github.com/wainersm/qemu
> - Branch: python_qmp_sockets_error
> 
> CI:
> - Travis (FAIL): https://travis-ci.org/wainersm/qemu/builds/629772066
>    Failure not related with this series. Even QEMU master branch is
>    failing to build when `--without-default-devices --disable-user`.
> 
> 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 | 91 +++++++++++++++++++++++++++++++++-------------
>   1 file changed, 65 insertions(+), 26 deletions(-)
> 

Thanks, applied patches 1, 2 and 5 to my python-next tree:
https://gitlab.com/philmd/qemu/commits/python-next

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

[Wainer dos Santos Moschetta]

On 1/30/20 8:41 PM, Philippe Mathieu-Daudé wrote:
> On 12/27/19 2:40 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.
>>
>> Git:
>> - Tree: https://github.com/wainersm/qemu
>> - Branch: python_qmp_sockets_error
>>
>> CI:
>> - Travis (FAIL): https://travis-ci.org/wainersm/qemu/builds/629772066
>>    Failure not related with this series. Even QEMU master branch is
>>    failing to build when `--without-default-devices --disable-user`.
>>
>> 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 | 91 +++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 65 insertions(+), 26 deletions(-)
>>
>
> Thanks, applied patches 1, 2 and 5 to my python-next tree:
> https://gitlab.com/philmd/qemu/commits/python-next
>
>

Great, I was going to ask you that. Patches 3 and 4 are likely to need a 
respin.

Thanks Philippe!