[PATCH 0/2] python/qemu: follow-up changes for ConsoleSocket

[PATCH 0/2] python/qemu: follow-up changes for ConsoleSocket

[Robert Foley]

This patch series introduces a few follow-up changes after the introduction of 
ConsoleSocket.

The first patch introduces cleanup changes for pylint and flake8.

The second patch allows machine.py to use a single type for the console_socket,
a ConsoleSocket.
Since machine.py will use ConsoleSocket for both the draining and non-draining
cases, we changed ConsoleSocket to handle the case where it does not drain the
socket at all and essentially behaves like a socket.

Robert Foley (2):
  python/qemu: Cleanup changes to ConsoleSocket
  python/qemu: Change ConsoleSocket to optionally drain socket.

 python/qemu/console_socket.py | 133 +++++++++++++++++++---------------
 python/qemu/machine.py        |  14 ++--
 python/qemu/pylintrc          |   2 +-
 3 files changed, 82 insertions(+), 67 deletions(-)

-- 
2.17.1

[PATCH 1/2] python/qemu: Cleanup changes to ConsoleSocket

[Robert Foley]

The changes to console_socket.py and machine.py are to
cleanup for pylint and flake8.

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 python/qemu/console_socket.py | 58 +++++++++++++++++------------------
 python/qemu/machine.py        |  7 +++--
 python/qemu/pylintrc          |  2 +-
 3 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
index 830cb7c628..6a746c1dbf 100644
--- a/python/qemu/console_socket.py
+++ b/python/qemu/console_socket.py
@@ -1,12 +1,9 @@
-#!/usr/bin/env python3
-#
-# This python module implements a ConsoleSocket object which is
-# designed always drain the socket itself, and place
-# the bytes into a in memory buffer for later processing.
-#
-# Optionally a file path can be passed in and we will also
-# dump the characters to this file for debug.
-#
+"""
+QEMU Console Socket Module:
+
+This python module implements a ConsoleSocket object,
+which can drain a socket and optionally dump the bytes to file.
+"""
 # Copyright 2020 Linaro
 #
 # Authors:
@@ -15,20 +12,27 @@
 # This code is licensed under the GPL version 2 or later.  See
 # the COPYING file in the top-level directory.
 #
+
 import asyncore
 import socket
 import threading
-import io
-import os
-import sys
 from collections import deque
 import time
-import traceback
+
 
 class ConsoleSocket(asyncore.dispatcher):
+    """
+    ConsoleSocket represents a socket attached to a char device.
 
+    Drains the socket and places the bytes into an in memory buffer
+    for later processing.
+
+    Optionally a file path can be passed in and we will also
+    dump the characters to this file for debugging purposes.
+    """
     def __init__(self, address, file=None):
         self._recv_timeout_sec = 300
+        self._sleep_time = 0.5
         self._buffer = deque()
         self._asyncore_thread = None
         self._sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
@@ -70,31 +74,28 @@ class ConsoleSocket(asyncore.dispatcher):
 
     def handle_read(self):
         """process arriving characters into in memory _buffer"""
-        try:
-            data = asyncore.dispatcher.recv(self, 1)
-            # latin1 is needed since there are some chars
-            # we are receiving that cannot be encoded to utf-8
-            # such as 0xe2, 0x80, 0xA6.
-            string = data.decode("latin1")
-        except:
-            print("Exception seen.")
-            traceback.print_exc()
-            return
+        data = asyncore.dispatcher.recv(self, 1)
+        # latin1 is needed since there are some chars
+        # we are receiving that cannot be encoded to utf-8
+        # such as 0xe2, 0x80, 0xA6.
+        string = data.decode("latin1")
         if self._logfile:
             self._logfile.write("{}".format(string))
             self._logfile.flush()
         for c in string:
             self._buffer.extend(c)
 
