[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 08/20] python/machine.py: fix _popen access
From: |
Kevin Wolf |
Subject: |
Re: [PATCH 08/20] python/machine.py: fix _popen access |
Date: |
Wed, 7 Oct 2020 12:07:05 +0200 |
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>
- [PATCH 01/20] python/qemu: use isort to lay out imports, (continued)
- [PATCH 01/20] python/qemu: use isort to lay out imports, John Snow, 2020/10/06
- [PATCH 03/20] python/machine.py: reorder __init__, John Snow, 2020/10/06
- [PATCH 02/20] python/machine.py: Fix monitor address typing, John Snow, 2020/10/06
- [PATCH 04/20] python/machine.py: Don't modify state in _base_args(), John Snow, 2020/10/06
- [PATCH 08/20] python/machine.py: fix _popen access, John Snow, 2020/10/06
- Re: [PATCH 08/20] python/machine.py: fix _popen access,
Kevin Wolf <=
[PATCH 09/20] python/qemu: make 'args' style arguments immutable, John Snow, 2020/10/06
[PATCH 05/20] python/machine.py: Handle None events in events_wait, John Snow, 2020/10/06
[PATCH 07/20] python/machine.py: Add _qmp access shim, John Snow, 2020/10/06
[PATCH 06/20] python/machine.py: use qmp.command, John Snow, 2020/10/06