From mboxrd@z Thu Jan  1 00:00:00 1970
From: Yolkfull Chow <yzhou@redhat.com>
Subject: Re: [Autotest] [Autotest PATCH] KVM-test: Add a subtest 'qemu_img'
Date: Thu, 28 Jan 2010 17:49:54 +0800
Message-ID: <20100128094954.GC2356@aFu.nay.redhat.com>
References: <2023444877.247901264595238261.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com>
 <635106311.248441264595866589.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com>
 <20100128030753.GA2356@aFu.nay.redhat.com>
 <a50cf5ab1001271931y320fb105v295e4fc65c537761@mail.gmail.com>
Reply-To: Yolkfull Chow <yzhou@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: QUOTED-PRINTABLE
Cc: Michael Goldish <mgoldish@redhat.com>, autotest@test.kernel.org,
	kvm@vger.kernel.org
To: sudhir kumar <smalikphy@gmail.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:44612 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1754950Ab0A1JuB (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 28 Jan 2010 04:50:01 -0500
Content-Disposition: inline
In-Reply-To: <a50cf5ab1001271931y320fb105v295e4fc65c537761@mail.gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Thu, Jan 28, 2010 at 09:01:43AM +0530, sudhir kumar wrote:
> Yolkfull,
> Good test. Did never come to my mind to add such a test to autotest.
> I would like to test your latest patch!!

Thanks Sudhir, please then help review that patch as well.:)

>=20
> On Thu, Jan 28, 2010 at 8:37 AM, Yolkfull Chow <yzhou@redhat.com> wro=
te:
> > On Wed, Jan 27, 2010 at 07:37:46AM -0500, Michael Goldish wrote:
> >>
> >> ----- "Yolkfull Chow" <yzhou@redhat.com> wrote:
> >>
> >> > On Tue, Jan 26, 2010 at 03:11:34PM -0200, Lucas Meneghel Rodrigu=
es
> >> > wrote:
> >> > > On Tue, 2010-01-26 at 11:25 +0800, Yolkfull Chow wrote:
> >> > > > This is designed to test all subcommands of 'qemu-img' howev=
er
> >> > > > so far 'commit' is not implemented.
> >> > >
> >> > > Hi Yolkful, this is very good! Seeing this test made me think =
about
> >> > that
> >> > > stand alone autotest module we commited a while ago, that does
> >> > > qemu_iotests testsuite on the host.
> >> > >
> >> > > Perhaps we could 'port' this module to the kvm test, since it =
is
> >> > more
> >> >
> >> > Lucas, do you mean the client-side 'kvmtest' ?
> >> >
> >> > And thanks for comments. :)
> >> >
> >> > > convenient to execute it inside a kvm test job (in a job where=
 we
> >> > test
> >> > > more than 2 build types, for example, we need to execute qemu_=
img
> >> > and
> >> > > qemu_io_tests for every qemu-img built).
> >> > >
> >> > > Could you look at implementing this?
> >> > >
> >> > > > * For 'check' subcommand test, it will 'dd' to create a file=
 with