-    def recv(self, n=1, sleep_delay_s=0.1):
-        """Return chars from in memory buffer"""
+    def recv(self, buffer_size=1):
+        """Return chars from in memory buffer.
+           Maintains the same API as socket.socket.recv.
+        """
         start_time = time.time()
-        while len(self._buffer) < n:
-            time.sleep(sleep_delay_s)
+        while len(self._buffer) < buffer_size:
+            time.sleep(self._sleep_time)
             elapsed_sec = time.time() - start_time
             if elapsed_sec > self._recv_timeout_sec:
                 raise socket.timeout
-        chars = ''.join([self._buffer.popleft() for i in range(n)])
+        chars = ''.join([self._buffer.popleft() for i in range(buffer_size)])
         # We choose to use latin1 to remain consistent with
         # handle_read() and give back the same data as the user would
         # receive if they were reading directly from the
@@ -103,7 +104,6 @@ class ConsoleSocket(asyncore.dispatcher):
 
     def set_blocking(self):
         """Maintain compatibility with socket API"""
-        pass
 
     def settimeout(self, seconds):
         """Set current timeout on recv"""
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index c25f0b42cf..6769359766 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -26,7 +26,7 @@ import socket
 import tempfile
 from typing import Optional, Type
 from types import TracebackType
-from qemu.console_socket import ConsoleSocket
+from . import console_socket
 
 from . import qmp
 
@@ -592,8 +592,9 @@ class QEMUMachine:
         """
         if self._console_socket is None:
             if self._drain_console:
-                self._console_socket = ConsoleSocket(self._console_address,
-                                                    file=self._console_log_path)
+                self._console_socket = console_socket.ConsoleSocket(
+                    self._console_address,
+                    file=self._console_log_path)
             else:
                 self._console_socket = socket.socket(socket.AF_UNIX,
                                                      socket.SOCK_STREAM)
diff --git a/python/qemu/pylintrc b/python/qemu/pylintrc
index 5d6ae7367d..3f69205000 100644
--- a/python/qemu/pylintrc
+++ b/python/qemu/pylintrc
@@ -33,7 +33,7 @@ good-names=i,
            Run,
            _,
            fd,
-
+           c,
 [VARIABLES]
 
 [STRING]
-- 
2.17.1

[PATCH 2/2] python/qemu: Change ConsoleSocket to optionally drain socket.

[Robert Foley]

The primary purpose of this change is to clean up
machine.py's console_socket property to return a single type,
a ConsoleSocket.

ConsoleSocket now derives from a socket, which means that
in the default case (of not draining), machine.py
will see the same behavior as it did prior to ConsoleSocket.

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 python/qemu/console_socket.py | 81 +++++++++++++++++++++--------------
 python/qemu/machine.py        | 13 ++----
 2 files changed, 54 insertions(+), 40 deletions(-)

diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
index 6a746c1dbf..475de5b101 100644
--- a/python/qemu/console_socket.py
+++ b/python/qemu/console_socket.py
@@ -13,68 +13,76 @@ which can drain a socket and optionally dump the bytes to file.
 # the COPYING file in the top-level directory.
 #
 
-import asyncore
 import socket
 import threading
 from collections import deque
 import time
 
 
-class ConsoleSocket(asyncore.dispatcher):
+class ConsoleSocket(socket.socket):
     """
     ConsoleSocket represents a socket attached to a char device.
 
