Re: [PATCH v2 4/6] python/machine: use socketpair() for console connections

[Ani Sinha]

> On 25-Jul-2023, at 11:33 PM, John Snow <jsnow@redhat.com> wrote:
> 
> Create a socketpair for the console output. This should help eliminate
> race conditions around console text early in the boot process that might
> otherwise have been dropped on the floor before being able to connect to
> QEMU under "server,nowait".
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Thanks for doing this. I recall we spoke about this late last year in the 
context of fixing my bios-bits avocado test and adding a console output there.
Except the concern below,

Reviewed-by: Ani Sinha <anisinha@redhat.com>


> ---
> python/qemu/machine/machine.py | 30 +++++++++++++++++++++++++++---
> 1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> index 26f0fb8a81..09f214c95c 100644
> --- a/python/qemu/machine/machine.py
> +++ b/python/qemu/machine/machine.py
> @@ -159,6 +159,8 @@ def __init__(self,
> 
>         self._name = name or f"{id(self):x}"
>         self._sock_pair: Optional[Tuple[socket.socket, socket.socket]] = None
> +        self._cons_sock_pair: Optional[
> +            Tuple[socket.socket, socket.socket]] = None
>         self._temp_dir: Optional[str] = None
>         self._base_temp_dir = base_temp_dir
>         self._sock_dir = sock_dir
> @@ -315,8 +317,9 @@ def _base_args(self) -> List[str]:
>         for _ in range(self._console_index):
>             args.extend(['-serial', 'null'])
>         if self._console_set:
> -            chardev = ('socket,id=console,path=%s,server=on,wait=off' %
> -                       self._console_address)
> +            assert self._cons_sock_pair is not None
> +            fd = self._cons_sock_pair[0].fileno()
> +            chardev = f"socket,id=console,fd={fd}"
>             args.extend(['-chardev', chardev])
>             if self._console_device_type is None:
>                 args.extend(['-serial', 'chardev:console'])
> @@ -351,6 +354,10 @@ def _pre_launch(self) -> None:
>                 nickname=self._name
>             )
> 
> +        if self._console_set:
> +            self._cons_sock_pair = socket.socketpair()
> +            os.set_inheritable(self._cons_sock_pair[0].fileno(), True)
> +
>         # NOTE: Make sure any opened resources are *definitely* freed in
>         # _post_shutdown()!
>         # pylint: disable=consider-using-with
> @@ -368,6 +375,9 @@ def _pre_launch(self) -> None:
>     def _post_launch(self) -> None:
>         if self._sock_pair:
>             self._sock_pair[0].close()
> +        if self._cons_sock_pair:
> +            self._cons_sock_pair[0].close()
> +
>         if self._qmp_connection:
>             if self._sock_pair:
>                 self._qmp.connect()
> @@ -518,6 +528,11 @@ def _early_cleanup(self) -> None:
>             self._console_socket.close()
>             self._console_socket = None
> 
> +        if self._cons_sock_pair:
> +            self._cons_sock_pair[0].close()
> +            self._cons_sock_pair[1].close()
> +            self._cons_sock_pair = None
> +
>     def _hard_shutdown(self) -> None:
>         """
>         Perform early cleanup, kill the VM, and wait for it to terminate.
> @@ -878,10 +893,19 @@ def console_socket(self) -> socket.socket:
>         Returns a socket connected to the console
>         """
>         if self._console_socket is None:
> +            if not self._console_set:
> +                raise QEMUMachineError(
> +                    "Attempt to access console socket with no connection")
> +            assert self._cons_sock_pair is not None
> +            # os.dup() is used here for sock_fd because otherwise we'd
> +            # have two rich python socket objects that would each try to
> +            # close the same underlying fd when either one gets garbage
> +            # collected.
>             self._console_socket = console_socket.ConsoleSocket(
> -                self._console_address,
> +                sock_fd=os.dup(self._cons_sock_pair[1].fileno()),
>                 file=self._console_log_path,
>                 drain=self._drain_console)
> +            self._cons_sock_pair[1].close()

I am not 100% sure but should we save the new sock_fd here? Like
self._cons_sock_pair[1] = sock_fd ;

Then next time console_socket() is invoked, the correct fd will be duped.

>         return self._console_socket
> 
>     @property
> -- 
> 2.41.0
>