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

[Ani Sinha]

> On 26-Jul-2023, at 10:51 PM, John Snow <jsnow@redhat.com> wrote:
> 
> 
> 
> On Wed, Jul 26, 2023, 6:50 AM Ani Sinha <anisinha@redhat.com> wrote:
> 
> 
> > 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.
> 
> Yep! I think you need a few more changes to do what you wanted. IIRC, you 
> also want to be able to drain the console log while waiting for the vm to 
> terminate of its own accord, which I don't support yet.
> 
> (If you use console socket's self draining mode, it should be possible to 
> forego the early termination of the console socket and allow this behavior. 
> Maybe I can work that in now...)

yeah we want to collect all the console logs while the VM is running until it 
self terminates. Maybe you can add a flag for this behavior to not early 
terminate the socket. I think we need to add mathods to keep reading the socket 
and write to a file until the socket is closed. Maybe QemuMachine needs to be 
enhanced. 

> 
> Anything else I'm forgetting ...?
> 
> Except the concern below,
> 
> Reviewed-by: Ani Sinha <anisinha@redhat.com>
> 
> Thanks 😊 
> 
> 
> 
> > ---
> > 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.
> 
> It should be cached to self._console_socket, so it should return the same 
> instance every time, no second call to os.dup().

yeah I missed your patch 3. All good now.

> 
> self._console_socket takes ownership of the duplicated fd and we retain 
> ownership of _cons_sock_pair[1] which we then close right after.
> 
> All three sockets are closed and None'd if applicable during _early_cleanup().
> 
> 
> >         return self._console_socket
> > 
> >     @property
> > -- 
> > 2.41.0
> >