Hi Feng, I've tested your patch, and have some additional comments to make:
On Mon, Feb 22, 2010 at 6:30 AM, Feng Yang wrote:
> Signed-off-by: Feng Yang
> ---
> client/tests/kvm/scripts/check_image.py | 85
> +++
> client/tests/kvm/tests_base.cfg.sample | 4 ++
> 2 files changed, 89 insertions(+), 0 deletions(-)
> create mode 100755 client/tests/kvm/scripts/check_image.py
>
> diff --git a/client/tests/kvm/scripts/check_image.py
> b/client/tests/kvm/scripts/check_image.py
> new file mode 100755
> index 000..97f5136
> --- /dev/null
> +++ b/client/tests/kvm/scripts/check_image.py
> @@ -0,0 +1,85 @@
> +import os, sys, commands
> +
> +
> +class ImageCheckError(Exception):
> + """
> + Simple wrapper for the builtin Exception class.
> + """
> + pass
> +
> +
> +class ImageCheck(object):
> + """
> + Check qcow2 image by qemu-img info/check command.
> + """
> + def __init__(self):
> + """
> + Gets params from environment variables and sets class attributes.
> + """
> + self.image_path_list = []
> + client_dir = os.environ['AUTODIR']
> + self.kvm_dir = os.path.join(client_dir, 'tests/kvm')
> + img_to_check = os.environ['KVM_TEST_images'].split()
> +
> + for img in img_to_check:
> + img_name_str = "KVM_TEST_image_name_%s" % img
> + if not os.environ.has_key(img_name_str):
> + img_name_str = "KVM_TEST_image_name"
> + img_format_str = "KVM_TEST_image_format_%s" % img
> + if os.environ.has_key(img_format_str):
> + image_format = os.environ[img_format_str]
> + else:
> + image_format = os.environ['KVM_TEST_image_format']
> + if image_format != "qcow2":
> + continue
> + image_name = os.environ[img_name_str]
> + image_filename = "%s.%s" % (image_name, image_format)
> + image_filename = os.path.join(self.kvm_dir, image_filename)
> + self.image_path_list.append(image_filename)
> + if os.environ.has_key('KVM_TEST_qemu_img_binary'):
> + self.qemu_img_path = os.environ['KVM_TEST_qemu_img_binary']
> + else:
> + self.qemu_img_path = os.path.join(self.kvm_dir, 'qemu-img')
> +
> +
> + def exec_img_cmd(self, cmd_type, image_path):
> + """
> + Run qemu-img info/check on given image.
> +
> + �...@param cmd_type: Sub command used together with qemu.
> + �...@param image_path: Real path of the image.
> + """
> + cmd = ' '.join([self.qemu_img_path, cmd_type, image_path])
> + print "checking with %s" % cmd
> + (status, output) = commands.getstatusoutput(cmd)
Here you could print the output of the commands (as opposed to only
printing it if things fail), it's not a lot of output and it's nice to
see what the programs are printing to the user.
> + if status or (cmd_type == "check" and not "No errors" in output):
> + msg = "Command %s failed" % cmd
> + print output
> + return False, msg
> + else:
> + return True, ''
> +
> +
> + def check_image(self):
> + """
> + Run qemu-img info/check to check the image in list.
> +
> + If the image checking is failed, raise an exception.
> + """
> + # Check all the image in list.
> + errmsg = []
> + for image_path in self.image_path_list:
> + s, o = self.exec_img_cmd('info', image_path)
> + if not s:
> + errmsg.append(o)
> + s, o = self.exec_img_cmd('check', image_path)
We have one problem: qemu-img check is not available, let's say, on
RHEL 5.X (the feature was added at a later point). So I suggest a
qemu-img capability check first, while initializing the checker class,
and simply finish the script cleanly if we have no 'check' command
implemented (with the appropriate messages put on the logs).
> + if not s:
> + errmsg.append(o)
> +
> + if len(errmsg) > 0:
> + raise ImageCheckError('Errors are found and please check log!')
> +
> +
> +if __name__ == "__main__":
> + image_check = ImageCheck()
> + image_check.check_image()
> diff --git a/client/tests/kvm/tests_base.cfg.sample
> b/client/tests/kvm/tests_base.cfg.sample
> index e9fdd05..159c1be 100644
> --- a/client/tests/kvm/tests_base.cfg.sample
> +++ b/client/tests/kvm/tests_base.cfg.sample
> @@ -1019,6 +1019,10 @@ virtio|virtio_blk|e1000:
> variants:
> - @qcow2:
> image_format = qcow2
> + post_command = python scripts/check_image.py
It is a good practice to append commands to post_command, not defining
them exclusively, because in the fututre we might have more than one
post command being run at a single test. In practice, it looks like
this:
post_command = " python scripts/check_image.py;"
> + remove_image = no
> + post_command_t
