| Message ID | 20230720130448.921356-3-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | python/machine: use socketpair() for console socket | expand |
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 :| >
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): 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
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(-)