[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 1/2] qemu.py: make 'args' public
From: |
Cleber Rosa |
Subject: |
Re: [Qemu-devel] [PATCH v4 1/2] qemu.py: make 'args' public |
Date: |
Wed, 26 Jul 2017 14:34:58 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 |
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 <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.
>
> 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 ]
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v4 0/2] scripts/qemu.py fixes and cleanups, Amador Pahim, 2017/07/24
- [Qemu-devel] [PATCH v4 1/2] qemu.py: make 'args' public, Amador Pahim, 2017/07/24
- Re: [Qemu-devel] [PATCH v4 1/2] qemu.py: make 'args' public, Stefan Hajnoczi, 2017/07/25
- Re: [Qemu-devel] [PATCH v4 1/2] qemu.py: make 'args' public, Amador Pahim, 2017/07/25
- Re: [Qemu-devel] [PATCH v4 1/2] qemu.py: make 'args' public, Cleber Rosa, 2017/07/25
- Re: [Qemu-devel] [PATCH v4 1/2] qemu.py: make 'args' public, Stefan Hajnoczi, 2017/07/26
- Re: [Qemu-devel] [PATCH v4 1/2] qemu.py: make 'args' public,
Cleber Rosa <=
- Re: [Qemu-devel] [PATCH v4 1/2] qemu.py: make 'args' public, Amador Pahim, 2017/07/27
- Re: [Qemu-devel] [PATCH v4 1/2] qemu.py: make 'args' public, Amador Pahim, 2017/07/27
[Qemu-devel] [PATCH v4 2/2] qemu.py: cleanup and fixes, Amador Pahim, 2017/07/24