Thanks Lucas for reviewing the patch. I have few comments inline.
On Thu, 2012-06-21 at 00:16 -0300, Lucas Meneghel Rodrigues wrote: > On Wed, Jun 20, 2012 at 3:06 PM, Satheesh Rajendran > <sathn...@linux.vnet.ibm.com> wrote: > > Hi Lucas, > > > > Please find the below patch to test the block stream test case. > > This patch addresses the below item: > > https://github.com/autotest/autotest/issues/403 > > Thanks Satheesh, I like the overall quality of the test, some comments below: > > > > > > > Signed-off-by: Satheesh Rajendran <sathn...@linux.vnet.ibm.com> > > > > create mode 100644 client/tests/kvm/tests/block_stream.py > > > > diff --git a/client/tests/kvm/tests/block_stream.py > > b/client/tests/kvm/tests/block_stream.py > > new file mode 100644 > > index 0000000..0150c12 > > --- /dev/null > > +++ b/client/tests/kvm/tests/block_stream.py > > @@ -0,0 +1,148 @@ > > +import re, os, logging, commands, string, time > > +from autotest.client.shared import utils, error > > +from autotest.client.virt import kvm_monitor, virt_utils, virt_vm, > > aexpect, virt_env_process > > You can set up this function to use the decorator, to use error.context below: > @error.context_aware > > +def run_block_stream(test, params, env): > > + """ > > + Test block streaming functionality. > > + > > + 1) Create a image_bak.img with the backing file image.img > > + 2) Start the image_bak.img in qemu command line. > > + 3) Request for block-stream ide0-hd0/virtio0 > > + 4) Wait till the block job finishs > > + 5) Check for backing file in image_bak.img > > + 6) TODO: Check for the size of the image_bak.img should not exceeds > > the image.img > > + 7) TODO(extra): Block job completion can be check in QMP > > + """ > > + image_format = params.get("image_format") > > + image_name = params.get("image_name", "image") > > + drive_format = params.get("drive_format") > > It'd be nice to get the values of qemu-img through > params.get("qemu_img_binary"), since this way we'd be using the exact > qemu-img command this job is using. > Will do it. > > + backing_file_name = "%s_bak" % (image_name) > > + > > + def check_block_jobs_info(): > > + """ > > + Verify the status of block-jobs reported by monitor command info > > block-jobs. > > + @return: parsed output of info block-jobs > > + """ > > + fail = 0 > > + > > + try: > > + output = vm.monitor.info("block-jobs") > > + except kvm_monitor.MonitorError, e: > > + logging.error(e) > > + fail += 1 > > + return None, None > > + return (re.match("\w+", str(output)), re.findall("\d+", > > str(output))) > > + > > + try: > > + # Remove the existing backing file > > + backing_file = "%s.%s" % (backing_file_name, image_format) > > + if os.path.isfile(backing_file): > > + os.remove(backing_file) > > + > > + # Create the new backing file > > + create_cmd = "qemu-img create -b %s.%s -f %s %s.%s" % (image_name, > > + > > image_format, > > + > > image_format, > > + > > backing_file_name, > > + > > image_format) > > + try: > > + utils.system(create_cmd) > > + except error.CmdError, e: > > + raise error.TestFail("Could not create a backing file!") > > ^ Here you can just set error.context() (see in the other tests code > examples of use), and then just execute utils.system(create_cmd). > Well, it'd be something like: > > error.context("Creating a backing file", logging.INFO) > utils.system(create_cmd) > Will do it. > > + logging.info("backing_file created!") > > + > > + info_cmd = "qemu-img info %s.%s" % (image_name,image_format) > > + try: > > + results = utils.system_output(info_cmd) > > + except error.CmdError, e: > > + raise error.TestFail("Could not find the image file!") > > Same here, use error.context(), so you don't need a try/except block. > Will do it. > > + logging.info("Infocmd output of basefile: %s" ,results) > > + > > + # Set the qemu harddisk to the backing file > > + logging.info("Original image_name is: %s", > > params.get('image_name')) > > + params['image_name'] = backing_file_name > > + logging.info("Param image_name changed to: %s", > > + params.get('image_name')) > > + > > + # Start virtual machine, using backing file as its harddisk > > + vm_name = params.get('main_vm') > > + virt_env_process.preprocess_vm(test, params, env, vm_name) > > + vm = env.get_vm(vm_name) > > + vm.create() > > + timeout = int(params.get("login_timeout", 360)) > > + session = vm.wait_for_login(timeout=timeout) > > + > > + info_cmd = "qemu-img info %s.%s" % (backing_file_name,image_format) > > + try: > > + results = utils.system_output(info_cmd) > > + except error.CmdError, e: > > + raise error.TestFail("Could not find the image file!") > > ^ Same here > Will do it. > > + > > + logging.info("Infocmd output of backing file before block > > streaming: %s" ,results) > > + > > + if not re.search("backing file:", str(results)): > > + raise error.TestFail(" Backing file is not available in the > > backdrive image") > > + > > + ##### > > + #start streaming in qemu-cmd line > > + ##### > > + if 'ide' in drive_format: > > + try: > > + vm.monitor.cmd("block-stream ide0-hd0") > > + except kvm_monitor.MonitorError, e: > > + logging.error(e) > Is it safe just log an error here? If this fails, I assume the whole > test is ruined, isn't it? > Yes, you are correct, but if there is a error in this command continuing test becomes useless, I should have come out with a test fail, I would introduce the below line. raise error.TestFail(e) > > + elif 'virtio' in drive_format: > > + try: > > + vm.monitor.cmd("block-stream virtio0") > > Same here > Same as above. > > + except kvm_monitor.MonitorError, e: > > + logging.error(e) > > + else: > > + raise error.TestFail("The drive format is not supported") > > + > > + while True: > > + blkjobout, blkjobstatus = check_block_jobs_info() > > + if 'Streaming' in blkjobout.group(0): > > + logging.info("[(Completed bytes): %s (Total bytes): %s > > (Speed in bytes/s): %s]", blkjobstatus[-3], blkjobstatus[-2], > > blkjobstatus[-1]) > > + time.sleep(10) > > + continue > > + if 'No' in blkjobout.group(0): > > + logging.info("Block job completed") > > + break > > + > > + info_cmd = "qemu-img info %s.%s" % (backing_file_name,image_format) > > + try: > > + results = utils.system_output(info_cmd) > > + except error.CmdError, e: > > + raise error.TestFail("Could not find the image file!") > > Again, you can use context and avoid the exception handling. > Will do it. > > + logging.info("Infocmd output of backing file after block > > streaming: %s" ,results) > > + > > + if re.search("backing file:", str(results)): > > + raise error.TestFail(" Backing file is still available in the > > backdrive image") > > + # > > + #TODO > > + #. The file size should be more/less equal to the "baking file" > > size > > + # > > + > > + #shutdown the virtual machine > > + vm.destroy() > > + > > + # relogin with the backup-harddrive > > + vm.create() > > + timeout = int(params.get("login_timeout", 360)) > > + session = vm.wait_for_login(timeout=timeout) > > + try: > > + output = session.cmd("touch /testfile") > > + logging.info("Output of touch /testfile: %s", output) > > + except error.CmdError, e: > > 2 things: > * Again, you can avoid the exception handling > * The exception you *should* catch here is aexpect.ShellError > (pointing this out for the sake of information). > Thanks for the info, will do it. This command adds a restriction to execute for windows guest. I would add a check for running on windows guest too. > > + raise error.TestFail("Could not create a file in the > > backup-harddrive!") > > + > > + #Finally shutdown the virtual machine > > + vm.destroy() > > + logging.info("END") > > + finally: > > + # Remove the backing file > > + if os.path.isfile(backing_file): > > + os.remove(backing_file) > > diff --git a/client/virt/subtests.cfg.sample > > b/client/virt/subtests.cfg.sample > > index ebfbff5..7c1c715 100644 > > --- a/client/virt/subtests.cfg.sample > > +++ b/client/virt/subtests.cfg.sample > > @@ -783,6 +783,9 @@ variants: > > type = stop_continue > > kill_vm_on_error = yes > > > > + - block_stream: > > + type = block_stream > > + > > - nfs_corrupt: > > only Linux > > type = nfs_corrupt > > -- > > 1.7.5.4 > > I am testing the patch with the modification and will send it shortly. Regards, -Satheesh.
_______________________________________________ Autotest mailing list Autotest@test.kernel.org http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
