Hi,
Thanks for the detailed feedback! I will look at making these changes.
On Fri, 10 Jul 2020 at 15:20, John Snow <js...@redhat.com> wrote:
>
>
>
> On 7/7/20 3:08 AM, Alex Bennée wrote:
> > From: Robert Foley <robert.fo...@linaro.org>
> >
>
<snip>
> > + def recv(self, n=1, sleep_delay_s=0.1):
> > + """Return chars from in memory buffer"""
> > + start_time = time.time()
> > + while len(self._buffer) < n:
> > + time.sleep(sleep_delay_s)
> > + 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)])
> > + # 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
> > + # socket w/o our intervention.
> > + return chars.encode("latin1")
> > +
>
> console_socket.py:89:4: W0221: Parameters differ from overridden 'recv'
> method (arguments-differ)
>
> Seems pretty different from the asyncore.dispatcher recv method, is that
> intentional?
The intention is that the API be the same as asyncore.dispatcher recv.
The sleep_delay_s can be removed, and n is the same as buffer_size in
asyncore.dispatcher recv. Will plan to rename n -> buffer_size.
> https://github.com/python/cpython/blob/master/Lib/asyncore.py
>
<snip>
> > def __enter__(self):
> > return self
> > @@ -580,7 +591,11 @@ class QEMUMachine:
> > Returns a socket connected to the console
> > """
> > if self._console_socket is None:
> > - self._console_socket = socket.socket(socket.AF_UNIX,
> > - socket.SOCK_STREAM)
> > - self._console_socket.connect(self._console_address)
> > + if self._drain_console:
> > + self._console_socket = ConsoleSocket(self._console_address,
> > +
> > file=self._console_log_path)
>
> Needs one more space, but the line is already too long as-is.
>
> > + else:
> > + self._console_socket = socket.socket(socket.AF_UNIX,
> > + socket.SOCK_STREAM)
> > + self._console_socket.connect(self._console_address)
> > return self._console_socket
> >
>
> This makes the typing for _console_socket really tough ... but
> technically not a regression as the mypy code isn't merged yet.
>From the comment on mypy, I understand that we need to return a
constant type?
One option to provide a constant type is to simply always return
ConsoleSocket here.
A few changes would be needed inside of ConsoleSocket,
but essentially ConsoleSocket would handle the detail
of draining the console (or not), and thus eliminate this
if/else above reducing it to something like this:
self._console_socket = ConsoleSocket(self._console_address,
file=self._console_log_path,
drain=self._drain_console)
How does this sound?
Thanks & Regards,
-Rob
>
> --js
>
