[2/4] python/console_socket: accept existing FD in initializer

Useful if we want to use ConsoleSocket() for a socket created by
socketpair().

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/machine/console_socket.py | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

On Thu, Jul 20, 2023 at 09:04:46AM -0400, John Snow wrote:
> Useful if we want to use ConsoleSocket() for a socket created by
> socketpair().
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/machine/console_socket.py | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/python/qemu/machine/console_socket.py b/python/qemu/machine/console_socket.py
> index 4e28ba9bb2..42bfa12411 100644
> --- a/python/qemu/machine/console_socket.py
> +++ b/python/qemu/machine/console_socket.py
> @@ -17,7 +17,7 @@
>  import socket
>  import threading
>  import time
> -from typing import Deque, Optional
> +from typing import Deque, Optional, Union
>  
>  
>  class ConsoleSocket(socket.socket):
> @@ -30,13 +30,16 @@ class ConsoleSocket(socket.socket):
>      Optionally a file path can be passed in and we will also
>      dump the characters to this file for debugging purposes.
>      """
> -    def __init__(self, address: str, file: Optional[str] = None,
> +    def __init__(self, address: Union[str, int], file: Optional[str] = None,
>                   drain: bool = False):

IMHO calling the pre-opened FD an "address" is pushing the
interpretation a bit.

It also makes the behaviour non-obvious from a caller. Seeing a
caller, you have to work backwards to find out what type it has,
to figure out the semantics of the method call.

IOW, I'd prefer to see

   address: Optional[str], sock_fd: Optional[int]

and then just do a check

   if (address is not None and sock_fd is not None) or
      (address is None and sock_fd is None):
      raise Exception("either 'address' or 'sock_fd' is required")

thus when you see

   ConsoleSocket(sock_fd=xxx)

it is now obvious it has different behaviour to

   ConsoleSocket(address=yyy)


>          self._recv_timeout_sec = 300.0
>          self._sleep_time = 0.5
>          self._buffer: Deque[int] = deque()
> -        socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
> -        self.connect(address)
> +        if isinstance(address, str):
> +            socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
> +            self.connect(address)
> +        else:
> +            socket.socket.__init__(self, fileno=address)
>          self._logfile = None
>          if file:
>              # pylint: disable=consider-using-with
> -- 
> 2.41.0
> 

With regards,
Daniel

On Thu, Jul 20, 2023 at 10:01 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Jul 20, 2023 at 09:04:46AM -0400, John Snow wrote:
> > Useful if we want to use ConsoleSocket() for a socket created by
> > socketpair().
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  python/qemu/machine/console_socket.py | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/python/qemu/machine/console_socket.py b/python/qemu/machine/console_socket.py
> > index 4e28ba9bb2..42bfa12411 100644
> > --- a/python/qemu/machine/console_socket.py
> > +++ b/python/qemu/machine/console_socket.py
> > @@ -17,7 +17,7 @@
> >  import socket
> >  import threading
> >  import time
> > -from typing import Deque, Optional
> > +from typing import Deque, Optional, Union
> >
> >
> >  class ConsoleSocket(socket.socket):
> > @@ -30,13 +30,16 @@ class ConsoleSocket(socket.socket):
> >      Optionally a file path can be passed in and we will also
> >      dump the characters to this file for debugging purposes.
> >      """
> > -    def __init__(self, address: str, file: Optional[str] = None,
> > +    def __init__(self, address: Union[str, int], file: Optional[str] = None,
> >                   drain: bool = False):
>
> IMHO calling the pre-opened FD an "address" is pushing the
> interpretation a bit.
>

You're right.

> It also makes the behaviour non-obvious from a caller. Seeing a
> caller, you have to work backwards to find out what type it has,
> to figure out the semantics of the method call.
>
> IOW, I'd prefer to see
>
>    address: Optional[str], sock_fd: Optional[int]
>
> and then just do a check
>
>    if (address is not None and sock_fd is not None) or
>       (address is None and sock_fd is None):
>       raise Exception("either 'address' or 'sock_fd' is required")
>
> thus when you see
>
>    ConsoleSocket(sock_fd=xxx)
>
> it is now obvious it has different behaviour to
>
>    ConsoleSocket(address=yyy)
>

Yeah, that's just a little harder to type - in the sense that it
appears as though you could omit either argument by just observing the
signature. One thing I like about "one mandatory argument that takes
many forms" is that it's obvious you need to give it *something* from
the signature.

You're right that the name is now very silly, though.

The "obvious it has different behavior" is a good argument, I'll change it.

--js

>
> >          self._recv_timeout_sec = 300.0
> >          self._sleep_time = 0.5
> >          self._buffer: Deque[int] = deque()
> > -        socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
> > -        self.connect(address)
> > +        if isinstance(address, str):
> > +            socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
> > +            self.connect(address)
> > +        else:
> > +            socket.socket.__init__(self, fileno=address)
> >          self._logfile = None
> >          if file:
> >              # pylint: disable=consider-using-with
> > --
> > 2.41.0
> >
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>