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

[Cleber Rosa]

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 <address@hidden>
>> Reviewed-by: Fam Zheng <address@hidden>
>> ---
>>  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  ]

signature.asc
Description: OpenPGP digital signature