Yolkfull,
Good test. Did never come to my mind to add such a test to autotest.
I would like to test your latest patch!!
On Thu, Jan 28, 2010 at 8:37 AM, Yolkfull Chow <yzhou@xxxxxxxxxx> wrote:
> 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
> _______________________________________________
> Autotest mailing list
> Autotest@xxxxxxxxxxxxxxx
> http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
>
--
Sudhir Kumar
--
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
