[Qemu-devel] [PATCH v4 0/2] scripts/qemu.py fixes and cleanups

[Qemu-devel] [PATCH v4 0/2] scripts/qemu.py fixes and cleanups

[Amador Pahim]

First commit renames self._args to self.args. The leading underscore
represents that the variable is private and as such it should not be
accessed externally. But that variable is the main API for inclusion
of Qemu command arguments, so the rename makes it public.

Second commit contains some fixes and cleanups to make sure we are
correctly trying to launch the VM and properly shutting it down. More
details about the changes in the commit message.

Changes v1->v2:
 - Style fixes to make checkpatch.pl happy.
 - Rebased.
Changes v2->v3:
 - Fix typo in patch 3 ("qemu.py: make 'args' public") commit message.
Changes v3->v4:
 - Squash the 2 first commits since they are co-dependant.
 - Cleanup launch() and shutdown().
 - Reorder the commits, putting the rename of self._args first.
 - Rebased.

Amador Pahim (2):
  qemu.py: make 'args' public
  qemu.py: cleanup and fixes

 scripts/qemu.py               | 79 ++++++++++++++++++++++++++++---------------
 tests/qemu-iotests/iotests.py | 18 +++++-----
 2 files changed, 60 insertions(+), 37 deletions(-)

-- 
2.13.3

[Qemu-devel] [PATCH v4 1/2] qemu.py: make 'args' public

[Amador Pahim]

Let's make args public so users can extend it without feeling like
abusing the internal API.

Signed-off-by: Amador Pahim <apahim@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
 scripts/qemu.py               | 10 +++++-----
 tests/qemu-iotests/iotests.py | 18 +++++++++---------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 880e3e8219..fb83e3f998 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -33,7 +33,7 @@ class QEMUMachine(object):
         self._qemu_log_path = os.path.join(test_dir, name + ".log")
         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
