[09/20] python/qemu: make 'args' style arguments immutable

Commit Message

John Snow Oct. 6, 2020, 11:58 p.m. UTC

These arguments don't need to be mutable and aren't really used as
such. Clarify their types as immutable and adjust code to match where
necessary.

In general, It's probably best not to accept a user-defined mutable
object and store it as internal object state unless there's a strong
justification for doing so. Instead, try to use generic types as input
with empty tuples as the default, and coerce to list where necessary.

Signed-off-by: John Snow <[e-mail]>
Reviewed-by: Kevin Wolf <[e-mail]>
---
 python/qemu/machine.py | 30 +++++++++++++++++-------------
 python/qemu/qtest.py   | 22 +++++++++++++++++-----
 2 files changed, 34 insertions(+), 18 deletions(-)

Comments

Philippe Mathieu-Daudé Oct. 7, 2020, 4:55 a.m. UTC | #1

On 10/7/20 1:58 AM, John Snow wrote:
> These arguments don't need to be mutable and aren't really used as
> such. Clarify their types as immutable and adjust code to match where
> necessary.
> 
> In general, It's probably best not to accept a user-defined mutable
> object and store it as internal object state unless there's a strong
> justification for doing so. Instead, try to use generic types as input
> with empty tuples as the default, and coerce to list where necessary.
> 
> Signed-off-by: John Snow <[e-mail]>
> Reviewed-by: Kevin Wolf <[e-mail]>
> ---
>  python/qemu/machine.py | 30 +++++++++++++++++-------------
>  python/qemu/qtest.py   | 22 +++++++++++++++++-----
>  2 files changed, 34 insertions(+), 18 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>

Patch

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 4e762fcd529..e599cb7439b 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -18,6 +18,7 @@ 
 #
 
 import errno
+from itertools import chain
 import logging
 import os
 import shutil
@@ -30,6 +31,8 @@ 
     Dict,
     List,
     Optional,
+    Sequence,
+    Tuple,
     Type,
 )
 
@@ -74,8 +77,12 @@  class QEMUMachine:
         # vm is guaranteed to be shut down here
     """
 
-    def __init__(self, binary, args=None, wrapper=None, name=None,
-                 test_dir="/var/tmp",
+    def __init__(self,
+                 binary: str,
+                 args: Sequence[str] = (),
+                 wrapper: Sequence[str] = (),
+                 name: Optional[str] = None,
+                 test_dir: str = "/var/tmp",
                  monitor_address: Optional[SocketAddrT] = None,
                  socket_scm_helper=None, sock_dir=None,
                  drain_console=False, console_log=None):
@@ -97,14 +104,7 @@  def __init__(self, binary, args=None, wrapper=None, name=None,
         # Direct user configuration
 
         self._binary = binary
-
-        if args is None:
-            args = []
-        # Copy mutable input: we will be modifying our copy
         self._args = list(args)
-
-        if wrapper is None:
-            wrapper = []
         self._wrapper = wrapper
 
         self._name = name or "qemu-%d" % os.getpid()
@@ -136,7 +136,7 @@  def __init__(self, binary, args=None, wrapper=None, name=None,
         self._iolog = None
         self._qmp_set = True   # Enable QMP monitor by default.
         self._qmp_connection: Optional[qmp.QEMUMonitorProtocol] = None
-        self._qemu_full_args = None
+        self._qemu_full_args: Tuple[str, ...] = ()
         self._temp_dir = None
         self._launched = False
         self._machine = None
@@ -368,7 +368,7 @@  def launch(self):
             raise QEMUMachineError('VM already launched')
 
         self._iolog = None
-        self._qemu_full_args = None
+        self._qemu_full_args = ()
         try:
             self._launch()
             self._launched = True
@@ -388,8 +388,12 @@  def _launch(self):
         """
         devnull = open(os.path.devnull, 'rb')
         self._pre_launch()
-        self._qemu_full_args = (self._wrapper + [self._binary] +
-                                self._base_args + self._args)
+        self._qemu_full_args = tuple(
+            chain(self._wrapper,
+                  [self._binary],
+                  self._base_args,
+                  self._args)
+        )
         LOG.debug('VM launch command: %r', ' '.join(self._qemu_full_args))
         self._popen = subprocess.Popen(self._qemu_full_args,
                                        stdin=devnull,
diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py
index 7fde2565a0a..675310d8dfe 100644
--- a/python/qemu/qtest.py
+++ b/python/qemu/qtest.py
@@ -19,7 +19,12 @@ 
 
 import os
 import socket
-from typing import List, Optional, TextIO
+from typing import (
+    List,
+    Optional,
+    Sequence,
+    TextIO,
+)
 
 from .machine import QEMUMachine
 
@@ -99,8 +104,13 @@  class QEMUQtestMachine(QEMUMachine):
     A QEMU VM, with a qtest socket available.
     """
 
-    def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
-                 socket_scm_helper=None, sock_dir=None):
+    def __init__(self,
+                 binary: str,
+                 args: Sequence[str] = (),
+                 name: Optional[str] = None,
+                 test_dir: str = "/var/tmp",
+                 socket_scm_helper: Optional[str] = None,
+                 sock_dir: Optional[str] = None):
         if name is None:
             name = "qemu-%d" % os.getpid()
         if sock_dir is None:
@@ -114,8 +124,10 @@  def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
     @property
     def _base_args(self) -> List[str]:
         args = super()._base_args
-        args.extend(['-qtest', 'unix:path=' + self._qtest_path,
-                     '-accel', 'qtest'])
+        args.extend([
+            '-qtest', f"unix:path={self._qtest_path}",
+            '-accel', 'qtest'
+        ])
         return args
 
     def _pre_launch(self):