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

[Ani Sinha]

> On 27-Jul-2023, at 11:22 AM, Ani Sinha <anisinha@redhat.com> wrote:
> 
> 
> 
>> 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. 

I see something like

      console_drainer = datadrainer.LineLogger(self.vm.console_socket.fileno(),
                                                 
logger=self.log.getChild('console'))
        console_drainer.start()

in LinuxTest. Maybe I would be able to use something similar.

> 
>> 
>> 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