On Wed, Jan 27, 2010 at 07:37:46AM -0500, Michael Goldish wrote:
>
> ----- "Yolkfull Chow" <yzhou@xxxxxxxxxx> wrote:
>
> > On Tue, Jan 26, 2010 at 03:11:34PM -0200, Lucas Meneghel Rodrigues
> > wrote:
> > > On Tue, 2010-01-26 at 11:25 +0800, Yolkfull Chow wrote:
> > > > This is designed to test all subcommands of 'qemu-img' however
> > > > 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 convert it
> > to be
> > > > supported formats (qcow2 and raw so far) to see whether there's
> > error
> > > > after convertion.
> > > >
> > > > * For 'convert' subcommand test, it will convert both to 'qcow2'
> > and 'raw' from
> > > > the format specified in config file. And only check 'qcow2' after
> > convertion.
> > > >
> > > > * For 'snapshot' subcommand test, it will create two snapshots and
> > list them.
> > > > Finally delete them if no errors found.
> > > >
> > > > * For 'info' subcommand test, it simply get output from specified
> > image file.
> > > >
> > > > Signed-off-by: Yolkfull Chow <yzhou@xxxxxxxxxx>
> > > > ---
> > > > client/tests/kvm/tests/qemu_img.py | 155
> > ++++++++++++++++++++++++++++++++
> > > > client/tests/kvm/tests_base.cfg.sample | 36 ++++++++
> > > > 2 files changed, 191 insertions(+), 0 deletions(-)
> > > > create 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):
> > > > + """
> > > > + `qemu-img' functions test:
> > > > + 1) Judge what subcommand is going to be tested
> > > > + 2) Run subcommand test
> > > > +
> > > > + @param test: kvm test object
> > > > + @param params: Dictionary with the test parameters
> > > > + @param env: Dictionary with test environment.
> > > > + """
> > > > + cmd = params.get("qemu_img_binary")
> > >
> > > It is a good idea to verify if cmd above resolves to an absolute
> > path,
> > > to avoid problems. If it doesn't resolve, verify if there's the
> > symbolic
> > > link under kvm test dir pointing to qemu-img, and if it does exist,
> > 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.exists(),
> like this:
>
> cmd = kvm_utils.get_path(params.get("qemu_img_binary"))
> if not os.path.exists(cmd):
> 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 exists and
> makes sure it's not a broken symlink.
Yes, thanks for pointing that out.
>
> > > > + subcommand = params.get("subcommand")
> > > > + image_format = params.get("image_format")
> > > > + image_name = kvm_vm.get_image_filename(params, test.bindir)
> > > > +
> > > > + def check(img):
> > > > + global cmd
> > > > + cmd += " check %s" % img
> > > > + logging.info("Checking image '%s'..." % img)
> > > > + s, o = commands.getstatusoutput(cmd)
> > > > + if not (s == 0 or "does not support checks" in o):
> > > > + return (False, o)
> > > > + return (True, "")
> > >
> > > Please use utils.system_output here instead of the equivalent
> > commands
> > > API on the above code. This comment applies to all further uses of
> > > commands.[function].
> > >
> > > > +
> > > > + # Subcommand 'qemu-img check' test
> > > > + # This tests will 'dd' to create a specified size file, and
> > check it.
> > > > + # Then convert it to supported image_format in each loop and
> > check again.
> > > > + def check_test():
> > > > + size = params.get("dd_image_size")
> > > > + test_image = params.get("dd_image_name")
> > > > + create_image_cmd = params.get("create_image_cmd")
> > > > + create_image_cmd = create_image_cmd % (test_image, size)
> > > > + s, o = commands.getstatusoutput(create_image_cmd)
> > > > + if s != 0:
> > > > + raise error.TestError("Failed command: %s; Output is:
> > %s" %
> > > > +
> > (create_image_cmd, o))
> > > > + s, o = check(test_image)
> > > > + if not s:
> > > > + raise error.TestFail("Failed to check image '%s' with
> > error: %s" %
> > > > +
> > (test_image, o))
> > > > + for fmt in
> > params.get("supported_image_formats").split():
> > > > + output_image = test_image + ".%s" % fmt
> > > > + convert(fmt, test_image, output_image)
> > > > + s, o = check(output_image)
> > > > + if not s:
> > > > + raise error.TestFail("Check image '%s' got error:
> > %s" %
> > > > +
> > (output_image, o))
> > > > + commands.getoutput("rm -f %s" % output_image)
> > > > + commands.getoutput("rm -f %s" % test_image)
> > > > + #Subcommand 'qemu-img create' test
> > > > + def create_test():
> > > > + global cmd
> > >
> > > I don't like very much this idea of using a global variable, instead
> > it
> > > should be preferrable to use a class and have a class attribute
> > with
> > > 'cmd'. This way it would be safer, since the usage of cmd is
> > > encapsulated. This comment applies to all further usage of 'global
> > cmd'.
>
> Do we really need a class for this? If we want to avoid using a global
> variable, 'cmd' can be passed as a parameter to all the inner functions
> that use it (check(), convert(), create_test(), etc).
Actually before using 'global' to declare it, I was passing 'cmd' as a parameter
to all inner functions of subcommand. Anyway, I could change it back. :)
>
> > > > + cmd += " create"
> > > > + if params.get("encrypted") == "yes":
> > > > + cmd += " -e"
> > > > + if params.get("base_image"):
> > > > + cmd += " -F %s -b %s" %
> > (params.get("base_image_format"),
> > > > + params.get("base_image"))
> > > > + format = params.get("image_format")
> > > > + cmd += " -f %s" % format
> > > > + image_name_test = os.path.join(test.bindir,
> > > > + params.get("image_name_test")) + '.' +
> > format
> > > > + cmd += " %s %s" % (image_name_test,
> > params.get("image_size_test"))
> > > > + s, o = commands.getstatusoutput(cmd)
> > > > + if s != 0:
> > > > + raise error.TestFail("Create image '%s' failed: %s"
> > %
> > > > + (image_name_test,
> > o))
> > > > + commands.getoutput("rm -f %s" % image_name_test)
> > > > + def convert(output_format, image_name, output_filename,
> > > > + format=None, compressed="no", encrypted="no"):
> > > > + global cmd
> > > > + cmd += " convert"
> > > > + if compressed == "yes":
> > > > + cmd += " -c"
> > > > + if encrypted == "yes":
> > > > + cmd += " -e"
> > > > + if format:
> > > > + cmd += " -f %s" % image_format
> > > > + cmd += " -O %s" % params.get("dest_image_format")
> > > > + cmd += " %s %s" % (image_name, output_filename)
> > > > + s, o = commands.getstatusoutput(cmd)
> > > > + if s != 0:
> > > > + raise error.TestFail("Image converted failed;
> > Command: %s;"
> > > > + "Output is: %s" % (cmd, o))
> > > > + #Subcommand 'qemu-img convert' test
> > > > + def convert_test():
> > > > + dest_image_format = params.get("dest_image_format")
> > > > + output_filename = "%s.converted_%s" % (image_name,
> > dest_image_format)
> > > > +
> > > > + convert(dest_image_format, image_name,
> > params.get("dest_image_format"),
> > > > + image_format, params.get("compressed"),
> > params.get("encrypted"))
> > > > +
> > > > + if dest_image_format == "qcow2":
> > > > + s, o = check(output_filename)
> > > > + if s:
> > > > + commands.getoutput("rm -f %s" % output_filename)
> > > > + else:
> > > > + raise error.TestFail("Check image '%s' failed
> > with error: %s" %
> > > > +
> > (dest_image_format, o))
> > > > + else:
> > > > + commands.getoutput("rm -f %s" % output_filename)
> > > > + #Subcommand 'qemu-img info' test
> > > > + def info_test():
> > > > + global cmd
> > > > + cmd += " info"
> > > > + cmd += " -f %s %s" % (image_format, image_name)
> > > > + s, o = commands.getstatusoutput(cmd)
> > > > + if s != 0:
> > > > + raise error.TestFail("Get info of image '%s' failed:
> > %s" %
> > > > +
> > (image_name, o))
> > > > + logging.info("Info of image '%s': \n%s" % (image_name,
> > o))
> > >
> > > In the above, we could parse info output and see if at least it
> > says
> > > that the image is the type we expected it to be.
> > >
> > > > + #Subcommand 'qemu-img snapshot' test
> > > > + def snapshot_test():
> > > > + global cmd
> > > > + cmd += " snapshot"
> > > > + for i in range(2):
> > > > + crtcmd = cmd
> > > > + snapshot_name = "snapshot%d" % i
> > > > + crtcmd += " -c %s %s" % (snapshot_name, image_name)
> > > > + s, o = commands.getstatusoutput(crtcmd)
> > > > + if s != 0:
> > > > + raise error.TestFail("Create snapshot failed via
> > command: %s;"
> > > > + "Output is: %s" % (crtcmd,
> > o))
> > > > + logging.info("Created snapshot#%d" % i)
> > > > + listcmd = cmd
> > > > + listcmd += " -l %s" % image_name
> > > > + s, o = commands.getstatusoutput(listcmd)
> > > > + if not ("snapshot0" in o and "snapshot1" in o and s ==
> > 0):
> > > > + raise error.TestFail("Snapshot created failed or
> > missed;"
> > > > + "snapshot list is: \n%s" % o)
> > > > + for i in range(2):
> > > > + snapshot_name = "snapshot%d" % i
> > > > + delcmd = cmd
> > > > + delcmd += " -d %s %s" % (snapshot_name, image_name)
> > > > + s, o = commands.getstatusoutput(delcmd)
> > > > + if s != 0:
> > > > + raise error.TestFail("Delete snapshot '%s'
> > failed: %s" %
> > > > +
> > (snapshot_name, o))
> > > > +
> > > > + #Subcommand 'qemu-img commit' test
> > > > + def commit_test(cmd):
> > > > + pass
> > > > +
> > > > + # Here starts test
> > > > + eval("%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 mistake when changing
to use 'global'. Will fix it in next version which will also add 'rebase' subcommand test.
Thanks for comments, Michael.
>
> > >
> > > In the above expression, we would also benefit from encapsulating
> > all
> > > qemu-img tests on a class:
> > >
> > > tester = QemuImgTester()
> > > tester.test(subcommand)
> > >
> > > Or something similar.
> > >
> > > That said, I was wondering if we could consolidate all qemu-img
> > tests to
> > > a single execution, instead of splitting it to several variants. We
> > > could keep a failure record, execute all tests and fail the entire
> > test
> > > if any of them failed. It's not like terribly important, but it
> > 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 my
> > > suggestions.
> >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
