Re: [PATCH 2/2] python/qemu: Change ConsoleSocket to optionally drain socket.

[Robert Foley]

On Thu, 16 Jul 2020 at 09:42, Alex Bennée <alex.bennee@linaro.org> wrote:
>
<snip>
> > +        self._drain_thread = None
> > +        socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
> > +        self.connect(address)
> > +        self._drain = drain
>
> We end up with two variables that represent the fact we have draining
> happening. Could we rationalise it into:
>
>   if drain:
>      self._drain_thread = self._thread_start()
>   else
>      self._drain_thread = None # if this is needed
>
> And then tests for:
>
>   if not self._drain:
>
> become
>
>   if self._drain_thread is None:

Good point, this is simpler.  Will update.

<snip>
> > +            if self._drain and self._drain_thread is not None:
> > +                thread, self._drain_thread = self._drain_thread, None
> Would self._drain ever not have self._drain_thread set?

No, I believe that if drain is set, it results in _drain_thread also being set.
This will be cleaned up once we drop the _drain.

>
> >                  thread.join()
> > +            socket.socket.close(self)
> <snip>
> > diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> > index 6769359766..62709d86e4 100644
> > --- a/python/qemu/machine.py
> > +++ b/python/qemu/machine.py
> > @@ -22,7 +22,6 @@ import logging
> >  import os
> >  import subprocess
> >  import shutil
> > -import socket
>
> FYI minor patch conflict here with master

OK, will rebase and fix this conflict.

Thanks & Regards,
-Rob
>
> >  import tempfile
> >  from typing import Optional, Type
> >  from types import TracebackType
> > @@ -591,12 +590,8 @@ class QEMUMachine:
> >          Returns a socket connected to the console
> >          """
> >          if self._console_socket is None:
> > -            if self._drain_console:
> > -                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)
> > -                self._console_socket.connect(self._console_address)
> > +            self._console_socket = console_socket.ConsoleSocket(
> > +                self._console_address,
> > +                file=self._console_log_path,
> > +                drain=self._drain_console)
> >          return self._console_socket
>
>
> --
> Alex Bennée