Re: [PULL 10/41] python/qemu: Add ConsoleSocket for optional use in QEMUMachine

[Robert Foley]

Hi,
Thanks for the detailed feedback!  I will look at making these changes.

On Fri, 10 Jul 2020 at 15:20, John Snow <jsnow@redhat.com> wrote:
>
>
>
> On 7/7/20 3:08 AM, Alex Bennée wrote:
> > From: Robert Foley <robert.foley@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
>