Re: [PATCH] KVM Test: Add check_image script as post_command script of qcow2 variant.

[Feng Yang <fyang@redhat.com>]

Hi Lucas

Thanks for your comments! I am sorry for replying your email so late!
A new patch will be sent out according to your comments.

Best Regards

Feng Yang

----- "Lucas Meneghel Rodrigues" <lmr@redhat.com> wrote:

> From: "Lucas Meneghel Rodrigues" <lmr@redhat.com>
> To: "Feng Yang" <fyang@redhat.com>
> Cc: autotest@test.kernel.org, kvm@vger.kernel.org
> Sent: Thursday, February 11, 2010 12:23:51 AM GMT +08:00 Beijing / Chongqing / Hong Kong / Urumqi
> Subject: Re: [Autotest] [PATCH] KVM Test: Add check_image script as post_command script of qcow2 variant.
>
> On Fri, 2010-01-29 at 18:17 +0800, Feng Yang wrote: 
> > Add check_image.py in scripts folder.  It will performance qemu-img
> info
> > and qemu-img check.
> > Add check_image.py as post command of qcow2 variant.
> 
> Hi Feng, thanks for your patch! See some comments below:
> 
> > Signed-off-by: Feng Yang <fyang@redhat.com>
> > ---
> >  client/tests/kvm/scripts/check_image.py |   89
> +++++++++++++++++++++++++++++++
> >  client/tests/kvm/tests_base.cfg.sample  |    6 ++
> >  2 files changed, 95 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 0000000..a37f055
> > --- /dev/null
> > +++ b/client/tests/kvm/scripts/check_image.py
> > @@ -0,0 +1,89 @@
> > +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')
> > +        if os.environ.has_key('KVM_TEST_img_to_check'):
> > +            img_to_check =
> os.environ['KVM_TEST_img_to_check'].split()
> > +        else:
> > +            img_to_check = os.environ['KVM_TEST_images'].split()
> 
> On the above attribution, I didn't understand why we would 'hardcode'
> a
> particular image to be checked, it is my understanding that we want
> to
> check systematically all the images after each test.
> 
> > +        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)
> > +
> > +
> > +    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.
> > +        """
> > +        if os.environ.has_key('KVM_TEST_qemu_img_binary'):
> > +            qemu_img_path = os.environ['KVM_TEST_qemu_img_binary']
> > +        else:
> > +            qemu_img_path = os.path.join(self.kvm_dir, 'qemu-img')
> 
> The above can be put on the class constructor, make qemu_img_cmd a
> class
> attribute.
> 
> > +        cmd = ' '.join([qemu_img_path, cmd_type, image_path])
> > +        print "checking with %s" % cmd
> > +        (status, output) = commands.getstatusoutput(cmd)
> > +        if status or ( cmd_type == "check" and not "No errors" in
> output ):
> 
> You can remove the spaces around the expression you want to evaluate.
> 
> > +            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)
> > +            if not s:
> > +                errmsg.append(o)
> > +
> > +        if len(errmsg) > 0:
> > +            raise ImageCheckError('Errors are found and please
> check log '\
> > +                                  'for details!')
> 
> You don't need the \ to break line in the above statement, you can
> just
> remove it.
> 
> > +
> > +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 2d03f1f..33f9586 100644
> > --- a/client/tests/kvm/tests_base.cfg.sample
> > +++ b/client/tests/kvm/tests_base.cfg.sample
> > @@ -938,6 +938,12 @@ virtio|virtio_blk|e1000:
> >  variants:
> >      - @qcow2:
> >          image_format = qcow2
> > +        kill_vm = yes
> 
> Why the use of the preprocessor directive 'kill_vm' here? qemu-img
> check
> doesn't work with the VM turned on?
> 
> > +        post_command = python scripts/check_image.py
> > +        remove_image = no
> > +        # img_to_check =
> > +        post_command_timeout = 600
> > +
> >      - vmdk:
> >          only Fedora Ubuntu Windows
> >          only smp2