-    Drains the socket and places the bytes into an in memory buffer
-    for later processing.
+    Optionally (if drain==True), drains the socket and places the bytes
+    into an in memory buffer for later processing.
 
     Optionally a file path can be passed in and we will also
     dump the characters to this file for debugging purposes.
     """
-    def __init__(self, address, file=None):
+    def __init__(self, address, file=None, drain=False):
         self._recv_timeout_sec = 300
         self._sleep_time = 0.5
         self._buffer = deque()
-        self._asyncore_thread = None
-        self._sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
-        self._sock.connect(address)
+        self._drain_thread = None
+        socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
+        self.connect(address)
+        self._drain = drain
         self._logfile = None
         if file:
             self._logfile = open(file, "w")
-        asyncore.dispatcher.__init__(self, sock=self._sock)
         self._open = True
-        self._thread_start()
+        if drain:
+            self._thread_start()
+
+    def _drain_fn(self):
+        """Drains the socket and runs while the socket is open."""
+        while self._open:
+            try:
+                self._drain_socket()
+            except socket.timeout:
+                # The socket is expected to timeout since we set a
+                # short timeout to allow the thread to exit when
+                # self._open is set to False.
+                time.sleep(self._sleep_time)
 
     def _thread_start(self):
-        """Kick off a thread to wait on the asyncore.loop"""
-        if self._asyncore_thread is not None:
+        """Kick off a thread to drain the socket."""
+        if self._drain_thread is not None:
             return
-        self._asyncore_thread = threading.Thread(target=asyncore.loop,
-                                                 kwargs={'timeout':1})
-        self._asyncore_thread.daemon = True
-        self._asyncore_thread.start()
-
-    def handle_close(self):
-        """redirect close to base class"""
-        # Call the base class close, but not self.close() since
-        # handle_close() occurs in the context of the thread which
-        # self.close() attempts to join.
-        asyncore.dispatcher.close(self)
+        # Configure socket to not block and timeout.
+        # This allows our drain thread to not block
+        # on recieve and exit smoothly.
+        socket.socket.setblocking(self, 0)
+        socket.socket.settimeout(self, 1)
+        self._drain_thread = threading.Thread(target=self._drain_fn)
+        self._drain_thread.daemon = True
+        self._drain_thread.start()
 
     def close(self):
         """Close the base object and wait for the thread to terminate"""
         if self._open:
             self._open = False
-            asyncore.dispatcher.close(self)
-            if self._asyncore_thread is not None:
-                thread, self._asyncore_thread = self._asyncore_thread, None
+            if self._drain and self._drain_thread is not None:
+                thread, self._drain_thread = self._drain_thread, None
                 thread.join()
+            socket.socket.close(self)
             if self._logfile:
                 self._logfile.close()
                 self._logfile = None
 
-    def handle_read(self):
+    def _drain_socket(self):
         """process arriving characters into in memory _buffer"""
-        data = asyncore.dispatcher.recv(self, 1)
+        data = socket.socket.recv(self, 1)
         # latin1 is needed since there are some chars
         # we are receiving that cannot be encoded to utf-8
         # such as 0xe2, 0x80, 0xA6.
@@ -89,6 +97,9 @@ class ConsoleSocket(asyncore.dispatcher):
         """Return chars from in memory buffer.
            Maintains the same API as socket.socket.recv.
         """
+        if not self._drain:
+            # Not buffering the socket, pass thru to socket.
+            return socket.socket.recv(self, buffer_size)
         start_time = time.time()
         while len(self._buffer) < buffer_size:
             time.sleep(self._sleep_time)
@@ -102,9 +113,17 @@ class ConsoleSocket(asyncore.dispatcher):
         # socket w/o our intervention.
         return chars.encode("latin1")
 
-    def set_blocking(self):
-        """Maintain compatibility with socket API"""
+    def setblocking(self, value):
+        """When not draining we pass thru to the socket,
+           since when draining we control socket blocking.
+        """
+        if not self._drain:
+            socket.socket.setblocking(self, value)
 
     def settimeout(self, seconds):
-        """Set current timeout on recv"""
+        """When not draining we pass thru to the socket,
+           since when draining we control the timeout.
+        """
         self._recv_timeout_sec = seconds
+        if not self._drain:
+            socket.socket.settimeout(self, seconds)
diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 6769359766..62709d86e4 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -22,7 +22,6 @@ import logging
 import os
 import subprocess
 import shutil
-import socket
 import tempfile
 from typing import Optional, Type
 from types import TracebackType
@@ -591,12 +590,8 @@ class QEMUMachine:
         Returns a socket connected to the console
         """
         if self._console_socket is None:
-            if self._drain_console:
-                self._console_socket = console_socket.ConsoleSocket(
-                    self._console_address,
-                    file=self._console_log_path)
-            else:
-                self._console_socket = socket.socket(socket.AF_UNIX,
-                                                     socket.SOCK_STREAM)
-                self._console_socket.connect(self._console_address)
+            self._console_socket = console_socket.ConsoleSocket(
+                self._console_address,
+                file=self._console_log_path,
+                drain=self._drain_console)
         return self._console_socket
-- 
2.17.1

Re: [PATCH 1/2] python/qemu: Cleanup changes to ConsoleSocket

[Alex Bennée]

Robert Foley <robert.foley@linaro.org> writes:

> The changes to console_socket.py and machine.py are to
> cleanup for pylint and flake8.
>
> Signed-off-by: Robert Foley <robert.foley@linaro.org>
> ---
>  python/qemu/console_socket.py | 58 +++++++++++++++++------------------
>  python/qemu/machine.py        |  7 +++--
>  python/qemu/pylintrc          |  2 +-
>  3 files changed, 34 insertions(+), 33 deletions(-)
>
> diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
> index 830cb7c628..6a746c1dbf 100644
> --- a/python/qemu/console_socket.py
> +++ b/python/qemu/console_socket.py
> @@ -1,12 +1,9 @@
<snip>
> @@ -103,7 +104,6 @@ class ConsoleSocket(asyncore.dispatcher):
>  
>      def set_blocking(self):
>          """Maintain compatibility with socket API"""
> -        pass

Hmm shouldn't this be with the change in 2/2 because I thought you
needed a "pass" for an empty function in python?

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH 1/2] python/qemu: Cleanup changes to ConsoleSocket

[Robert Foley]

On Thu, 16 Jul 2020 at 07:07, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Robert Foley <robert.foley@linaro.org> writes:
>
> > The changes to console_socket.py and machine.py are to
> > cleanup for pylint and flake8.
> >
> > Signed-off-by: Robert Foley <robert.foley@linaro.org>
> > ---
> >  python/qemu/console_socket.py | 58 +++++++++++++++++------------------
> >  python/qemu/machine.py        |  7 +++--
> >  python/qemu/pylintrc          |  2 +-
> >  3 files changed, 34 insertions(+), 33 deletions(-)
> >
> > diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
> > index 830cb7c628..6a746c1dbf 100644
> > --- a/python/qemu/console_socket.py
> > +++ b/python/qemu/console_socket.py
> > @@ -1,12 +1,9 @@
> <snip>
> > @@ -103,7 +104,6 @@ class ConsoleSocket(asyncore.dispatcher):
> >
> >      def set_blocking(self):
> >          """Maintain compatibility with socket API"""
> > -        pass
>
> Hmm shouldn't this be with the change in 2/2 because I thought you
> needed a "pass" for an empty function in python?

Thanks for the review !

Sure, I can move this change to 2/2.  Probably makes more sense there
since we're changing this function there too.

This change was one of the suggestions from John Snow.
He pointed out that the pass is not needed here since the docstring takes
the role of the function body.

Thanks & Regards,
-Rob

>
> Otherwise:
>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>
> --
> Alex Bennée

Re: [PATCH 1/2] python/qemu: Cleanup changes to ConsoleSocket

[Alex Bennée]

Robert Foley <robert.foley@linaro.org> writes:

