| Message ID | 20170724124438.27344-2-apahim@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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.
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.
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.
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.
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.
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):