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

[John Snow]

On 7/11/20 12:15 PM, Robert Foley wrote:
> Hi,
> Thanks for the detailed feedback!  I will look at making these changes.
> 

Sorry that it came so late ...

> 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?
> 

It keeps the API simpler to do that, yeah.

> 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
> 

That would be one way, but I'm not sure how hard it will be because
other clients in the acceptance tests use the socket.makefile() routine
-- does that work for the asyncore object?

The other way would be to offer a .drain_console() style routine that
takes the existing console socket object, wraps it in the asyncore
dispatcher, and returns a new object with its own type and behaviors.

It looks like asyncore is deprecated already, so isolating it into its
own method would make it a bit easier to replace in the future, I'd think.

(I was working on prototyping something for you, but hadn't worked on it
much over the weekend!)

Thanks,
--js