diff mbox series

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

Message ID 20230725180337.2937292-5-jsnow@redhat.com
State New
Headers show
Series python/machine: use socketpair() for console socket | expand

Commit Message

John Snow July 25, 2023, 6:03 p.m. UTC
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>
---
 python/qemu/machine/machine.py | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

Comments

Daniel P. Berrangé July 25, 2023, 6:21 p.m. UTC | #1
On Tue, Jul 25, 2023 at 02:03:35PM -0400, John Snow 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>
> ---
>  python/qemu/machine/machine.py | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

With regards,
Daniel
Ani Sinha July 26, 2023, 10:50 a.m. UTC | #2
> 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
>
John Snow July 26, 2023, 5:21 p.m. UTC | #3
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...)

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().

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
> >
>
>
Ani Sinha July 27, 2023, 5:52 a.m. UTC | #4
> 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
> >
Ani Sinha July 27, 2023, 6:49 a.m. UTC | #5
> 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
Daniel P. Berrangé July 27, 2023, 10:41 a.m. UTC | #6
On Thu, Jul 27, 2023 at 11:22:35AM +0530, Ani Sinha 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.


There's no special code required for that - any -chardev backend can
have logfile=/some/path added to it.


With regards,
Daniel
diff mbox series

Patch

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()
         return self._console_socket
 
     @property