> >> > specified
> >> > > > size and see whether it's supported to be checked. Then conv=
ert it
> >> > to be
> >> > > > supported formats (qcow2 and raw so far) to see whether ther=
e's
> >> > error
> >> > > > after convertion.
> >> > > >
> >> > > > * For 'convert' subcommand test, it will convert both to 'qc=
ow2'
> >> > and 'raw' from
> >> > > > the format specified in config file. And only check 'qcow2' =
after
> >> > convertion.
> >> > > >
> >> > > > * For 'snapshot' subcommand test, it will create two snapsho=
ts and
> >> > list them.
> >> > > > Finally delete them if no errors found.
> >> > > >
> >> > > > * For 'info' subcommand test, it simply get output from spec=
ified
> >> > image file.
> >> > > >
> >> > > > Signed-off-by: Yolkfull Chow <yzhou@redhat.com>
> >> > > > ---
> >> > > > =A0client/tests/kvm/tests/qemu_img.py =A0 =A0 | =A0155
> >> > ++++++++++++++++++++++++++++++++
> >> > > > =A0client/tests/kvm/tests_base.cfg.sample | =A0 36 ++++++++
> >> > > > =A02 files changed, 191 insertions(+), 0 deletions(-)
> >> > > > =A0create mode 100644 client/tests/kvm/tests/qemu_img.py
> >> > > >
> >> > > > diff --git a/client/tests/kvm/tests/qemu_img.py
> >> > b/client/tests/kvm/tests/qemu_img.py
> >> > > > new file mode 100644
> >> > > > index 0000000..1ae04f0
> >> > > > --- /dev/null
> >> > > > +++ b/client/tests/kvm/tests/qemu_img.py
> >> > > > @@ -0,0 +1,155 @@
> >> > > > +import os, logging, commands
> >> > > > +from autotest_lib.client.common_lib import error
> >> > > > +import kvm_vm
> >> > > > +
> >> > > > +
> >> > > > +def run_qemu_img(test, params, env):
> >> > > > + =A0 =A0"""
> >> > > > + =A0 =A0`qemu-img' functions test:
> >> > > > + =A0 =A01) Judge what subcommand is going to be tested
> >> > > > + =A0 =A02) Run subcommand test
> >> > > > +
> >> > > > + =A0 =A0@param test: kvm test object
> >> > > > + =A0 =A0@param params: Dictionary with the test parameters
> >> > > > + =A0 =A0@param env: Dictionary with test environment.
> >> > > > + =A0 =A0"""
> >> > > > + =A0 =A0cmd =3D params.get("qemu_img_binary")
> >> > >
> >> > > It is a good idea to verify if cmd above resolves to an absolu=
te
> >> > path,
> >> > > to avoid problems. If it doesn't resolve, verify if there's th=
e
> >> > symbolic
> >> > > link under kvm test dir pointing to qemu-img, and if it does e=
xist,
> >> > make
> >> > > sure it points to a valid file (ie, symlink is not broken).
> >>
> >> This can be done quickly using kvm_utils.get_path() and os.path.ex=
ists(),
> >> like this:
> >>
> >> cmd =3D kvm_utils.get_path(params.get("qemu_img_binary"))
> >> if not os.path.exists(cmd):
> >> =A0 =A0 raise error.TestError("qemu-img binary not found")
> >>
> >> kvm_utils.get_path() is the standard way of getting both absolute =
and
> >> relative paths, and os.path.exists() checks whether the file exist=
s and
> >> makes sure it's not a broken symlink.
> >
> > Yes, thanks for pointing that out.
> >
> >>
> >> > > > + =A0 =A0subcommand =3D params.get("subcommand")
> >> > > > + =A0 =A0image_format =3D params.get("image_format")
> >> > > > + =A0 =A0image_name =3D kvm_vm.get_image_filename(params, te=
st.bindir)
> >> > > > +
> >> > > > + =A0 =A0def check(img):
> >> > > > + =A0 =A0 =A0 =A0global cmd
> >> > > > + =A0 =A0 =A0 =A0cmd +=3D " check %s" % img
> >> > > > + =A0 =A0 =A0 =A0logging.info("Checking image '%s'..." % img=
)
> >> > > > + =A0 =A0 =A0 =A0s, o =3D commands.getstatusoutput(cmd)
> >> > > > + =A0 =A0 =A0 =A0if not (s =3D=3D 0 or "does not support che=
cks" in o):
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0return (False, o)
> >> > > > + =A0 =A0 =A0 =A0return (True, "")
> >> > >
> >> > > Please use utils.system_output here instead of the equivalent
> >> > commands
> >> > > API on the above code. This comment applies to all further use=
s of
> >> > > commands.[function].
> >> > >
> >> > > > +
> >> > > > + =A0 =A0# Subcommand 'qemu-img check' test
> >> > > > + =A0 =A0# This tests will 'dd' to create a specified size f=
ile, and
> >> > check it.
> >> > > > + =A0 =A0# Then convert it to supported image_format in each=
 loop and
> >> > check again.
> >> > > > + =A0 =A0def check_test():
> >> > > > + =A0 =A0 =A0 =A0size =3D params.get("dd_image_size")
> >> > > > + =A0 =A0 =A0 =A0test_image =3D params.get("dd_image_name")
> >> > > > + =A0 =A0 =A0 =A0create_image_cmd =3D params.get("create_ima=
ge_cmd")
> >> > > > + =A0 =A0 =A0 =A0create_image_cmd =3D create_image_cmd % (te=
st_image, size)
> >> > > > + =A0 =A0 =A0 =A0s, o =3D commands.getstatusoutput(create_im=
age_cmd)
> >> > > > + =A0 =A0 =A0 =A0if s !=3D 0:
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0raise error.TestError("Failed comma=
nd: %s; Output is:
> >> > %s" %
> >> > > > +
> >> > (create_image_cmd, o))
> >> > > > + =A0 =A0 =A0 =A0s, o =3D check(test_image)
> >> > > > + =A0 =A0 =A0 =A0if not s:
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0raise error.TestFail("Failed to che=
ck image '%s' with
> >> > error: %s" %
> >> > > > +
> >> > (test_image, o))
> >> > > > + =A0 =A0 =A0 =A0for fmt in
> >> > params.get("supported_image_formats").split():
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0output_image =3D test_image + ".%s"=
 % fmt
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0convert(fmt, test_image, output_ima=
ge)
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0s, o =3D check(output_image)
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0if not s:
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0raise error.TestFail("Check=
 image '%s' got error:
> >> > %s" %
> >> > > > +
> >> > (output_image, o))
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0commands.getoutput("rm -f %s" % out=
put_image)
> >> > > > + =A0 =A0 =A0 =A0commands.getoutput("rm -f %s" % test_image)
> >> > > > + =A0 =A0#Subcommand 'qemu-img create' test
> >> > > > + =A0 =A0def create_test():
> >> > > > + =A0 =A0 =A0 =A0global cmd
> >> > >
> >> > > I don't like very much this idea of using a global variable, i=
nstead
> >> > it
> >> > > should be preferrable to use a class and have a class attribut=
e
> >> > with
> >> > > 'cmd'. This way it would be safer, since the usage of cmd is
> >> > > encapsulated. This comment applies to all further usage of 'gl=
obal
> >> > cmd'.
> >>
> >> Do we really need a class for this? =A0If we want to avoid using a=
 global
> >> variable, 'cmd' can be passed as a parameter to all the inner func=
tions
> >> that use it (check(), convert(), create_test(), etc).
> >
> > Actually before using 'global' to declare it, I was passing 'cmd' a=
s a parameter
> > to all inner functions of subcommand. Anyway, I could change it bac=
k. :)
> >
> >>
> >> > > > + =A0 =A0 =A0 =A0cmd +=3D " create"
> >> > > > + =A0 =A0 =A0 =A0if params.get("encrypted") =3D=3D "yes":
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0cmd +=3D " -e"
> >> > > > + =A0 =A0 =A0 =A0if params.get("base_image"):
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0cmd +=3D " -F %s -b %s" %
> >> > (params.get("base_image_format"),
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 params.get("base_image"))
> >> > > > + =A0 =A0 =A0 =A0format =3D params.get("image_format")
> >> > > > + =A0 =A0 =A0 =A0cmd +=3D " -f %s" % format
> >> > > > + =A0 =A0 =A0 =A0image_name_test =3D os.path.join(test.bindi=
r,
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0params.=
get("image_name_test")) + '.' +
> >> > format
> >> > > > + =A0 =A0 =A0 =A0cmd +=3D " %s %s" % (image_name_test,
> >> > params.get("image_size_test"))
> >> > > > + =A0 =A0 =A0 =A0s, o =3D commands.getstatusoutput(cmd)
> >> > > > + =A0 =A0 =A0 =A0if s !=3D 0:
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0raise error.TestFail("Create image =
'%s' failed: %s"
> >> > %
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 =A0 =A0(image_name_test,
> >> > o))
> >> > > > + =A0 =A0 =A0 =A0commands.getoutput("rm -f %s" % image_name_=
test)
> >> > > > + =A0 =A0def convert(output_format, image_name, output_filen=
ame,
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0format=3DNone, compressed=3D=
"no", encrypted=3D"no"):
> >> > > > + =A0 =A0 =A0 =A0global cmd
> >> > > > + =A0 =A0 =A0 =A0cmd +=3D " convert"
> >> > > > + =A0 =A0 =A0 =A0if compressed =3D=3D "yes":
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0cmd +=3D " -c"
> >> > > > + =A0 =A0 =A0 =A0if encrypted =3D=3D "yes":
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0cmd +=3D " -e"
> >> > > > + =A0 =A0 =A0 =A0if format:
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0cmd +=3D " -f %s" % image_format
> >> > > > + =A0 =A0 =A0 =A0cmd +=3D " -O %s" % params.get("dest_image_=
format")
> >> > > > + =A0 =A0 =A0 =A0cmd +=3D " %s %s" % (image_name, output_fil=
ename)
> >> > > > + =A0 =A0 =A0 =A0s, o =3D commands.getstatusoutput(cmd)
> >> > > > + =A0 =A0 =A0 =A0if s !=3D 0:
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0raise error.TestFail("Image convert=
ed failed;
> >> > Command: %s;"
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 "Output is: %s" % (cmd, o))
> >> > > > + =A0 =A0#Subcommand 'qemu-img convert' test
> >> > > > + =A0 =A0def convert_test():
> >> > > > + =A0 =A0 =A0 =A0dest_image_format =3D params.get("dest_imag=
e_format")
> >> > > > + =A0 =A0 =A0 =A0output_filename =3D "%s.converted_%s" % (im=
age_name,
> >> > dest_image_format)
> >> > > > +
> >> > > > + =A0 =A0 =A0 =A0convert(dest_image_format, image_name,
> >> > params.get("dest_image_format"),
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0image_format, params.get("c=
ompressed"),
> >> > params.get("encrypted"))
> >> > > > +
> >> > > > + =A0 =A0 =A0 =A0if dest_image_format =3D=3D "qcow2":
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0s, o =3D check(output_filename)
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0if s:
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0commands.getoutput("rm -f %=
s" % output_filename)
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0else:
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0raise error.TestFail("Check=
 image '%s' failed
> >> > with error: %s" %
> >> > > > +
> >> > (dest_image_format, o))
> >> > > > + =A0 =A0 =A0 =A0else:
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0commands.getoutput("rm -f %s" % out=
put_filename)
> >> > > > + =A0 =A0#Subcommand 'qemu-img info' test
> >> > > > + =A0 =A0def info_test():
> >> > > > + =A0 =A0 =A0 =A0global cmd
> >> > > > + =A0 =A0 =A0 =A0cmd +=3D " info"
> >> > > > + =A0 =A0 =A0 =A0cmd +=3D " -f %s %s" % (image_format, image=
_name)
> >> > > > + =A0 =A0 =A0 =A0s, o =3D commands.getstatusoutput(cmd)
> >> > > > + =A0 =A0 =A0 =A0if s !=3D 0:
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0raise error.TestFail("Get info of i=
mage '%s' failed:
> >> > %s" %
> >> > > > +
> >> > (image_name, o))
> >> > > > + =A0 =A0 =A0 =A0logging.info("Info of image '%s': \n%s" % (=
image_name,
> >> > o))
> >> > >
> >> > > In the above, we could parse info output and see if at least i=
t
> >> > says
> >> > > that the image is the type we expected it to be.
> >> > >
> >> > > > + =A0 =A0#Subcommand 'qemu-img snapshot' test
> >> > > > + =A0 =A0def snapshot_test():
> >> > > > + =A0 =A0 =A0 =A0global cmd
> >> > > > + =A0 =A0 =A0 =A0cmd +=3D " snapshot"
> >> > > > + =A0 =A0 =A0 =A0for i in range(2):
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0crtcmd =3D cmd
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0snapshot_name =3D "snapshot%d" % i
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0crtcmd +=3D " -c %s %s" % (snapshot=
_name, image_name)
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0s, o =3D commands.getstatusoutput(c=
rtcmd)
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0if s !=3D 0:
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0raise error.TestFail("Creat=
e snapshot failed via
> >> > command: %s;"
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 "Output is: %s" % (crtcmd,
> >> > o))
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0logging.info("Created snapshot#%d" =
% i)
> >> > > > + =A0 =A0 =A0 =A0listcmd =3D cmd
> >> > > > + =A0 =A0 =A0 =A0listcmd +=3D " -l %s" % image_name
> >> > > > + =A0 =A0 =A0 =A0s, o =3D commands.getstatusoutput(listcmd)
> >> > > > + =A0 =A0 =A0 =A0if not ("snapshot0" in o and "snapshot1" in=
 o and s =3D=3D
> >> > 0):
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0raise error.TestFail("Snapshot crea=
ted failed or
> >> > missed;"
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 "snapshot list is: \n%s" % o)
> >> > > > + =A0 =A0 =A0 =A0for i in range(2):
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0snapshot_name =3D "snapshot%d" % i
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0delcmd =3D cmd
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0delcmd +=3D " -d %s %s" % (snapshot=
_name, image_name)
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0s, o =3D commands.getstatusoutput(d=
elcmd)
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0if s !=3D 0:
> >> > > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0raise error.TestFail("Delet=
e snapshot '%s'
> >> > failed: %s" %
> >> > > > +
> >> > (snapshot_name, o))
> >> > > > +
> >> > > > + =A0 =A0#Subcommand 'qemu-img commit' test
> >> > > > + =A0 =A0def commit_test(cmd):
> >> > > > + =A0 =A0 =A0 =A0pass
> >> > > > +
> >> > > > + =A0 =A0# Here starts test
> >> > > > + =A0 =A0eval("%s_test" % subcommand)
> >>
> >> Aren't you missing a () -- eval("%s_test()" % subcommand)?
> >>
> >> BTW, Yolkfull, have you tried running the test and verifying that =
it
> >> works?
> >
> > Oh, really missed it. I tested it when using method of passing 'cmd=
' as a parameter
> > to subcommand functions and it worked fine. So introduced this mist=
ake when changing
> > to use 'global'. Will fix it in next version which will also add 'r=
ebase' subcommand test.
> >
> > Thanks for comments, Michael.
> >
> >>
> >> > >
> >> > > In the above expression, we would also benefit from encapsulat=
ing
> >> > all
> >> > > qemu-img tests on a class:
> >> > >
> >> > > =A0 =A0 =A0 tester =3D QemuImgTester()
> >> > > =A0 =A0 =A0 tester.test(subcommand)
> >> > >
> >> > > Or something similar.
> >> > >
> >> > > That said, I was wondering if we could consolidate all qemu-im=
g
> >> > tests to
> >> > > a single execution, instead of splitting it to several variant=
s. We
> >> > > could keep a failure record, execute all tests and fail the en=
tire
> >> > test
> >> > > if any of them failed. It's not like terribly important, but i=
t
> >> > seems
> >> > > more logical to group all qemu-img subcommands testing under a
> >> > single
> >> > > test.
> >> > >
> >> > > Thanks for your work, and please take a look at implementing m=
y
> >> > > suggestions.
> >> >
> >> > > --
> >> > > 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 =A0http://vger.kernel.org/majordomo-inf=
o.html
> >> > --
> >> > To unsubscribe from this list: send the line "unsubscribe kvm" i=
n
> >> > the body of a message to majordomo@vger.kernel.org
> >> > More majordomo info at =A0http://vger.kernel.org/majordomo-info.=
html
> > _______________________________________________
> > Autotest mailing list
> > Autotest@test.kernel.org
> > http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
> >
>=20
>=20
>=20
> --=20
> Sudhir Kumar