diff mbox series

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

Message ID 20200715204814.2630-2-robert.foley@linaro.org
State New
Headers show
Series python/qemu: follow-up changes for ConsoleSocket | expand

Commit Message

Robert Foley July 15, 2020, 8:48 p.m. UTC
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(-)

Comments

Alex Bennée July 16, 2020, 11:07 a.m. UTC | #1
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>
Robert Foley July 16, 2020, 12:50 p.m. UTC | #2
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
Alex Bennée July 16, 2020, 1:13 p.m. UTC | #3
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
Philippe Mathieu-Daudé July 16, 2020, 1:42 p.m. UTC | #4
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
> 
>
diff mbox series

Patch

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]