Remove some external command callouts
cover Remove some external command callouts
Calling out to external commands is slow, noisy, and unreliable. This patchset replaces two such cases with Python equivalents.
Avi Kivity (2):
Replace subprocess 'rm *.ppm' by equivalent Python code Convert images to JPEG using PIL instead of an external program
client/tests/kvm/kvm_guest_wizard.py | 6 +++--- client/tests/kvm/kvm_preprocessing.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-)
PATCH 1/2 Replace subprocess 'rm *.ppm' by equivalent Python code
--- client/tests/kvm/kvm_preprocessing.py | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
client/tests/kvm/kvm_preprocessing.py +3 -3
@@ -1,4 +1,4 @@ -import sys, os, time, commands, re, logging, signal +import sys, os, time, commands, re, logging, signal, glob from autotest_lib.client.bin import test from autotest_lib.client.common_lib import error import kvm_vm, kvm_utils, kvm_subprocess
@@ -269,8 +269,8 @@ def postprocess(test, params, env): if params.get("keep_ppm_files") != "yes": logging.debug("'keep_ppm_files' not specified; removing all PPM files" " from debug dir...") - rm_cmd = "rm -vf %s" % os.path.join(test.debugdir, "*.ppm") - kvm_subprocess.run_fg(rm_cmd, logging.debug, "(rm) ", timeout=5.0) + for f in glob.glob(os.path.join(test.debugdir, '*.ppm')): + os.unlink(f) # Execute any post_commands if params.get("post_command"):
Signature · 1 line
1.6.3.3
PATCH 2/2 Convert images to JPEG using PIL instead of an external program
In reply to Avi KivityThis is faster since we don't need to fork/exec/wait for an external program each time.
--- client/tests/kvm/kvm_guest_wizard.py | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
client/tests/kvm/kvm_guest_wizard.py +3 -3
@@ -1,6 +1,7 @@ import os, time, md5, re, shutil, logging from autotest_lib.client.common_lib import utils, error import kvm_utils, ppm_utils, kvm_subprocess +import PIL.Image """ Utilities to perform automatic guest installation using step files.
@@ -110,9 +111,8 @@ def barrier_2(vm, words, params, debug_dir, data_scrdump_filename, history_scrdump_filename = os.path.join(history_dir, "scrdump-step_%s-%s.jpg" % (current_step_num, time.strftime("%Y%m%d-%H%M%S"))) - kvm_subprocess.run_fg("convert -quality 30 %s %s" % - (scrdump_filename, history_scrdump_filename), - logging.debug, "(convert) ", timeout=30) + image = PIL.Image.open(scrdump_filename) + image.save(history_scrdump_filename, format = 'JPEG', quality = 30) # Compare md5sum of barrier region with the expected md5sum calced_md5sum = ppm_utils.get_region_md5sum(w, h, data, x1, y1, dx, dy,
Signature · 1 line
1.6.3.3
Re: [PATCH KVM-AUTOTEST 2/2] Convert images to JPEG using PIL instead of an external program
Quoted 21 lines · Avi Kivity
This is faster since we don't need to fork/exec/wait for an external program each time.
Signed-off-by Avi Kivity <[email protected]>--- client/tests/kvm/kvm_guest_wizard.py | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)client/tests/kvm/kvm_guest_wizard.py +1
-0@@ -1,6 +1,7 @@ import os, time, md5, re, shutil, logging from autotest_lib.client.common_lib import utils, error import kvm_utils, ppm_utils, kvm_subprocess +import PIL.Image """ Utilities to perform automatic guest installation using step files.
client/tests/kvm/kvm_guest_wizard.py def barrier_2(vm, words, params, debug_dir, data_scrdump_filename, · line 115
Quoted 8 lines
client/tests/kvm/kvm_guest_wizard.py +2
-3@@ -110,9 +111,8 @@ def barrier_2(vm, words, params, debug_dir, data_scrdump_filename, history_scrdump_filename = os.path.join(history_dir, "scrdump-step_%s-%s.jpg" % (current_step_num, time.strftime("%Y%m%d-%H%M%S"))) - kvm_subprocess.run_fg("convert -quality 30 %s %s" % - (scrdump_filename, history_scrdump_filename), - logging.debug, "(convert) ", timeout=30) + image = PIL.Image.open(scrdump_filename) + image.save(history_scrdump_filename, format = 'JPEG', quality = 30)
Looks great, but since the python imaging library is an external library, we need to handle import failures. We can't guarantee that it will allways be installed, so we just degrade functionality gracefully in the case is not present.
Quoted 2 lines
client/tests/kvm/kvm_guest_wizard.py +0
-0# Compare md5sum of barrier region with the expected md5sum calced_md5sum = ppm_utils.get_region_md5sum(w, h, data, x1, y1, dx, dy,
Re: [PATCH KVM-AUTOTEST 2/2] Convert images to JPEG using PIL instead of an external program
Quoted 12 lines · Lucas Meneghel Rodrigues
client/tests/kvm/kvm_guest_wizard.py def barrier_2(vm, words, params, debug_dir, data_scrdump_filename, · line 116
Quoted 10 lines
client/tests/kvm/kvm_guest_wizard.py +2
-3@@ -110,9 +111,8 @@ def barrier_2(vm, words, params, debug_dir, data_scrdump_filename, history_scrdump_filename = os.path.join(history_dir, "scrdump-step_%s-%s.jpg" % (current_step_num, time.strftime("%Y%m%d-%H%M%S"))) - kvm_subprocess.run_fg("convert -quality 30 %s %s" % - (scrdump_filename, history_scrdump_filename), - logging.debug, "(convert) ", timeout=30) + image = PIL.Image.open(scrdump_filename) + image.save(history_scrdump_filename, format = 'JPEG', quality = 30)Utilities to perform automatic guest installation using step files.
Quoted 5 lines
Looks great, but since the python imaging library is an external library, we need to handle import failures. We can't guarantee that it will allways be installed, so we just degrade functionality gracefully in the case is not present.
Why not require it unconditionally? It's not new and installing it is trivial.
Signature · 1 line
error compiling committee.c: too many arguments to function
Re: [PATCH KVM-AUTOTEST 2/2] Convert images to JPEG using PIL instead of an external program
Quoted 13 lines · Avi Kivity
Quoted 12 lines · Lucas Meneghel Rodrigues
client/tests/kvm/kvm_guest_wizard.py def barrier_2(vm, words, params, debug_dir, data_scrdump_filename, · line 116
Quoted 10 lines
client/tests/kvm/kvm_guest_wizard.py +2
-3@@ -110,9 +111,8 @@ def barrier_2(vm, words, params, debug_dir, data_scrdump_filename, history_scrdump_filename = os.path.join(history_dir, "scrdump-step_%s-%s.jpg" % (current_step_num, time.strftime("%Y%m%d-%H%M%S"))) - kvm_subprocess.run_fg("convert -quality 30 %s %s" % - (scrdump_filename, history_scrdump_filename), - logging.debug, "(convert) ", timeout=30) + image = PIL.Image.open(scrdump_filename) + image.save(history_scrdump_filename, format = 'JPEG', quality = 30)Utilities to perform automatic guest installation using step files.
Quoted 7 lines
Quoted 4 lines
Looks great, but since the python imaging library is an external library, we need to handle import failures. We can't guarantee that it will allways be installed, so we just degrade functionality gracefully in the case is not present.
Why not require it unconditionally? It's not new and installing it is trivial.
Just general autotest policy I try to follow. For client side testing, we try to make as few assumptions as possible about the machine where we are going to run the client, except to have a python install > 2.4 and a working toolchain (to build C/C++ programs). We try to not rely on anything but those 2 items. For the autotest server machine it's OK to depend on external libraries.
I also agree that this is a special case:
* PIL is almost ubiquitous across linux distributions
* It comes installed by default in most setups
* It's not like we are going to test kvm on dramatically old linux
systems
But I prefer to follow the project policy when possible. The reason why I accepted the original code that Michael wrote to perform the conversion was graceful degradation of functionality (if you don't have ImageMagick installed, the test will not abort).
We could handle import failures in a way that supports graceful degradation and prints a warning message saying that the user don't have PIL installed, and that the conversion functionality only works when having PIL installed. What do you think? If you still think it's not worth the effort of doing this import handling I am going to think a little bit more, and eventually apply this.
Re: [PATCH KVM-AUTOTEST 2/2] Convert images to JPEG using PIL instead of an external program
Quoted 5 lines · Lucas Meneghel Rodrigues
But I prefer to follow the project policy when possible. The reason why I accepted the original code that Michael wrote to perform the conversion was graceful degradation of functionality (if you don't have ImageMagick installed, the test will not abort).
Well, policy is policy. See the attached (untested).
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 1 line
From
16537ea5270d65837cbd04c13c7289b0714a6d64Mon Sep 17 00:00:00 2001
From: Avi Kivity <[email protected]>
Date: Wed, 12 Aug 2009 12:00:52 +0300
Subject: [KVM-AUTOTEST PATCH] Convert images to JPEG using PIL instead of an external program
This is faster since we don't need to fork/exec/wait for an external program each time.
--- client/tests/kvm/kvm_guest_wizard.py | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)
client/tests/kvm/kvm_guest_wizard.py +8 -3
@@ -110,9 +110,14 @@ def barrier_2(vm, words, params, debug_dir, data_scrdump_filename, history_scrdump_filename = os.path.join(history_dir, "scrdump-step_%s-%s.jpg" % (current_step_num, time.strftime("%Y%m%d-%H%M%S"))) - kvm_subprocess.run_fg("convert -quality 30 %s %s" % - (scrdump_filename, history_scrdump_filename), - logging.debug, "(convert) ", timeout=30) + def convert_image(src, dest): + try: + import PIL.Image + image = PIL.Image.open(src) + image.save(dest, format = 'JPEG', quality = 30) + except: + pass + convert_image(scrdump_filename, history_scrdump_filename) # Compare md5sum of barrier region with the expected md5sum calced_md5sum = ppm_utils.get_region_md5sum(w, h, data, x1, y1, dx, dy,
Signature · 1 line
1.6.3.3
Re: [PATCH KVM-AUTOTEST 2/2] Convert images to JPEG using PIL instead of an external program
Quoted 8 lines · Avi Kivity
Quoted 5 lines · Lucas Meneghel Rodrigues
But I prefer to follow the project policy when possible. The reason why I accepted the original code that Michael wrote to perform the conversion was graceful degradation of functionality (if you don't have ImageMagick installed, the test will not abort).
Well, policy is policy. See the attached (untested).
Ok Avi, I've spotted other places where we are also making unneeded calls to rm and mogrify, so I changed them, combined the patches, changed the exception trap only to catch the exceptions we are waiting for, and send the patch to the mailing list. Please let me know what you think.
Lucas
Re: [PATCH KVM-AUTOTEST 2/2] Convert images to JPEG using PIL instead of an external program
Quoted 6 lines · Lucas Meneghel Rodrigues
Ok Avi, I've spotted other places where we are also making unneeded calls to rm and mogrify, so I changed them, combined the patches, changed the exception trap only to catch the exceptions we are waiting for, and send the patch to the mailing list. Please let me know what you think.
Your rework looks good, thanks.
Signature · 1 line
error compiling committee.c: too many arguments to function