[PATCH KVM-AUTOTEST 0/2] Remove some external command callouts

[PATCH KVM-AUTOTEST 0/2] Remove some external command callouts

[Avi Kivity]

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 KVM-AUTOTEST 1/2] Replace subprocess 'rm *.ppm' by equivalent Python code

[Avi Kivity]

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 client/tests/kvm/kvm_preprocessing.py |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/client/tests/kvm/kvm_preprocessing.py b/client/tests/kvm/kvm_preprocessing.py
index d118826..c3772a3 100644
--- a/client/tests/kvm/kvm_preprocessing.py
+++ b/client/tests/kvm/kvm_preprocessing.py
@@ -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"):
-- 
1.6.3.3

[PATCH KVM-AUTOTEST 2/2] Convert images to JPEG using PIL instead of an external program

[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 <avi@redhat.com>
---
 client/tests/kvm/kvm_guest_wizard.py |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/client/tests/kvm/kvm_guest_wizard.py b/client/tests/kvm/kvm_guest_wizard.py
index 73b830e..f9e5476 100644
--- a/client/tests/kvm/kvm_guest_wizard.py
+++ b/client/tests/kvm/kvm_guest_wizard.py
@@ -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,
-- 
1.6.3.3

Re: [PATCH KVM-AUTOTEST 2/2] Convert images to JPEG using PIL instead of an external program

[Lucas Meneghel Rodrigues]

On Wed, 2009-08-12 at 12:34 +0300, Avi Kivity wrote:
> This is faster since we don't need to fork/exec/wait for an external
> program each time.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  client/tests/kvm/kvm_guest_wizard.py |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/client/tests/kvm/kvm_guest_wizard.py b/client/tests/kvm/kvm_guest_wizard.py
> index 73b830e..f9e5476 100644
> --- a/client/tests/kvm/kvm_guest_wizard.py
> +++ b/client/tests/kvm/kvm_guest_wizard.py
> @@ -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)

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.

>          # 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

[Avi Kivity]

On 08/12/2009 03:44 PM, Lucas Meneghel Rodrigues wrote:
>> @@ -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.
>
> 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.

-- 
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

[Lucas Meneghel Rodrigues]

On Wed, 2009-08-12 at 16:00 +0300, Avi Kivity wrote:
> On 08/12/2009 03:44 PM, Lucas Meneghel Rodrigues wrote:
> >> @@ -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.
> >
> > 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

[Avi Kivity]

[-- Attachment #1: Type: text/plain, Size: 447 bytes --]

On 08/12/2009 04:26 PM, Lucas Meneghel Rodrigues wrote:
> 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).

-- 
error compiling committee.c: too many arguments to function


[-- Attachment #2: 0001-Convert-images-to-JPEG-using-PIL-instead-of-an-exter.patch --]
[-- Type: text/x-patch, Size: 1721 bytes --]

>From 16537ea5270d65837cbd04c13c7289b0714a6d64 Mon Sep 17 00:00:00 2001
From: Avi Kivity <avi@redhat.com>
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.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 client/tests/kvm/kvm_guest_wizard.py |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/client/tests/kvm/kvm_guest_wizard.py b/client/tests/kvm/kvm_guest_wizard.py
index 73b830e..f3cc482 100644
--- a/client/tests/kvm/kvm_guest_wizard.py
+++ b/client/tests/kvm/kvm_guest_wizard.py
@@ -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,
-- 
1.6.3.3

Re: [PATCH KVM-AUTOTEST 2/2] Convert images to JPEG using PIL instead of an external program

[Lucas Meneghel Rodrigues]

On Wed, 2009-08-12 at 16:33 +0300, Avi Kivity wrote:
> On 08/12/2009 04:26 PM, Lucas Meneghel Rodrigues wrote:
> > 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

[Avi Kivity]

On 08/12/2009 06:01 PM, Lucas Meneghel Rodrigues wrote:
> 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.

-- 
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

[Michael Goldish]

----- "Avi Kivity" <avi@redhat.com> wrote:

> This is faster since we don't need to fork/exec/wait for an external
> program each time.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  client/tests/kvm/kvm_guest_wizard.py |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/client/tests/kvm/kvm_guest_wizard.py
> b/client/tests/kvm/kvm_guest_wizard.py
> index 73b830e..f9e5476 100644
> --- a/client/tests/kvm/kvm_guest_wizard.py
> +++ b/client/tests/kvm/kvm_guest_wizard.py
> @@ -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)

OK, makes sense -- assuming PIL is as common as ImageMagick, this
should be fine.

(I personally prefer to conform to PEP 8, and the spacing around the
keyword arguments doesn't as far as I know, but that is really a very
minor issue.)

>  
>          # Compare md5sum of barrier region with the expected md5sum
>          calced_md5sum = ppm_utils.get_region_md5sum(w, h, data, x1,
> y1, dx, dy,
> -- 
> 1.6.3.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html