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 <js...@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 <kw...@redhat.com>
