Re: [Qemu-devel] [RFC PATCH 2/7] avocado: Update python scripts to upstream codebase

[Cleber Rosa]

On 04/19/2018 12:46 PM, Philippe Mathieu-Daudé wrote:
> QEMUMachine arguments member is called '_args'.
> 
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
>  scripts/qemu.py                    | 14 +++++++-------
>  tests/avocado/avocado_qemu/test.py | 12 ++++++------
>  tests/qemu-iotests/iotests.py      | 28 ++++++++++++++--------------
>  3 files changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index bd66620f45..0eecc44d09 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -81,7 +81,7 @@ class QEMUMachine(object):
>          self._qemu_log_file = None
>          self._popen = None
>          self._binary = binary
> -        self.args = list(args)     # Force copy args in case we modify them
> +        self._args = list(args)     # Force copy args in case we modify them
>          self._wrapper = wrapper
>          self._events = []
>          self._iolog = None
> @@ -109,8 +109,8 @@ class QEMUMachine(object):
>      # This can be used to add an unused monitor instance.
>      def add_monitor_telnet(self, ip, port):
>          args = 'tcp:%s:%d,server,nowait,telnet' % (ip, port)
> -        self.args.append('-monitor')
> -        self.args.append(args)
> +        self._args.append('-monitor')
> +        self._args.append(args)
>  
>      def add_fd(self, fd, fdset, opaque, opts=''):
>          '''Pass a file descriptor to the VM'''
> @@ -120,8 +120,8 @@ class QEMUMachine(object):
>          if opts:
>              options.append(opts)
>  
> -        self.args.append('-add-fd')
> -        self.args.append(','.join(options))
> +        self._args.append('-add-fd')
> +        self._args.append(','.join(options))
>          return self
>  
>      def send_fd_scm(self, fd_file_path):
> @@ -184,7 +184,7 @@ class QEMUMachine(object):
>                  '-display', 'none', '-vga', 'none']
>  
>      def _create_console(self, console_address):
> -        for item in self.args:
> +        for item in self._args:
>              for option in ['isa-serial', 'spapr-vty', 'sclpconsole']:
>                  if option in item:
>                      return []
> @@ -274,7 +274,7 @@ class QEMUMachine(object):
>          bargs = self._base_args()
>          bargs.extend(self._create_console(console_address))
>          self._qemu_full_args = (self._wrapper + [self._binary] +
> -                                bargs + self.args)
> +                                bargs + self._args)
>          self._popen = subprocess.Popen(self._qemu_full_args,
>                                         stdin=devnull,
>                                         stdout=self._qemu_log_file,
> diff --git a/tests/avocado/avocado_qemu/test.py 
> b/tests/avocado/avocado_qemu/test.py
> index 5a08dace45..1ead917014 100644
> --- a/tests/avocado/avocado_qemu/test.py
> +++ b/tests/avocado/avocado_qemu/test.py
> @@ -297,8 +297,8 @@ class _VM(qemu.QEMUMachine):
>          port = self.ports.find_free_port()
>          newvm = _VM(self.qemu_dst_bin, self._arch, username=self.username,
>                      password=self.password)
> -        newvm.args = self.args
> -        newvm.args.extend(['-incoming', 'tcp:0:%s' % port])
> +        newvm._args = self._args
> +        newvm._args.extend(['-incoming', 'tcp:0:%s' % port])
>          newvm.username = self.username
>          newvm.password = self.password
>  
> @@ -329,7 +329,7 @@ class _VM(qemu.QEMUMachine):
>          :param extra: Extra parameters to the -drive option
>          """
>          file_option = 'file=%s' % path
> -        for item in self.args:
> +        for item in self._args:
>              if file_option in item:
>                  logging.error('Image %s already present', path)
>                  return
> @@ -340,7 +340,7 @@ class _VM(qemu.QEMUMachine):
>          if snapshot:
>              file_option += ',snapshot=on'
>  
> -        self.args.extend(['-drive', file_option])
> +        self._args.extend(['-drive', file_option])
>  
>          if username is not None:
>              self.username = username
> @@ -387,7 +387,7 @@ class _VM(qemu.QEMUMachine):
>          process.run("%s -output %s -volid cidata -joliet -rock %s %s" %
>                      (geniso_bin, iso_path, metadata_path, userdata_path))
>  
> -        self.args.extend(['-cdrom', iso_path])
> +        self._args.extend(['-cdrom', iso_path])
>  
>  class QemuTest(Test):
>  
> @@ -415,4 +415,4 @@ class QemuTest(Test):
>          if machine_kvm_type is not None:
>              machine += "kvm-type=%s," % machine_kvm_type
>          if machine:
> -            self.vm.args.extend(['-machine', machine])
> +            self.vm._args.extend(['-machine', machine])
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index a2e4f03743..b25d48a91b 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -293,18 +293,18 @@ class VM(qtest.QEMUQtestMachine):
>          self._num_drives = 0
>  
>      def add_object(self, opts):
> -        self.args.append('-object')
> -        self.args.append(opts)
> +        self._args.append('-object')
> +        self._args.append(opts)
>          return self
>  
>      def add_device(self, opts):
> -        self.args.append('-device')
> -        self.args.append(opts)
> +        self._args.append('-device')
> +        self._args.append(opts)
>          return self
>  
>      def add_drive_raw(self, opts):
> -        self.args.append('-drive')
> -        self.args.append(opts)
> +        self._args.append('-drive')
> +        self._args.append(opts)
>          return self
>  
>      def add_drive(self, path, opts='', interface='virtio', format=imgfmt):
> @@ -322,27 +322,27 @@ class VM(qtest.QEMUQtestMachine):
>  
>          if format == 'luks' and 'key-secret' not in opts:
>              # default luks support
> -            if luks_default_secret_object not in self.args:
> +            if luks_default_secret_object not in self._args:
>                  self.add_object(luks_default_secret_object)
>  
>              options.append(luks_default_key_secret_opt)
>  
> -        self.args.append('-drive')
> -        self.args.append(','.join(options))
> +        self._args.append('-drive')
> +        self._args.append(','.join(options))
>          self._num_drives += 1
>          return self
>  
>      def add_blockdev(self, opts):
> -        self.args.append('-blockdev')
> +        self._args.append('-blockdev')
>          if isinstance(opts, str):
> -            self.args.append(opts)
> +            self._args.append(opts)
>          else:
> -            self.args.append(','.join(opts))
> +            self._args.append(','.join(opts))
>          return self
>  
>      def add_incoming(self, addr):
> -        self.args.append('-incoming')
> -        self.args.append(addr)
> +        self._args.append('-incoming')
> +        self._args.append(addr)
>          return self
>  
>      def pause_drive(self, drive, event=None):
> 

Well, this is basically a revert of parts of this patch:

  https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg03446.html

There was some previous discussions on this topic, basically regarding
two conflicting points:

1) The fact that other instances of scripts.qemu.QEMUMachine (such as
the _VM instances created by avocado_qemu.tests.QemuTest) need access to
the QEMU args;

2) That attributes prefixed with (single) underscore should only be
accessed by code in the class itself.

The discussion then orbited around the usefulness (or not) of a "dumb"
wrapper for a list.  For the record, I'm in favor of exposing the list
directly, until/if some extra functionality is needed.