* [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers
@ 2017-08-24 7:21 Stefan Hajnoczi
2017-08-24 7:22 ` [Qemu-devel] [PATCH 1/3] qemu.py: make VM() a context manager Stefan Hajnoczi
` (4 more replies)
0 siblings, 5 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2017-08-24 7:21 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Daniel Berrange, Stefan Hajnoczi
This series introduces context managers for the two most commonly used
resources: files and VMs. Context managers eliminate the need to call a
cleanup function explicitly.
Tests should declare resources upfront in a with statement. Resources are
automatically cleaned up whether the test passes or fails:
with FilePath('test.img') as img_path,
VM() as vm:
...test...
# img_path is deleted and vm is shut down automatically
The final patch converts 194 to use context managers instead of
atexit.register(). This makes the code shorter and easier to read.
Stefan Hajnoczi (3):
qemu.py: make VM() a context manager
iotests.py: add FilePath context manager
qemu-iotests: use context managers for resource cleanup in 194
scripts/qemu.py | 16 ++++++++-
tests/qemu-iotests/194 | 83 +++++++++++++++++++++----------------------
tests/qemu-iotests/iotests.py | 26 ++++++++++++++
3 files changed, 82 insertions(+), 43 deletions(-)
--
2.13.5
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH 1/3] qemu.py: make VM() a context manager
2017-08-24 7:21 [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers Stefan Hajnoczi
@ 2017-08-24 7:22 ` Stefan Hajnoczi
2017-08-25 12:44 ` Eduardo Habkost
2017-08-24 7:22 ` [Qemu-devel] [PATCH 2/3] iotests.py: add FilePath " Stefan Hajnoczi
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Stefan Hajnoczi @ 2017-08-24 7:22 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Daniel Berrange, Stefan Hajnoczi, Eduardo Habkost
There are a number of ways to ensure that the QEMU process is shut down
when the test ends, including atexit.register(), try: finally:, or
unittest.teardown() methods. All of these require extra code and the
programmer must remember to add vm.shutdown().
A nice solution is context managers:
with VM(binary) as vm:
...
# vm is guaranteed to be shut down here
Cc: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
scripts/qemu.py | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 880e3e8219..4d8ee10943 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -21,7 +21,14 @@ import qmp.qmp
class QEMUMachine(object):
- '''A QEMU VM'''
+ '''A QEMU VM
+
+ Use this object as a context manager to ensure the QEMU process terminates::
+
+ with VM(binary) as vm:
+ ...
+ # vm is guaranteed to be shut down here
+ '''
def __init__(self, binary, args=[], wrapper=[], name=None, test_dir="/var/tmp",
monitor_address=None, socket_scm_helper=None, debug=False):
@@ -40,6 +47,13 @@ class QEMUMachine(object):
self._socket_scm_helper = socket_scm_helper
self._debug = debug
+ def __enter__(self):
+ return self
+
+ def __exit__(self, exc_type, exc_val, exc_tb):
+ self.shutdown()
+ return False
+
# 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)
--
2.13.5
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH 2/3] iotests.py: add FilePath context manager
2017-08-24 7:21 [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers Stefan Hajnoczi
2017-08-24 7:22 ` [Qemu-devel] [PATCH 1/3] qemu.py: make VM() a context manager Stefan Hajnoczi
@ 2017-08-24 7:22 ` Stefan Hajnoczi
2017-08-24 7:22 ` [Qemu-devel] [PATCH 3/3] qemu-iotests: use context managers for resource cleanup in 194 Stefan Hajnoczi
` (2 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2017-08-24 7:22 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Daniel Berrange, Stefan Hajnoczi
The scratch/ (TEST_DIR) directory is not automatically cleaned up after
test execution. It is the responsibility of tests to remove any files
they create.
A nice way of doing this is to declare files at the beginning of the
test and automatically remove them with a context manager:
with iotests.FilePath('test.img') as img_path:
qemu_img(...)
qemu_io(...)
# img_path is guaranteed to be deleted here
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
tests/qemu-iotests/iotests.py | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 7233983f3c..07fa1626a0 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -160,6 +160,32 @@ class Timeout:
def timeout(self, signum, frame):
raise Exception(self.errmsg)
+
+class FilePath(object):
+ '''An auto-generated filename that cleans itself up.
+
+ Use this context manager to generate filenames and ensure that the file
+ gets deleted::
+
+ with TestFilePath('test.img') as img_path:
+ qemu_img('create', img_path, '1G')
+ # migration_sock_path is automatically deleted
+ '''
+ def __init__(self, name):
+ filename = '{0}-{1}'.format(os.getpid(), name)
+ self.path = os.path.join(test_dir, filename)
+
+ def __enter__(self):
+ return self.path
+
+ def __exit__(self, exc_type, exc_val, exc_tb):
+ try:
+ os.remove(self.path)
+ except OSError:
+ pass
+ return False
+
+
class VM(qtest.QEMUQtestMachine):
'''A QEMU VM'''
--
2.13.5
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH 3/3] qemu-iotests: use context managers for resource cleanup in 194
2017-08-24 7:21 [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers Stefan Hajnoczi
2017-08-24 7:22 ` [Qemu-devel] [PATCH 1/3] qemu.py: make VM() a context manager Stefan Hajnoczi
2017-08-24 7:22 ` [Qemu-devel] [PATCH 2/3] iotests.py: add FilePath " Stefan Hajnoczi
@ 2017-08-24 7:22 ` Stefan Hajnoczi
2017-08-24 8:38 ` [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers Fam Zheng
2017-08-31 11:26 ` Stefan Hajnoczi
4 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2017-08-24 7:22 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Daniel Berrange, Stefan Hajnoczi
Switch from atexit.register() to a more elegant idiom of declaring
resources in a with statement:
with FilePath('monitor.sock') as monitor_path,
VM() as vm:
...
The files and VMs will be automatically cleaned up whether the test
passes or fails.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
tests/qemu-iotests/194 | 83 +++++++++++++++++++++++++-------------------------
1 file changed, 41 insertions(+), 42 deletions(-)
diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index 8028111e21..06aea661d4 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -19,55 +19,54 @@
#
# Non-shared storage migration test using NBD server and drive-mirror
-import os
-import atexit
import iotests
iotests.verify_platform(['linux'])
-img_size = '1G'
-source_img_path = os.path.join(iotests.test_dir, 'source.img')
-dest_img_path = os.path.join(iotests.test_dir, 'dest.img')
-iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, source_img_path, img_size)
-iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, dest_img_path, img_size)
+with iotests.FilePath('source.img') as source_img_path, \
+ iotests.FilePath('dest.img') as dest_img_path, \
+ iotests.FilePath('migration.sock') as migration_sock_path, \
+ iotests.FilePath('nbd.sock') as nbd_sock_path, \
+ iotests.VM('source') as source_vm, \
+ iotests.VM('dest') as dest_vm:
-iotests.log('Launching VMs...')
-migration_sock_path = os.path.join(iotests.test_dir, 'migration.sock')
-nbd_sock_path = os.path.join(iotests.test_dir, 'nbd.sock')
-source_vm = iotests.VM('source').add_drive(source_img_path)
-dest_vm = (iotests.VM('dest').add_drive(dest_img_path)
- .add_incoming('unix:{0}'.format(migration_sock_path)))
-source_vm.launch()
-atexit.register(source_vm.shutdown)
-dest_vm.launch()
-atexit.register(dest_vm.shutdown)
+ img_size = '1G'
+ iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, source_img_path, img_size)
+ iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, dest_img_path, img_size)
-iotests.log('Launching NBD server on destination...')
-iotests.log(dest_vm.qmp('nbd-server-start', addr={'type': 'unix', 'data': {'path': nbd_sock_path}}))
-iotests.log(dest_vm.qmp('nbd-server-add', device='drive0', writable=True))
+ iotests.log('Launching VMs...')
+ (source_vm.add_drive(source_img_path)
+ .launch())
+ (dest_vm.add_drive(dest_img_path)
+ .add_incoming('unix:{0}'.format(migration_sock_path))
+ .launch())
-iotests.log('Starting drive-mirror on source...')
-iotests.log(source_vm.qmp(
- 'drive-mirror',
- device='drive0',
- target='nbd+unix:///drive0?socket={0}'.format(nbd_sock_path),
- sync='full',
- format='raw', # always raw, the server handles the format
- mode='existing'))
+ iotests.log('Launching NBD server on destination...')
+ iotests.log(dest_vm.qmp('nbd-server-start', addr={'type': 'unix', 'data': {'path': nbd_sock_path}}))
+ iotests.log(dest_vm.qmp('nbd-server-add', device='drive0', writable=True))
-iotests.log('Waiting for drive-mirror to complete...')
-iotests.log(source_vm.event_wait('BLOCK_JOB_READY'),
- filters=[iotests.filter_qmp_event])
+ iotests.log('Starting drive-mirror on source...')
+ iotests.log(source_vm.qmp(
+ 'drive-mirror',
+ device='drive0',
+ target='nbd+unix:///drive0?socket={0}'.format(nbd_sock_path),
+ sync='full',
+ format='raw', # always raw, the server handles the format
+ mode='existing'))
-iotests.log('Starting migration...')
-source_vm.qmp('migrate-set-capabilities',
- capabilities=[{'capability': 'events', 'state': True}])
-dest_vm.qmp('migrate-set-capabilities',
- capabilities=[{'capability': 'events', 'state': True}])
-iotests.log(source_vm.qmp('migrate', uri='unix:{0}'.format(migration_sock_path)))
+ iotests.log('Waiting for drive-mirror to complete...')
+ iotests.log(source_vm.event_wait('BLOCK_JOB_READY'),
+ filters=[iotests.filter_qmp_event])
-while True:
- event = source_vm.event_wait('MIGRATION')
- iotests.log(event, filters=[iotests.filter_qmp_event])
- if event['data']['status'] in ('completed', 'failed'):
- break
+ iotests.log('Starting migration...')
+ source_vm.qmp('migrate-set-capabilities',
+ capabilities=[{'capability': 'events', 'state': True}])
+ dest_vm.qmp('migrate-set-capabilities',
+ capabilities=[{'capability': 'events', 'state': True}])
+ iotests.log(source_vm.qmp('migrate', uri='unix:{0}'.format(migration_sock_path)))
+
+ while True:
+ event = source_vm.event_wait('MIGRATION')
+ iotests.log(event, filters=[iotests.filter_qmp_event])
+ if event['data']['status'] in ('completed', 'failed'):
+ break
--
2.13.5
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers
2017-08-24 7:21 [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers Stefan Hajnoczi
` (2 preceding siblings ...)
2017-08-24 7:22 ` [Qemu-devel] [PATCH 3/3] qemu-iotests: use context managers for resource cleanup in 194 Stefan Hajnoczi
@ 2017-08-24 8:38 ` Fam Zheng
2017-08-24 18:04 ` Stefan Hajnoczi
2017-08-31 11:26 ` Stefan Hajnoczi
4 siblings, 1 reply; 14+ messages in thread
From: Fam Zheng @ 2017-08-24 8:38 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel, Kevin Wolf
On Thu, 08/24 08:21, Stefan Hajnoczi wrote:
> Tests should declare resources upfront in a with statement. Resources are
> automatically cleaned up whether the test passes or fails:
>
> with FilePath('test.img') as img_path,
> VM() as vm:
> ...test...
> # img_path is deleted and vm is shut down automatically
Looks good but still requires test writers to learn and remember to use FilePath
and with. These are still boilerplates. Here goes my personal oppinion, so may
not be plausible:
- For VM() maybe add an atexit in the launch() method also makes sure the VM is
eventually terminated.
This means vm.shutdown() is still needed in tearDown() if there are multiple
test methods and each of them expects a clean state, but that is probably
still less typing (and also indenting) than the with approach, and also easy
to remember (otherwise a test will fail).
- For scratch how about adding atexit in iotests.main to clean up everything in
the scratch directory? The rationale is similar to above.
Fam
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers
2017-08-24 8:38 ` [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers Fam Zheng
@ 2017-08-24 18:04 ` Stefan Hajnoczi
2017-08-25 7:32 ` Fam Zheng
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Hajnoczi @ 2017-08-24 18:04 UTC (permalink / raw)
To: Fam Zheng; +Cc: Stefan Hajnoczi, Kevin Wolf, qemu-devel
On Thu, Aug 24, 2017 at 04:38:43PM +0800, Fam Zheng wrote:
> On Thu, 08/24 08:21, Stefan Hajnoczi wrote:
> > Tests should declare resources upfront in a with statement. Resources are
> > automatically cleaned up whether the test passes or fails:
> >
> > with FilePath('test.img') as img_path,
> > VM() as vm:
> > ...test...
> > # img_path is deleted and vm is shut down automatically
>
> Looks good but still requires test writers to learn and remember to use FilePath
> and with.
You cannot forget to use FilePath() unless you love typing at
os.path.join(iotests.test_dir, 'test.img'). It's much better than open
coding filename generation!
> These are still boilerplates. Here goes my personal oppinion, so may
> not be plausible:
>
> - For VM() maybe add an atexit in the launch() method also makes sure the VM is
> eventually terminated.
>
> This means vm.shutdown() is still needed in tearDown() if there are multiple
> test methods and each of them expects a clean state, but that is probably
> still less typing (and also indenting) than the with approach, and also easy
> to remember (otherwise a test will fail).
I looked into atexit before going this route. atexit does not have an
unregister() API in Python 2. This makes it ugly to use because some
tests do not want the resource to remain for the duration of the
process.
A related point is that the Python objects used by atexit handlers live
until the end of the process. They cannot be garbage collected because
the atexit handler still has a reference to them.
The with statement's identation is annoying for straightforward scripts.
More complex tests use functions anyway, so the indentation doesn't
matter there - it can be hidden by a parent function or even a
decorator.
> - For scratch how about adding atexit in iotests.main to clean up everything in
> the scratch directory? The rationale is similar to above.
If we decide to clear out TEST_DIR then it should be done in ./check,
not by iotests.py, so that all tests get the same file cleanup behavior,
regardless of the language they are written in.
Also, we can then chdir(iotests.test_dir) so filename generation isn't
necessary at all. Tests can simply use 'test.img'. I guess there may
be some cases where absolute paths are necessary, but for the most part
this would be a win.
Kevin may have an opinion on whether TEST_DIR should be cleared out or
not.
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers
2017-08-24 18:04 ` Stefan Hajnoczi
@ 2017-08-25 7:32 ` Fam Zheng
2017-08-25 8:52 ` Stefan Hajnoczi
0 siblings, 1 reply; 14+ messages in thread
From: Fam Zheng @ 2017-08-25 7:32 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On Thu, 08/24 19:04, Stefan Hajnoczi wrote:
> On Thu, Aug 24, 2017 at 04:38:43PM +0800, Fam Zheng wrote:
> > On Thu, 08/24 08:21, Stefan Hajnoczi wrote:
> > > Tests should declare resources upfront in a with statement. Resources are
> > > automatically cleaned up whether the test passes or fails:
> > >
> > > with FilePath('test.img') as img_path,
> > > VM() as vm:
> > > ...test...
> > > # img_path is deleted and vm is shut down automatically
> >
> > Looks good but still requires test writers to learn and remember to use FilePath
> > and with.
>
> You cannot forget to use FilePath() unless you love typing at
> os.path.join(iotests.test_dir, 'test.img'). It's much better than open
> coding filename generation!
>
> > These are still boilerplates. Here goes my personal oppinion, so may
> > not be plausible:
> >
> > - For VM() maybe add an atexit in the launch() method also makes sure the VM is
> > eventually terminated.
> >
> > This means vm.shutdown() is still needed in tearDown() if there are multiple
> > test methods and each of them expects a clean state, but that is probably
> > still less typing (and also indenting) than the with approach, and also easy
> > to remember (otherwise a test will fail).
>
> I looked into atexit before going this route. atexit does not have an
> unregister() API in Python 2. This makes it ugly to use because some
> tests do not want the resource to remain for the duration of the
> process.
>
> A related point is that the Python objects used by atexit handlers live
> until the end of the process. They cannot be garbage collected because
> the atexit handler still has a reference to them.
I think this shortcoming can be solved with a clean up list ("all problems in
computer science can be solved by another level of indirection"):
_clean_up_list = set()
def _clean_up_handler():
for i in _clean_up_list:
try:
i()
except:
pass
atexit.register(_clean_up_handler)
class VM(...):
def launch():
...
_clean_up_list.add(self.launch)
def shutdown():
_clean_up_list.remove(self.launch)
...
>
> The with statement's identation is annoying for straightforward scripts.
> More complex tests use functions anyway, so the indentation doesn't
> matter there - it can be hidden by a parent function or even a
> decorator.
>
> > - For scratch how about adding atexit in iotests.main to clean up everything in
> > the scratch directory? The rationale is similar to above.
>
> If we decide to clear out TEST_DIR then it should be done in ./check,
> not by iotests.py, so that all tests get the same file cleanup behavior,
> regardless of the language they are written in.
>
> Also, we can then chdir(iotests.test_dir) so filename generation isn't
> necessary at all. Tests can simply use 'test.img'. I guess there may
> be some cases where absolute paths are necessary, but for the most part
> this would be a win.
Good point, ./check can even invoke scripts with scratch as their working
directory.
Fam
>
> Kevin may have an opinion on whether TEST_DIR should be cleared out or
> not.
>
> Stefan
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers
2017-08-25 7:32 ` Fam Zheng
@ 2017-08-25 8:52 ` Stefan Hajnoczi
2017-08-25 9:29 ` Fam Zheng
2017-08-28 10:55 ` Markus Armbruster
0 siblings, 2 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2017-08-25 8:52 UTC (permalink / raw)
To: Fam Zheng; +Cc: Stefan Hajnoczi, Kevin Wolf, qemu-devel
On Fri, Aug 25, 2017 at 03:32:29PM +0800, Fam Zheng wrote:
> On Thu, 08/24 19:04, Stefan Hajnoczi wrote:
> > On Thu, Aug 24, 2017 at 04:38:43PM +0800, Fam Zheng wrote:
> > > On Thu, 08/24 08:21, Stefan Hajnoczi wrote:
> > > > Tests should declare resources upfront in a with statement. Resources are
> > > > automatically cleaned up whether the test passes or fails:
> > > >
> > > > with FilePath('test.img') as img_path,
> > > > VM() as vm:
> > > > ...test...
> > > > # img_path is deleted and vm is shut down automatically
> > >
> > > Looks good but still requires test writers to learn and remember to use FilePath
> > > and with.
> >
> > You cannot forget to use FilePath() unless you love typing at
> > os.path.join(iotests.test_dir, 'test.img'). It's much better than open
> > coding filename generation!
> >
> > > These are still boilerplates. Here goes my personal oppinion, so may
> > > not be plausible:
> > >
> > > - For VM() maybe add an atexit in the launch() method also makes sure the VM is
> > > eventually terminated.
> > >
> > > This means vm.shutdown() is still needed in tearDown() if there are multiple
> > > test methods and each of them expects a clean state, but that is probably
> > > still less typing (and also indenting) than the with approach, and also easy
> > > to remember (otherwise a test will fail).
> >
> > I looked into atexit before going this route. atexit does not have an
> > unregister() API in Python 2. This makes it ugly to use because some
> > tests do not want the resource to remain for the duration of the
> > process.
> >
> > A related point is that the Python objects used by atexit handlers live
> > until the end of the process. They cannot be garbage collected because
> > the atexit handler still has a reference to them.
>
> I think this shortcoming can be solved with a clean up list ("all problems in
> computer science can be solved by another level of indirection"):
>
> _clean_up_list = set()
> def _clean_up_handler():
> for i in _clean_up_list:
> try:
> i()
> except:
> pass
>
> atexit.register(_clean_up_handler)
>
> class VM(...):
>
> def launch():
> ...
> _clean_up_list.add(self.launch)
>
> def shutdown():
> _clean_up_list.remove(self.launch)
> ...
atexit is still less powerful than context managers because its scope is
fixed. Handler functions are only called when the process terminates.
Many test cases do not want resources (especially the VMs) around
forever because they run several iterations or sub-tests.
The with statement can be used both for process-lifetime and for more
fine-grained scoping. That's why I chose it.
If you stick to atexit then sub-tests or iterations require manual
vm.shutdown() - something that is not necessary using the with
statement.
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers
2017-08-25 8:52 ` Stefan Hajnoczi
@ 2017-08-25 9:29 ` Fam Zheng
2017-08-30 12:44 ` Stefan Hajnoczi
2017-08-28 10:55 ` Markus Armbruster
1 sibling, 1 reply; 14+ messages in thread
From: Fam Zheng @ 2017-08-25 9:29 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Stefan Hajnoczi, Kevin Wolf, qemu-devel
On Fri, 08/25 09:52, Stefan Hajnoczi wrote:
> On Fri, Aug 25, 2017 at 03:32:29PM +0800, Fam Zheng wrote:
> > On Thu, 08/24 19:04, Stefan Hajnoczi wrote:
> > > On Thu, Aug 24, 2017 at 04:38:43PM +0800, Fam Zheng wrote:
> > > > On Thu, 08/24 08:21, Stefan Hajnoczi wrote:
> > > > > Tests should declare resources upfront in a with statement. Resources are
> > > > > automatically cleaned up whether the test passes or fails:
> > > > >
> > > > > with FilePath('test.img') as img_path,
> > > > > VM() as vm:
> > > > > ...test...
> > > > > # img_path is deleted and vm is shut down automatically
> > > >
> > > > Looks good but still requires test writers to learn and remember to use FilePath
> > > > and with.
> > >
> > > You cannot forget to use FilePath() unless you love typing at
> > > os.path.join(iotests.test_dir, 'test.img'). It's much better than open
> > > coding filename generation!
> > >
> > > > These are still boilerplates. Here goes my personal oppinion, so may
> > > > not be plausible:
> > > >
> > > > - For VM() maybe add an atexit in the launch() method also makes sure the VM is
> > > > eventually terminated.
> > > >
> > > > This means vm.shutdown() is still needed in tearDown() if there are multiple
> > > > test methods and each of them expects a clean state, but that is probably
> > > > still less typing (and also indenting) than the with approach, and also easy
> > > > to remember (otherwise a test will fail).
> > >
> > > I looked into atexit before going this route. atexit does not have an
> > > unregister() API in Python 2. This makes it ugly to use because some
> > > tests do not want the resource to remain for the duration of the
> > > process.
> > >
> > > A related point is that the Python objects used by atexit handlers live
> > > until the end of the process. They cannot be garbage collected because
> > > the atexit handler still has a reference to them.
> >
> > I think this shortcoming can be solved with a clean up list ("all problems in
> > computer science can be solved by another level of indirection"):
> >
> > _clean_up_list = set()
> > def _clean_up_handler():
> > for i in _clean_up_list:
> > try:
> > i()
> > except:
> > pass
> >
> > atexit.register(_clean_up_handler)
> >
> > class VM(...):
> >
> > def launch():
> > ...
> > _clean_up_list.add(self.launch)
> >
> > def shutdown():
> > _clean_up_list.remove(self.launch)
> > ...
>
> atexit is still less powerful than context managers because its scope is
> fixed. Handler functions are only called when the process terminates.
> Many test cases do not want resources (especially the VMs) around
> forever because they run several iterations or sub-tests.
>
> The with statement can be used both for process-lifetime and for more
> fine-grained scoping. That's why I chose it.
>
> If you stick to atexit then sub-tests or iterations require manual
> vm.shutdown() - something that is not necessary using the with
> statement.
Sure!
I just think that if leftover VM instances are a concern and not all test code
are converted to "with", having the atexit handler in addition may provide more
robustness.
Fam
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] qemu.py: make VM() a context manager
2017-08-24 7:22 ` [Qemu-devel] [PATCH 1/3] qemu.py: make VM() a context manager Stefan Hajnoczi
@ 2017-08-25 12:44 ` Eduardo Habkost
0 siblings, 0 replies; 14+ messages in thread
From: Eduardo Habkost @ 2017-08-25 12:44 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel, Kevin Wolf, Daniel Berrange
On Thu, Aug 24, 2017 at 08:22:00AM +0100, Stefan Hajnoczi wrote:
> There are a number of ways to ensure that the QEMU process is shut down
> when the test ends, including atexit.register(), try: finally:, or
> unittest.teardown() methods. All of these require extra code and the
> programmer must remember to add vm.shutdown().
>
> A nice solution is context managers:
>
> with VM(binary) as vm:
> ...
> # vm is guaranteed to be shut down here
>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
--
Eduardo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers
2017-08-25 8:52 ` Stefan Hajnoczi
2017-08-25 9:29 ` Fam Zheng
@ 2017-08-28 10:55 ` Markus Armbruster
1 sibling, 0 replies; 14+ messages in thread
From: Markus Armbruster @ 2017-08-28 10:55 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Fam Zheng, Kevin Wolf, Stefan Hajnoczi, qemu-devel
Stefan Hajnoczi <stefanha@redhat.com> writes:
> On Fri, Aug 25, 2017 at 03:32:29PM +0800, Fam Zheng wrote:
>> On Thu, 08/24 19:04, Stefan Hajnoczi wrote:
>> > On Thu, Aug 24, 2017 at 04:38:43PM +0800, Fam Zheng wrote:
>> > > On Thu, 08/24 08:21, Stefan Hajnoczi wrote:
>> > > > Tests should declare resources upfront in a with statement. Resources are
>> > > > automatically cleaned up whether the test passes or fails:
>> > > >
>> > > > with FilePath('test.img') as img_path,
>> > > > VM() as vm:
>> > > > ...test...
>> > > > # img_path is deleted and vm is shut down automatically
>> > >
>> > > Looks good but still requires test writers to learn and remember to use FilePath
>> > > and with.
>> >
>> > You cannot forget to use FilePath() unless you love typing at
>> > os.path.join(iotests.test_dir, 'test.img'). It's much better than open
>> > coding filename generation!
>> >
>> > > These are still boilerplates. Here goes my personal oppinion, so may
>> > > not be plausible:
>> > >
>> > > - For VM() maybe add an atexit in the launch() method also makes sure the VM is
>> > > eventually terminated.
>> > >
>> > > This means vm.shutdown() is still needed in tearDown() if there are multiple
>> > > test methods and each of them expects a clean state, but that is probably
>> > > still less typing (and also indenting) than the with approach, and also easy
>> > > to remember (otherwise a test will fail).
>> >
>> > I looked into atexit before going this route. atexit does not have an
>> > unregister() API in Python 2. This makes it ugly to use because some
>> > tests do not want the resource to remain for the duration of the
>> > process.
>> >
>> > A related point is that the Python objects used by atexit handlers live
>> > until the end of the process. They cannot be garbage collected because
>> > the atexit handler still has a reference to them.
>>
>> I think this shortcoming can be solved with a clean up list ("all problems in
>> computer science can be solved by another level of indirection"):
>>
>> _clean_up_list = set()
>> def _clean_up_handler():
>> for i in _clean_up_list:
>> try:
>> i()
>> except:
>> pass
>>
>> atexit.register(_clean_up_handler)
>>
>> class VM(...):
>>
>> def launch():
>> ...
>> _clean_up_list.add(self.launch)
>>
>> def shutdown():
>> _clean_up_list.remove(self.launch)
>> ...
>
> atexit is still less powerful than context managers because its scope is
> fixed. Handler functions are only called when the process terminates.
> Many test cases do not want resources (especially the VMs) around
> forever because they run several iterations or sub-tests.
>
> The with statement can be used both for process-lifetime and for more
> fine-grained scoping. That's why I chose it.
Moreover, context managers are Pythonic.
> If you stick to atexit then sub-tests or iterations require manual
> vm.shutdown() - something that is not necessary using the with
> statement.
>
> Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers
2017-08-25 9:29 ` Fam Zheng
@ 2017-08-30 12:44 ` Stefan Hajnoczi
2017-08-30 12:54 ` Fam Zheng
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Hajnoczi @ 2017-08-30 12:44 UTC (permalink / raw)
To: Fam Zheng; +Cc: Stefan Hajnoczi, Kevin Wolf, qemu-devel
On Fri, Aug 25, 2017 at 05:29:14PM +0800, Fam Zheng wrote:
> On Fri, 08/25 09:52, Stefan Hajnoczi wrote:
> > On Fri, Aug 25, 2017 at 03:32:29PM +0800, Fam Zheng wrote:
> > > On Thu, 08/24 19:04, Stefan Hajnoczi wrote:
> > > > On Thu, Aug 24, 2017 at 04:38:43PM +0800, Fam Zheng wrote:
> > > > > On Thu, 08/24 08:21, Stefan Hajnoczi wrote:
> > > > > > Tests should declare resources upfront in a with statement. Resources are
> > > > > > automatically cleaned up whether the test passes or fails:
> > > > > >
> > > > > > with FilePath('test.img') as img_path,
> > > > > > VM() as vm:
> > > > > > ...test...
> > > > > > # img_path is deleted and vm is shut down automatically
> > > > >
> > > > > Looks good but still requires test writers to learn and remember to use FilePath
> > > > > and with.
> > > >
> > > > You cannot forget to use FilePath() unless you love typing at
> > > > os.path.join(iotests.test_dir, 'test.img'). It's much better than open
> > > > coding filename generation!
> > > >
> > > > > These are still boilerplates. Here goes my personal oppinion, so may
> > > > > not be plausible:
> > > > >
> > > > > - For VM() maybe add an atexit in the launch() method also makes sure the VM is
> > > > > eventually terminated.
> > > > >
> > > > > This means vm.shutdown() is still needed in tearDown() if there are multiple
> > > > > test methods and each of them expects a clean state, but that is probably
> > > > > still less typing (and also indenting) than the with approach, and also easy
> > > > > to remember (otherwise a test will fail).
> > > >
> > > > I looked into atexit before going this route. atexit does not have an
> > > > unregister() API in Python 2. This makes it ugly to use because some
> > > > tests do not want the resource to remain for the duration of the
> > > > process.
> > > >
> > > > A related point is that the Python objects used by atexit handlers live
> > > > until the end of the process. They cannot be garbage collected because
> > > > the atexit handler still has a reference to them.
> > >
> > > I think this shortcoming can be solved with a clean up list ("all problems in
> > > computer science can be solved by another level of indirection"):
> > >
> > > _clean_up_list = set()
> > > def _clean_up_handler():
> > > for i in _clean_up_list:
> > > try:
> > > i()
> > > except:
> > > pass
> > >
> > > atexit.register(_clean_up_handler)
> > >
> > > class VM(...):
> > >
> > > def launch():
> > > ...
> > > _clean_up_list.add(self.launch)
> > >
> > > def shutdown():
> > > _clean_up_list.remove(self.launch)
> > > ...
> >
> > atexit is still less powerful than context managers because its scope is
> > fixed. Handler functions are only called when the process terminates.
> > Many test cases do not want resources (especially the VMs) around
> > forever because they run several iterations or sub-tests.
> >
> > The with statement can be used both for process-lifetime and for more
> > fine-grained scoping. That's why I chose it.
> >
> > If you stick to atexit then sub-tests or iterations require manual
> > vm.shutdown() - something that is not necessary using the with
> > statement.
>
> Sure!
>
> I just think that if leftover VM instances are a concern and not all test code
> are converted to "with", having the atexit handler in addition may provide more
> robustness.
Okay, I checked this. Existing code doesn't need to be changed (yet)
because:
1. Most existing code uses unittest's setUp()/tearDown() and already
correctly handles cleanup when the test fails.
2. The LUKS crypto test doesn't use unittest but also doesn't use VM(),
so it doesn't need.
Are you happy for me to merge this series?
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers
2017-08-30 12:44 ` Stefan Hajnoczi
@ 2017-08-30 12:54 ` Fam Zheng
0 siblings, 0 replies; 14+ messages in thread
From: Fam Zheng @ 2017-08-30 12:54 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On Wed, 08/30 13:44, Stefan Hajnoczi wrote:
> On Fri, Aug 25, 2017 at 05:29:14PM +0800, Fam Zheng wrote:
> > On Fri, 08/25 09:52, Stefan Hajnoczi wrote:
> > > On Fri, Aug 25, 2017 at 03:32:29PM +0800, Fam Zheng wrote:
> > > > On Thu, 08/24 19:04, Stefan Hajnoczi wrote:
> > > > > On Thu, Aug 24, 2017 at 04:38:43PM +0800, Fam Zheng wrote:
> > > > > > On Thu, 08/24 08:21, Stefan Hajnoczi wrote:
> > > > > > > Tests should declare resources upfront in a with statement. Resources are
> > > > > > > automatically cleaned up whether the test passes or fails:
> > > > > > >
> > > > > > > with FilePath('test.img') as img_path,
> > > > > > > VM() as vm:
> > > > > > > ...test...
> > > > > > > # img_path is deleted and vm is shut down automatically
> > > > > >
> > > > > > Looks good but still requires test writers to learn and remember to use FilePath
> > > > > > and with.
> > > > >
> > > > > You cannot forget to use FilePath() unless you love typing at
> > > > > os.path.join(iotests.test_dir, 'test.img'). It's much better than open
> > > > > coding filename generation!
> > > > >
> > > > > > These are still boilerplates. Here goes my personal oppinion, so may
> > > > > > not be plausible:
> > > > > >
> > > > > > - For VM() maybe add an atexit in the launch() method also makes sure the VM is
> > > > > > eventually terminated.
> > > > > >
> > > > > > This means vm.shutdown() is still needed in tearDown() if there are multiple
> > > > > > test methods and each of them expects a clean state, but that is probably
> > > > > > still less typing (and also indenting) than the with approach, and also easy
> > > > > > to remember (otherwise a test will fail).
> > > > >
> > > > > I looked into atexit before going this route. atexit does not have an
> > > > > unregister() API in Python 2. This makes it ugly to use because some
> > > > > tests do not want the resource to remain for the duration of the
> > > > > process.
> > > > >
> > > > > A related point is that the Python objects used by atexit handlers live
> > > > > until the end of the process. They cannot be garbage collected because
> > > > > the atexit handler still has a reference to them.
> > > >
> > > > I think this shortcoming can be solved with a clean up list ("all problems in
> > > > computer science can be solved by another level of indirection"):
> > > >
> > > > _clean_up_list = set()
> > > > def _clean_up_handler():
> > > > for i in _clean_up_list:
> > > > try:
> > > > i()
> > > > except:
> > > > pass
> > > >
> > > > atexit.register(_clean_up_handler)
> > > >
> > > > class VM(...):
> > > >
> > > > def launch():
> > > > ...
> > > > _clean_up_list.add(self.launch)
> > > >
> > > > def shutdown():
> > > > _clean_up_list.remove(self.launch)
> > > > ...
> > >
> > > atexit is still less powerful than context managers because its scope is
> > > fixed. Handler functions are only called when the process terminates.
> > > Many test cases do not want resources (especially the VMs) around
> > > forever because they run several iterations or sub-tests.
> > >
> > > The with statement can be used both for process-lifetime and for more
> > > fine-grained scoping. That's why I chose it.
> > >
> > > If you stick to atexit then sub-tests or iterations require manual
> > > vm.shutdown() - something that is not necessary using the with
> > > statement.
> >
> > Sure!
> >
> > I just think that if leftover VM instances are a concern and not all test code
> > are converted to "with", having the atexit handler in addition may provide more
> > robustness.
>
> Okay, I checked this. Existing code doesn't need to be changed (yet)
> because:
>
> 1. Most existing code uses unittest's setUp()/tearDown() and already
> correctly handles cleanup when the test fails.
>
> 2. The LUKS crypto test doesn't use unittest but also doesn't use VM(),
> so it doesn't need.
>
> Are you happy for me to merge this series?
Yes. Sounds good! Thanks.
Fam
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers
2017-08-24 7:21 [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers Stefan Hajnoczi
` (3 preceding siblings ...)
2017-08-24 8:38 ` [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers Fam Zheng
@ 2017-08-31 11:26 ` Stefan Hajnoczi
4 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2017-08-31 11:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Daniel Berrange
On Thu, Aug 24, 2017 at 08:21:59AM +0100, Stefan Hajnoczi wrote:
> This series introduces context managers for the two most commonly used
> resources: files and VMs. Context managers eliminate the need to call a
> cleanup function explicitly.
>
> Tests should declare resources upfront in a with statement. Resources are
> automatically cleaned up whether the test passes or fails:
>
> with FilePath('test.img') as img_path,
> VM() as vm:
> ...test...
> # img_path is deleted and vm is shut down automatically
>
> The final patch converts 194 to use context managers instead of
> atexit.register(). This makes the code shorter and easier to read.
>
> Stefan Hajnoczi (3):
> qemu.py: make VM() a context manager
> iotests.py: add FilePath context manager
> qemu-iotests: use context managers for resource cleanup in 194
>
> scripts/qemu.py | 16 ++++++++-
> tests/qemu-iotests/194 | 83 +++++++++++++++++++++----------------------
> tests/qemu-iotests/iotests.py | 26 ++++++++++++++
> 3 files changed, 82 insertions(+), 43 deletions(-)
>
> --
> 2.13.5
>
Thanks, applied to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-08-31 11:55 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-24 7:21 [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers Stefan Hajnoczi
2017-08-24 7:22 ` [Qemu-devel] [PATCH 1/3] qemu.py: make VM() a context manager Stefan Hajnoczi
2017-08-25 12:44 ` Eduardo Habkost
2017-08-24 7:22 ` [Qemu-devel] [PATCH 2/3] iotests.py: add FilePath " Stefan Hajnoczi
2017-08-24 7:22 ` [Qemu-devel] [PATCH 3/3] qemu-iotests: use context managers for resource cleanup in 194 Stefan Hajnoczi
2017-08-24 8:38 ` [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers Fam Zheng
2017-08-24 18:04 ` Stefan Hajnoczi
2017-08-25 7:32 ` Fam Zheng
2017-08-25 8:52 ` Stefan Hajnoczi
2017-08-25 9:29 ` Fam Zheng
2017-08-30 12:44 ` Stefan Hajnoczi
2017-08-30 12:54 ` Fam Zheng
2017-08-28 10:55 ` Markus Armbruster
2017-08-31 11:26 ` Stefan Hajnoczi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.