> On Thu, 16 Jul 2020 at 07:07, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Robert Foley <robert.foley@linaro.org> writes:
>>
>> > The changes to console_socket.py and machine.py are to
>> > cleanup for pylint and flake8.
>> >
>> > Signed-off-by: Robert Foley <robert.foley@linaro.org>
>> > ---
>> >  python/qemu/console_socket.py | 58 +++++++++++++++++------------------
>> >  python/qemu/machine.py        |  7 +++--
>> >  python/qemu/pylintrc          |  2 +-
>> >  3 files changed, 34 insertions(+), 33 deletions(-)
>> >
>> > diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
>> > index 830cb7c628..6a746c1dbf 100644
>> > --- a/python/qemu/console_socket.py
>> > +++ b/python/qemu/console_socket.py
>> > @@ -1,12 +1,9 @@
>> <snip>
>> > @@ -103,7 +104,6 @@ class ConsoleSocket(asyncore.dispatcher):
>> >
>> >      def set_blocking(self):
>> >          """Maintain compatibility with socket API"""
>> > -        pass
>>
>> Hmm shouldn't this be with the change in 2/2 because I thought you
>> needed a "pass" for an empty function in python?
>
> Thanks for the review !
>
> Sure, I can move this change to 2/2.  Probably makes more sense there
> since we're changing this function there too.
>
> This change was one of the suggestions from John Snow.
> He pointed out that the pass is not needed here since the docstring takes
> the role of the function body.

Ahh I did not know you could do that... I'll defer to John's superior
python knowledge.

>
> Thanks & Regards,
> -Rob
>
>>
>> Otherwise:
>>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> --
>> Alex Bennée


-- 
Alex Bennée

Re: [PATCH 2/2] python/qemu: Change ConsoleSocket to optionally drain socket.

[Alex Bennée]

Robert Foley <robert.foley@linaro.org> writes:

> The primary purpose of this change is to clean up
> machine.py's console_socket property to return a single type,
> a ConsoleSocket.
>
> ConsoleSocket now derives from a socket, which means that
> in the default case (of not draining), machine.py
> will see the same behavior as it did prior to ConsoleSocket.
>
> Signed-off-by: Robert Foley <robert.foley@linaro.org>
> ---
>  python/qemu/console_socket.py | 81 +++++++++++++++++++++--------------
>  python/qemu/machine.py        | 13 ++----
>  2 files changed, 54 insertions(+), 40 deletions(-)
>
> diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
> index 6a746c1dbf..475de5b101 100644
> --- a/python/qemu/console_socket.py
> +++ b/python/qemu/console_socket.py
> @@ -13,68 +13,76 @@ which can drain a socket and optionally dump the bytes to file.
>  # the COPYING file in the top-level directory.
>  #
>  
> -import asyncore
>  import socket
>  import threading
>  from collections import deque
>  import time
>  
>  
> -class ConsoleSocket(asyncore.dispatcher):
> +class ConsoleSocket(socket.socket):
>      """
>      ConsoleSocket represents a socket attached to a char device.
>  
> -    Drains the socket and places the bytes into an in memory buffer
> -    for later processing.
> +    Optionally (if drain==True), drains the socket and places the bytes
> +    into an in memory buffer for later processing.
>  
>      Optionally a file path can be passed in and we will also
>      dump the characters to this file for debugging purposes.
>      """
> -    def __init__(self, address, file=None):
> +    def __init__(self, address, file=None, drain=False):
>          self._recv_timeout_sec = 300
>          self._sleep_time = 0.5
>          self._buffer = deque()
> -        self._asyncore_thread = None
> -        self._sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
> -        self._sock.connect(address)
> +        self._drain_thread = None
> +        socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
> +        self.connect(address)
> +        self._drain = drain

We end up with two variables that represent the fact we have draining
happening. Could we rationalise it into:

  if drain:
     self._drain_thread = self._thread_start()
  else
     self._drain_thread = None # if this is needed

And then tests for:

  if not self._drain:

become

  if self._drain_thread is None:

