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 <address@hidden>
> ---
>  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 <address@hidden>