Re: [PATCH 08/20] python/machine.py: fix _popen access

[Kevin Wolf <kwolf@redhat.com>]

Am 07.10.2020 um 01:58 hat John Snow geschrieben:
> As always, Optional[T] causes problems with unchecked access. Add a
> helper that asserts the pipe is present before we attempt to talk with
> it.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

First a question about the preexisting state: I see that after
initialising self._popen once, we never reset it to None. Should we do
so on shutdown?

>  python/qemu/machine.py | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index 3e9cf09fd2d..4e762fcd529 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -131,7 +131,7 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
>          # Runstate
>          self._qemu_log_path = None
>          self._qemu_log_file = None
> -        self._popen = None
> +        self._popen: Optional['subprocess.Popen[bytes]'] = None

Another option that we have, especially if it's an attribute that is
never reset, would be to set the attribute only when it first gets a
value other than None. Accessing it while it hasn't been set yet
automatically results in an AttributeError. I don't think that's much
worse than the exception raised explicitly in a property wrapper.

In this case, you would only declare the type in __init__, but not
assign a value to it:

    self._popen: Optional['subprocess.Popen[bytes]']

Maybe a nicer alternative in some cases than adding properties around
everything.

Instead of checking for None, you would then have to use hasattr(),
which is a bit uglier, so I guess it's mainly for attributes where you
can assume that you will always have a value (if the caller isn't buggy)
and therefore don't even have a check in most places.

>          self._events = []
>          self._iolog = None
>          self._qmp_set = True   # Enable QMP monitor by default.
> @@ -244,6 +244,12 @@ def is_running(self):
>          """Returns true if the VM is running."""
>          return self._popen is not None and self._popen.poll() is None
>  
> +    @property
> +    def _subp(self) -> 'subprocess.Popen[bytes]':
> +        if self._popen is None:
> +            raise QEMUMachineError('Subprocess pipe not present')
> +        return self._popen
> +
>      def exitcode(self):
>          """Returns the exit code if possible, or None."""
>          if self._popen is None:

Of course, even if an alternative is possible, what you have is still
correct.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>