@@ -43,8 +43,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'''
@@ -54,8 +54,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):
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index abcf3c10e2..6925d8841e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -150,13 +150,13 @@ class VM(qtest.QEMUQtestMachine):
         self._num_drives = 0
 
     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):
@@ -172,17 +172,17 @@ class VM(qtest.QEMUQtestMachine):
         if opts:
             options.append(opts)
 
-        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 pause_drive(self, drive, event=None):
-- 
2.13.3

[Qemu-devel] [PATCH v4 2/2] qemu.py: cleanup and fixes

[Amador Pahim]

is_running():
- Use Popen.poll() instead of Popen.returncode to check whether
  the VM is running or not.

exitcode():
- Use Popen.poll() instead of Popen.returncode to return an updated
exit code.

_load_io_log():
- Add a try/except to prevent raising exception when qemu io file
does not exist.

launch():
- If VM is already running, do nothing.
- If vm is not running but was not cleaned up, call shutdown()
  before launching again.
- Offload the core of this method to _launch().
- Load the args and try to call _launch().
- Make sure we cleanup on exception.
- On exception, print an error message with the qemu command line
  and output.

_launch():
- Execute _pre_launch(), subprocess.Popen() and self._post_launch().
- No try/except here. Any exceptions will be handled by the caller.

shutdown():
- Make sure self._popen is not None before calling self._popen.wait().
- Cleanup the message on negative exit codes.
- Always execute self._load_io_log() and self._post_shutdown().

Signed-off-by: Amador Pahim <apahim@redhat.com>
---
 scripts/qemu.py | 69 ++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 46 insertions(+), 23 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index fb83e3f998..76d4880f36 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -39,6 +39,7 @@ class QEMUMachine(object):
         self._iolog = None
         self._socket_scm_helper = socket_scm_helper
         self._debug = debug
+        self._shutdown_pending = False
 
     # This can be used to add an unused monitor instance.
     def add_monitor_telnet(self, ip, port):
@@ -86,12 +87,12 @@ class QEMUMachine(object):
             raise
 
     def is_running(self):
-        return self._popen and (self._popen.returncode is None)
+        return self._popen and (self._popen.poll() is None)
 
     def exitcode(self):
         if self._popen is None:
             return None
-        return self._popen.returncode
+        return self._popen.poll()
 
     def get_pid(self):
         if not self.is_running():
@@ -99,8 +100,11 @@ class QEMUMachine(object):
         return self._popen.pid
 
     def _load_io_log(self):
-        with open(self._qemu_log_path, "r") as fh:
-            self._iolog = fh.read()
+        try:
+            with open(self._qemu_log_path, "r") as fh:
+                self._iolog = fh.read()
+        except IOError:
+            pass
 
     def _base_args(self):
         if isinstance(self._monitor_address, tuple):
@@ -126,25 +130,43 @@ class QEMUMachine(object):
         self._remove_if_exists(self._qemu_log_path)
 
     def launch(self):
-        '''Launch the VM and establish a QMP connection'''
-        devnull = open('/dev/null', 'rb')
-        qemulog = open(self._qemu_log_path, 'wb')
+        '''
+        Try to launch the VM and make sure we cleanup and expose the
+        command line/output in case of exception.
+        '''
+        if self.is_running():
+            return
+
+        if self._shutdown_pending:
+            sys.stderr.write('shutdown() was not called after previous '
+                             'launch(). Calling now.\n')
+            self.shutdown()
+
         try:
-            self._pre_launch()
-            args = self._wrapper + [self._binary] + self._base_args() + self._args
-            self._popen = subprocess.Popen(args, stdin=devnull, stdout=qemulog,
-                                           stderr=subprocess.STDOUT, shell=False)
-            self._post_launch()
+            args = None
+            args = self._wrapper + [self._binary] + self._base_args() + self.args
+            self._launch(args)
+            self._shutdown_pending = True
         except:
-            if self.is_running():
-                self._popen.kill()
-                self._popen.wait()
-            self._load_io_log()
-            self._post_shutdown()
+            self.shutdown()
+            sys.stderr.write('Error launching VM.\n%s%s' %
+                             ('Command:\n%s\n' %
+                              ' '.join(args) if args else '',
+                              'Output:\n%s\n' %
+                              ''.join(self._iolog) if self._iolog else ''))
             raise
 
+    def _launch(self, args):
+        '''Launch the VM and establish a QMP connection.'''
+        devnull = open('/dev/null', 'rb')
+        qemulog = open(self._qemu_log_path, 'wb')
+        self._pre_launch()
+        self._popen = subprocess.Popen(args, stdin=devnull, stdout=qemulog,
+                                       stderr=subprocess.STDOUT, shell=False)
+        self._post_launch()
+
     def shutdown(self):
-        '''Terminate the VM and clean up'''
+        '''Terminate the VM and clean up.'''
         if self.is_running():
             try:
                 self._qmp.cmd('quit')
@@ -152,11 +174,12 @@ class QEMUMachine(object):
             except:
                 self._popen.kill()
 
-            exitcode = self._popen.wait()
-            if exitcode < 0:
-                sys.stderr.write('qemu received signal %i: %s\n' % (-exitcode, ' '.join(self._args)))
-            self._load_io_log()
-            self._post_shutdown()
+        if self._popen and self._popen.wait() < 0:
+            sys.stderr.write('qemu received signal %i\n' % -self.exitcode())
+
+        self._load_io_log()
+        self._post_shutdown()
+        self._shutdown_pending = False
 
     underscore_to_dash = string.maketrans('_', '-')
     def qmp(self, cmd, conv_keys=True, **args):
-- 
2.13.3

Re: [Qemu-devel] [PATCH v4 1/2] qemu.py: make 'args' public

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 983 bytes --]

On Mon, Jul 24, 2017 at 02:44:37PM +0200, Amador Pahim wrote:
> Let's make args public so users can extend it without feeling like
> abusing the internal API.

Nothing is abusing an internal API.  PEP8 describes the difference
between public (no underscore), protected aka subclass API (single
underscore), and private fields (single or double underscore):

  https://www.python.org/dev/peps/pep-0008/

_args is a protected field.  It is perfectly normal for a subclass to
access a protected field in its parent class.

> Signed-off-by: Amador Pahim <apahim@redhat.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> ---
>  scripts/qemu.py               | 10 +++++-----
>  tests/qemu-iotests/iotests.py | 18 +++++++++---------
>  2 files changed, 14 insertions(+), 14 deletions(-)

Please don't do this, it encourages code duplication.  Now arbitrary
users can start accessing the public field directly instead of adding a
reusable interfaces like add_monitor_telnet(), add_fd(), etc.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v4 2/2] qemu.py: cleanup and fixes

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1370 bytes --]

On Mon, Jul 24, 2017 at 02:44:38PM +0200, Amador Pahim wrote:
> is_running():
> - Use Popen.poll() instead of Popen.returncode to check whether
>   the VM is running or not.
> 
> exitcode():
> - Use Popen.poll() instead of Popen.returncode to return an updated
> exit code.
> 
> _load_io_log():
> - Add a try/except to prevent raising exception when qemu io file
> does not exist.
> 
> launch():
> - If VM is already running, do nothing.
> - If vm is not running but was not cleaned up, call shutdown()
>   before launching again.
> - Offload the core of this method to _launch().
> - Load the args and try to call _launch().
> - Make sure we cleanup on exception.
> - On exception, print an error message with the qemu command line
>   and output.
> 
> _launch():
> - Execute _pre_launch(), subprocess.Popen() and self._post_launch().
> - No try/except here. Any exceptions will be handled by the caller.
> 
> shutdown():
> - Make sure self._popen is not None before calling self._popen.wait().
> - Cleanup the message on negative exit codes.
> - Always execute self._load_io_log() and self._post_shutdown().
> 
> Signed-off-by: Amador Pahim <apahim@redhat.com>

Please break this up into logical changes and include the rationale for
making them.  The commit description should explain "why" rather than
"what" the code change is.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v4 1/2] qemu.py: make 'args' public

[Amador Pahim]

On Tue, Jul 25, 2017 at 3:37 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Jul 24, 2017 at 02:44:37PM +0200, Amador Pahim wrote:
>> Let's make args public so users can extend it without feeling like
>> abusing the internal API.
>
> Nothing is abusing an internal API.  PEP8 describes the difference
> between public (no underscore), protected aka subclass API (single
> underscore), and private fields (single or double underscore):
>
>   https://www.python.org/dev/peps/pep-0008/
>
> _args is a protected field.  It is perfectly normal for a subclass to
> access a protected field in its parent class.

Precisely. Idea is to make it public according to the concept of
public x protected from that very documentation ("Public attributes
are those that you expect unrelated clients of your class to use").
This makes sense for the work I'm doing on creating
functional/regression tests for qemu using Avocado Framework (patchs
to come), where we instantiate a QEMUMachine object to be offered
inside the Test API.

>
>> Signed-off-by: Amador Pahim <apahim@redhat.com>
>> Reviewed-by: Fam Zheng <famz@redhat.com>
>> ---
>>  scripts/qemu.py               | 10 +++++-----
>>  tests/qemu-iotests/iotests.py | 18 +++++++++---------
>>  2 files changed, 14 insertions(+), 14 deletions(-)
>
> Please don't do this, it encourages code duplication.  Now arbitrary
> users can start accessing the public field directly instead of adding a
> reusable interfaces like add_monitor_telnet(), add_fd(), etc.

Reusable interfaces can be quite complex to implement when all you
need is to add some options to the qemu command line. I'd not require
that from arbitrary users. Anyway, dropping this commit. Thanks for
the review.

Re: [Qemu-devel] [PATCH v4 2/2] qemu.py: cleanup and fixes

[Amador Pahim]

On Tue, Jul 25, 2017 at 3:45 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Jul 24, 2017 at 02:44:38PM +0200, Amador Pahim wrote:
>> is_running():
>> - Use Popen.poll() instead of Popen.returncode to check whether
>>   the VM is running or not.
>>
>> exitcode():
>> - Use Popen.poll() instead of Popen.returncode to return an updated
>> exit code.
>>
>> _load_io_log():
>> - Add a try/except to prevent raising exception when qemu io file
>> does not exist.
>>
>> launch():
>> - If VM is already running, do nothing.
>> - If vm is not running but was not cleaned up, call shutdown()
>>   before launching again.
>> - Offload the core of this method to _launch().
>> - Load the args and try to call _launch().
>> - Make sure we cleanup on exception.
>> - On exception, print an error message with the qemu command line
>>   and output.
>>
>> _launch():
>> - Execute _pre_launch(), subprocess.Popen() and self._post_launch().
>> - No try/except here. Any exceptions will be handled by the caller.
>>
>> shutdown():
>> - Make sure self._popen is not None before calling self._popen.wait().
>> - Cleanup the message on negative exit codes.
>> - Always execute self._load_io_log() and self._post_shutdown().
>>
>> Signed-off-by: Amador Pahim <apahim@redhat.com>
>
> Please break this up into logical changes and include the rationale for
> making them.  The commit description should explain "why" rather than
> "what" the code change is.

Sure.

Re: [Qemu-devel] [PATCH v4 1/2] qemu.py: make 'args' public

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 2974 bytes --]



On 07/25/2017 09:37 AM, Stefan Hajnoczi wrote:
> On Mon, Jul 24, 2017 at 02:44:37PM +0200, Amador Pahim wrote:
>> Let's make args public so users can extend it without feeling like
>> abusing the internal API.
> 
> Nothing is abusing an internal API.  PEP8 describes the difference
> between public (no underscore), protected aka subclass API (single
> underscore), and private fields (single or double underscore):
> 
>   https://www.python.org/dev/peps/pep-0008/
> 
> _args is a protected field.  It is perfectly normal for a subclass to
> access a protected field in its parent class.
> 

Right, which means that instances should not be using it.  I guess
there's a clear conflict of what is assumed to be the intended use for
this class.

I can see the value in using it *directly*, and I can also see changing
the QEMU args as a requirement.

>> Signed-off-by: Amador Pahim <apahim@redhat.com>
>> Reviewed-by: Fam Zheng <famz@redhat.com>
>> ---
>>  scripts/qemu.py               | 10 +++++-----
>>  tests/qemu-iotests/iotests.py | 18 +++++++++---------
>>  2 files changed, 14 insertions(+), 14 deletions(-)
> 
> Please don't do this, it encourages code duplication.  Now arbitrary
> users can start accessing the public field directly instead of adding a
> reusable interfaces like add_monitor_telnet(), add_fd(), etc.
> 

Judging from tests/qemu-iotests/iotests.py:VM and your comment above, I
assume you see value in simple wrappers such as:

   def add_device(self, opts):
        self._args.append('-device')
        self._args.append(opts)
        return self

I honestly do not see any value here.  I do see value in other wrappers,
such as add_drive(), in which default values are there for convenience,
and a drive count is kept.  In the end, my point is that the there are
cases when a wrapper is just an annoyance and provides nothing more than
 the illusion of a better design.

If "args" is not made public, wrappers with no real value will start to
be proposed, either within the test's own subclass (like in
tests/qemu-iotests/iotests.py:VM) or will make its way to
scripts/qemu.py:QEMUMachine.

What you describe as "adding reusable interfaces" can be seen both as a
blessing if they're really well designed interfaces, or a curse if
they're something a test writer *has* to write while a test is being
written to get around the fact that "args" is not available.

Extending on the "add_device()" example, how would you feel about
something like:

   def add_arg(self, arg, opts=None):
      self._args.append(arg)
      if opts is not None:
          self._args.append(opts)

FIY, I have a bad feeling about it, but it's quite comparable to
add_device(), and it keeps "args" private.

-- 
Cleber Rosa
[ Sr Software Engineer - Virtualization Team - Red Hat ]
[ Avocado Test Framework - avocado-framework.github.io ]
[  7ABB 96EB 8B46 B94D 5E0F  E9BB 657E 8D33 A5F2 09F3  ]


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v4 1/2] qemu.py: make 'args' public

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2200 bytes --]

On Tue, Jul 25, 2017 at 11:30:16AM -0400, Cleber Rosa wrote:
> On 07/25/2017 09:37 AM, Stefan Hajnoczi wrote:
> > On Mon, Jul 24, 2017 at 02:44:37PM +0200, Amador Pahim wrote:
> >> Signed-off-by: Amador Pahim <apahim@redhat.com>
> >> Reviewed-by: Fam Zheng <famz@redhat.com>
> >> ---
> >>  scripts/qemu.py               | 10 +++++-----
> >>  tests/qemu-iotests/iotests.py | 18 +++++++++---------
> >>  2 files changed, 14 insertions(+), 14 deletions(-)
> > 
> > Please don't do this, it encourages code duplication.  Now arbitrary
> > users can start accessing the public field directly instead of adding a
> > reusable interfaces like add_monitor_telnet(), add_fd(), etc.
> > 
> 
> Judging from tests/qemu-iotests/iotests.py:VM and your comment above, I
> assume you see value in simple wrappers such as:
> 
>    def add_device(self, opts):
>         self._args.append('-device')
>         self._args.append(opts)
>         return self
> 
> I honestly do not see any value here.  I do see value in other wrappers,
> such as add_drive(), in which default values are there for convenience,
> and a drive count is kept.  In the end, my point is that the there are
> cases when a wrapper is just an annoyance and provides nothing more than
>  the illusion of a better design.

I don't see much value in simple wrappers either besides method chaining
(more on that below).

Getting back to this patch, I can only review patches in the context of
the current code base.  I don't know future plans you may have.  The
change may make sense together with other new changes that use a public
args field in a useful way - it just doesn't make sense the way it has
been presented in isolation.

About method chaining, the current code seems to be written with method
chaining in mind:

https://en.wikipedia.org/wiki/Method_chaining#Python

If all arguments are methods then everything can be chained:

  (vm.add_fd(fd.fileno(), 1, 'image0')
     .add_drive('fd:image0', interface='none')
     .add_device('virtio-blk-pci,drive=drive0'))

So I guess there is a small value in having add_device().  That said,
tests don't take advantage of method chaining much.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v4 1/2] qemu.py: make 'args' public

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 2817 bytes --]



On 07/26/2017 01:35 PM, Stefan Hajnoczi wrote:
> On Tue, Jul 25, 2017 at 11:30:16AM -0400, Cleber Rosa wrote:
>> On 07/25/2017 09:37 AM, Stefan Hajnoczi wrote:
>>> On Mon, Jul 24, 2017 at 02:44:37PM +0200, Amador Pahim wrote:
>>>> Signed-off-by: Amador Pahim <apahim@redhat.com>
>>>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>>> ---
>>>>  scripts/qemu.py               | 10 +++++-----
>>>>  tests/qemu-iotests/iotests.py | 18 +++++++++---------
>>>>  2 files changed, 14 insertions(+), 14 deletions(-)
>>>
>>> Please don't do this, it encourages code duplication.  Now arbitrary
>>> users can start accessing the public field directly instead of adding a
>>> reusable interfaces like add_monitor_telnet(), add_fd(), etc.
>>>
>>
>> Judging from tests/qemu-iotests/iotests.py:VM and your comment above, I
>> assume you see value in simple wrappers such as:
>>
>>    def add_device(self, opts):
>>         self._args.append('-device')
>>         self._args.append(opts)
>>         return self
>>
>> I honestly do not see any value here.  I do see value in other wrappers,
>> such as add_drive(), in which default values are there for convenience,
>> and a drive count is kept.  In the end, my point is that the there are
>> cases when a wrapper is just an annoyance and provides nothing more than
>>  the illusion of a better design.
> 
> I don't see much value in simple wrappers either besides method chaining
> (more on that below).
> 
> Getting back to this patch, I can only review patches in the context of
> the current code base.  I don't know future plans you may have.  The
> change may make sense together with other new changes that use a public
> args field in a useful way - it just doesn't make sense the way it has
> been presented in isolation.
> 

I think I described what the intention is: support tests that use
instances directly.  But yes, I think it can be better backed by real
code making use of it.

> About method chaining, the current code seems to be written with method
> chaining in mind:
> 
> https://en.wikipedia.org/wiki/Method_chaining#Python
> 
> If all arguments are methods then everything can be chained:
> 
>   (vm.add_fd(fd.fileno(), 1, 'image0')
>      .add_drive('fd:image0', interface='none')
>      .add_device('virtio-blk-pci,drive=drive0'))
> 
> So I guess there is a small value in having add_device().  That said,
> tests don't take advantage of method chaining much.
> 

Right, just about the same amount of value of a more generic
"add_arg(option, arguments=[])".  IMO not even worth its maintenance costs.

-- 
Cleber Rosa
[ Sr Software Engineer - Virtualization Team - Red Hat ]
[ Avocado Test Framework - avocado-framework.github.io ]
[  7ABB 96EB 8B46 B94D 5E0F  E9BB 657E 8D33 A5F2 09F3  ]


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v4 1/2] qemu.py: make 'args' public

[Amador Pahim]

On Wed, Jul 26, 2017 at 7:35 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Jul 25, 2017 at 11:30:16AM -0400, Cleber Rosa wrote:
>> On 07/25/2017 09:37 AM, Stefan Hajnoczi wrote:
>> > On Mon, Jul 24, 2017 at 02:44:37PM +0200, Amador Pahim wrote:
>> >> Signed-off-by: Amador Pahim <apahim@redhat.com>
>> >> Reviewed-by: Fam Zheng <famz@redhat.com>
>> >> ---
>> >>  scripts/qemu.py               | 10 +++++-----
>> >>  tests/qemu-iotests/iotests.py | 18 +++++++++---------
>> >>  2 files changed, 14 insertions(+), 14 deletions(-)
>> >
>> > Please don't do this, it encourages code duplication.  Now arbitrary
>> > users can start accessing the public field directly instead of adding a
>> > reusable interfaces like add_monitor_telnet(), add_fd(), etc.
>> >
>>
>> Judging from tests/qemu-iotests/iotests.py:VM and your comment above, I
>> assume you see value in simple wrappers such as:
>>
>>    def add_device(self, opts):
>>         self._args.append('-device')
>>         self._args.append(opts)
>>         return self
>>
>> I honestly do not see any value here.  I do see value in other wrappers,
>> such as add_drive(), in which default values are there for convenience,
>> and a drive count is kept.  In the end, my point is that the there are
>> cases when a wrapper is just an annoyance and provides nothing more than
>>  the illusion of a better design.
>
> I don't see much value in simple wrappers either besides method chaining
> (more on that below).
>
> Getting back to this patch, I can only review patches in the context of
> the current code base.  I don't know future plans you may have.  The
> change may make sense together with other new changes that use a public
> args field in a useful way - it just doesn't make sense the way it has
> been presented in isolation.

We disagree on that. The current patch makes sense by itself. It
contains the changes in iotests.py, which has an instance of
QEMUMachine and is using the self._args.
My point mentioning the work to come is to provide yet another use case.

>
> About method chaining, the current code seems to be written with method
> chaining in mind:
>
> https://en.wikipedia.org/wiki/Method_chaining#Python
>
> If all arguments are methods then everything can be chained:
>
>   (vm.add_fd(fd.fileno(), 1, 'image0')
>      .add_drive('fd:image0', interface='none')
>      .add_device('virtio-blk-pci,drive=drive0'))
>
> So I guess there is a small value in having add_device().  That said,
> tests don't take advantage of method chaining much.

Re: [Qemu-devel] [PATCH v4 1/2] qemu.py: make 'args' public

[Amador Pahim]

On Thu, Jul 27, 2017 at 9:39 AM, Amador Pahim <apahim@redhat.com> wrote:
> On Wed, Jul 26, 2017 at 7:35 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Tue, Jul 25, 2017 at 11:30:16AM -0400, Cleber Rosa wrote:
>>> On 07/25/2017 09:37 AM, Stefan Hajnoczi wrote:
>>> > On Mon, Jul 24, 2017 at 02:44:37PM +0200, Amador Pahim wrote:
>>> >> Signed-off-by: Amador Pahim <apahim@redhat.com>
>>> >> Reviewed-by: Fam Zheng <famz@redhat.com>
>>> >> ---
>>> >>  scripts/qemu.py               | 10 +++++-----
>>> >>  tests/qemu-iotests/iotests.py | 18 +++++++++---------
>>> >>  2 files changed, 14 insertions(+), 14 deletions(-)
>>> >
>>> > Please don't do this, it encourages code duplication.  Now arbitrary
>>> > users can start accessing the public field directly instead of adding a
>>> > reusable interfaces like add_monitor_telnet(), add_fd(), etc.
>>> >
>>>
>>> Judging from tests/qemu-iotests/iotests.py:VM and your comment above, I
>>> assume you see value in simple wrappers such as:
>>>
>>>    def add_device(self, opts):
>>>         self._args.append('-device')
>>>         self._args.append(opts)
>>>         return self
>>>
>>> I honestly do not see any value here.  I do see value in other wrappers,
>>> such as add_drive(), in which default values are there for convenience,
>>> and a drive count is kept.  In the end, my point is that the there are
>>> cases when a wrapper is just an annoyance and provides nothing more than
>>>  the illusion of a better design.
>>
>> I don't see much value in simple wrappers either besides method chaining
>> (more on that below).
>>
>> Getting back to this patch, I can only review patches in the context of
>> the current code base.  I don't know future plans you may have.  The
>> change may make sense together with other new changes that use a public
>> args field in a useful way - it just doesn't make sense the way it has
>> been presented in isolation.
>
> We disagree on that. The current patch makes sense by itself. It
> contains the changes in iotests.py, which has an instance of
> QEMUMachine and is using the self._args.
> My point mentioning the work to come is to provide yet another use case.
Actually taking another look to the iotests.py, it is not
instantiating the QEMUMachine, but subclassing it. So yes, this path
will wait till a valid use case for it. Thanks.
>
>>
>> About method chaining, the current code seems to be written with method
>> chaining in mind:
>>
>> https://en.wikipedia.org/wiki/Method_chaining#Python
>>
>> If all arguments are methods then everything can be chained:
>>
>>   (vm.add_fd(fd.fileno(), 1, 'image0')
>>      .add_drive('fd:image0', interface='none')
>>      .add_device('virtio-blk-pci,drive=drive0'))
>>
>> So I guess there is a small value in having add_device().  That said,
>> tests don't take advantage of method chaining much.