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

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- 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 --]