>          self._logfile = None
>          if file:
>              self._logfile = open(file, "w")
> -        asyncore.dispatcher.__init__(self, sock=self._sock)
>          self._open = True
> -        self._thread_start()
> +        if drain:
> +            self._thread_start()
> +
> +    def _drain_fn(self):
> +        """Drains the socket and runs while the socket is open."""
> +        while self._open:
> +            try:
> +                self._drain_socket()
> +            except socket.timeout:
> +                # The socket is expected to timeout since we set a
> +                # short timeout to allow the thread to exit when
> +                # self._open is set to False.
> +                time.sleep(self._sleep_time)
>  
>      def _thread_start(self):
> -        """Kick off a thread to wait on the asyncore.loop"""
> -        if self._asyncore_thread is not None:
> +        """Kick off a thread to drain the socket."""
> +        if self._drain_thread is not None:
>              return
> -        self._asyncore_thread = threading.Thread(target=asyncore.loop,
> -                                                 kwargs={'timeout':1})
> -        self._asyncore_thread.daemon = True
> -        self._asyncore_thread.start()
> -
> -    def handle_close(self):
> -        """redirect close to base class"""
> -        # Call the base class close, but not self.close() since
> -        # handle_close() occurs in the context of the thread which
> -        # self.close() attempts to join.
> -        asyncore.dispatcher.close(self)
> +        # Configure socket to not block and timeout.
> +        # This allows our drain thread to not block
> +        # on recieve and exit smoothly.
> +        socket.socket.setblocking(self, 0)
> +        socket.socket.settimeout(self, 1)
> +        self._drain_thread = threading.Thread(target=self._drain_fn)
> +        self._drain_thread.daemon = True
> +        self._drain_thread.start()
>  
>      def close(self):
>          """Close the base object and wait for the thread to terminate"""
>          if self._open:
>              self._open = False
> -            asyncore.dispatcher.close(self)
> -            if self._asyncore_thread is not None:
> -                thread, self._asyncore_thread = self._asyncore_thread, None
> +            if self._drain and self._drain_thread is not None:
> +                thread, self._drain_thread = self._drain_thread, None

Would self._drain ever not have self._drain_thread set?

>                  thread.join()
> +            socket.socket.close(self)
<snip>
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index 6769359766..62709d86e4 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -22,7 +22,6 @@ import logging
>  import os
>  import subprocess
>  import shutil
> -import socket

FYI minor patch conflict here with master

