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

[Philippe Mathieu-Daudé]

On 7/13/20 3:57 PM, John Snow wrote:
> 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 ...

I was looking for the patch that makes the python-next series rebase
to fail and now I see your comments :(

So we lost the race. I'll see what can still be merged.

Sorry it took so long due to the Avocado tests failing :(

OTOH I think it is time to declare the Python scripts need more
maintainers because we can't keep up. There are various scripts
and tests written in Python that missed the 5.1 freeze.

Cleber/Eduardo what do you think (about getting more maintainers
involved)?

Phil.

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