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 <address@hidden> 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.

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.