>  import tempfile
>  from typing import Optional, Type
>  from types import TracebackType
> @@ -591,12 +590,8 @@ class QEMUMachine:
>          Returns a socket connected to the console
>          """
>          if self._console_socket is None:
> -            if self._drain_console:
> -                self._console_socket = console_socket.ConsoleSocket(
> -                    self._console_address,
> -                    file=self._console_log_path)
> -            else:
> -                self._console_socket = socket.socket(socket.AF_UNIX,
> -                                                     socket.SOCK_STREAM)
> -                self._console_socket.connect(self._console_address)
> +            self._console_socket = console_socket.ConsoleSocket(
> +                self._console_address,
> +                file=self._console_log_path,
> +                drain=self._drain_console)
>          return self._console_socket


-- 
Alex Bennée

Re: [PATCH 1/2] python/qemu: Cleanup changes to ConsoleSocket

[Philippe Mathieu-Daudé]

On 7/16/20 3:13 PM, Alex Bennée wrote:
> 
> Robert Foley <robert.foley@linaro.org> writes:
> 
>> On Thu, 16 Jul 2020 at 07:07, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>
>>>
>>> Robert Foley <robert.foley@linaro.org> writes:
>>>
>>>> The changes to console_socket.py and machine.py are to
>>>> cleanup for pylint and flake8.
>>>>
>>>> Signed-off-by: Robert Foley <robert.foley@linaro.org>
>>>> ---
>>>>  python/qemu/console_socket.py | 58 +++++++++++++++++------------------
>>>>  python/qemu/machine.py        |  7 +++--
>>>>  python/qemu/pylintrc          |  2 +-
>>>>  3 files changed, 34 insertions(+), 33 deletions(-)
>>>>
>>>> diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
>>>> index 830cb7c628..6a746c1dbf 100644
>>>> --- a/python/qemu/console_socket.py
>>>> +++ b/python/qemu/console_socket.py
>>>> @@ -1,12 +1,9 @@
>>> <snip>
>>>> @@ -103,7 +104,6 @@ class ConsoleSocket(asyncore.dispatcher):
>>>>
>>>>      def set_blocking(self):
>>>>          """Maintain compatibility with socket API"""
>>>> -        pass
>>>
>>> Hmm shouldn't this be with the change in 2/2 because I thought you
>>> needed a "pass" for an empty function in python?
>>
>> Thanks for the review !
>>
>> Sure, I can move this change to 2/2.  Probably makes more sense there
>> since we're changing this function there too.
>>
>> This change was one of the suggestions from John Snow.
>> He pointed out that the pass is not needed here since the docstring takes
>> the role of the function body.
> 
> Ahh I did not know you could do that... I'll defer to John's superior
> python knowledge.

Yes, a function with a comment is not function with an empty body...

¯\_(ツ)_/¯

When you have doubt about Python, I recommend you Gary Bernhardt's talk
from CodeMash 2012.

> 
>>
>> Thanks & Regards,
>> -Rob
>>
>>>
>>> Otherwise:
>>>
>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>>
>>> --
>>> Alex Bennée
> 
> 

Re: [PATCH 2/2] python/qemu: Change ConsoleSocket to optionally drain socket.

[Robert Foley]

On Thu, 16 Jul 2020 at 09:42, Alex Bennée <alex.bennee@linaro.org> wrote:
>
<snip>
> > +        self._drain_thread = None
> > +        socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
> > +        self.connect(address)
> > +        self._drain = drain
>
> We end up with two variables that represent the fact we have draining
> happening. Could we rationalise it into:
>
>   if drain:
>      self._drain_thread = self._thread_start()
>   else
>      self._drain_thread = None # if this is needed
>
> And then tests for:
>
>   if not self._drain:
>
> become
>
>   if self._drain_thread is None:

Good point, this is simpler.  Will update.

<snip>
> > +            if self._drain and self._drain_thread is not None:
> > +                thread, self._drain_thread = self._drain_thread, None
> Would self._drain ever not have self._drain_thread set?

No, I believe that if drain is set, it results in _drain_thread also being set.
This will be cleaned up once we drop the _drain.

>
> >                  thread.join()
> > +            socket.socket.close(self)
> <snip>
> > diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> > index 6769359766..62709d86e4 100644
> > --- a/python/qemu/machine.py
> > +++ b/python/qemu/machine.py
> > @@ -22,7 +22,6 @@ import logging
> >  import os
> >  import subprocess
> >  import shutil
> > -import socket
>
> FYI minor patch conflict here with master

OK, will rebase and fix this conflict.

Thanks & Regards,
-Rob
>
> >  import tempfile
> >  from typing import Optional, Type
> >  from types import TracebackType
> > @@ -591,12 +590,8 @@ class QEMUMachine:
> >          Returns a socket connected to the console
> >          """
> >          if self._console_socket is None:
> > -            if self._drain_console:
> > -                self._console_socket = console_socket.ConsoleSocket(
> > -                    self._console_address,
> > -                    file=self._console_log_path)
> > -            else:
> > -                self._console_socket = socket.socket(socket.AF_UNIX,
> > -                                                     socket.SOCK_STREAM)
> > -                self._console_socket.connect(self._console_address)
> > +            self._console_socket = console_socket.ConsoleSocket(
> > +                self._console_address,
> > +                file=self._console_log_path,
> > +                drain=self._drain_console)
> >          return self._console_socket
>
>
> --
> Alex Bennée