Hi, Lucas

Thanks for your advice , I will  pay attention to principles

On 04/04/13 01:12, Lucas Meneghel Rodrigues wrote:
On 04/01/2013 08:37 AM, [email protected] wrote:
From: whuang <[email protected]>

I found several problems with this test, I'll comment about some of them here, then please look at the corrected version I've pushed to next. So on future patches, pay attention to the principles.

Thanks,

Lucas

Signed-off-by: whuang <[email protected]>
---
libvirt/tests/virsh_change_media.py | 191 ++++++++++++++++++++++++++++++++++++
  1 file changed, 191 insertions(+)
  create mode 100755 libvirt/tests/virsh_change_media.py

diff --git a/libvirt/tests/virsh_change_media.py b/libvirt/tests/virsh_change_media.py
new file mode 100755
index 0000000..93044a9
--- /dev/null
+++ b/libvirt/tests/virsh_change_media.py
@@ -0,0 +1,191 @@
+import logging, os
+from autotest.client.shared import error, utils
+from virttest import libvirt_vm, virsh
+from virttest.libvirt_xml import vm_xml
+
+def run_virsh_change_media(test, params, env):
+    """
+    Test command: virsh change-media.
+
+    The command  Change media of CD or floppy drive.
+    1. Prepare test environment.
+    2. Perform virsh change-media operation.
+    3. Recover test environment.
+    4. Confirm the test result.
+    """
+
+    def env_pre(old_iso, new_iso):
+        """
+        Prepare ISO image for test
+        """
+
+ utils.run("dd if=/dev/urandom of=%s/old bs=10M count=1" % cdrom_dir) + utils.run("dd if=/dev/urandom of=%s/new bs=10M count=1" % cdrom_dir)
+        utils.run("mkisofs -o %s %s/old" % (old_iso, cdrom_dir))
+        utils.run("mkisofs -o %s %s/new" % (new_iso, cdrom_dir))
+
+    def check_media(session, target_file, action):
+        """
+        Check guest cdrom files
+        1. guest session
+        2. the expected files
+        3. test case action
+        """
+
+        if action != "--eject ":
+            #sometimes device is busy so need mount twice
+            cmd = "mount /dev/cdrom /media -oloop"\
+                  "|| mount /dev/cdrom /media -oloop"
+            status, output = session.cmd_status_output(cmd)
+            logging.debug("Mount CDrom :%s %s" % (status, output))

^ CDrom is a very inconsistent term to use. All terms should be cdrom.

+            status, output = session.cmd_status_output("ls /media/%s" \
+                                                       % target_file)
+            if status == 0 :
+                logging.debug("Check %s file then umount CDrom device"\
+                              % output)
+                session.cmd("umount -l /dev/cdrom")
+            else:
+                logging.error("Ls file :%s" % output)
+                session.close()
+                raise error.TestFail("Can not find target file")

Not sure why use loop mode, since for the guest perspective this is a normal cdrom device. You can just use session.cmd for all commands you made here, also, instead of ls, if all you want is to check for the existence of the file, you can just use test -f (shell builtin).

+        else:
+ if session.cmd_status("mount /dev/cdrom /media -oloop") == 32:
+                logging.info("Eject succeed!")
+                return 0
+
+    def add_cdrom_device(vm_name, init_cdrom):
+        """
+        Add cdrom device for test vm
+        """
+
+        if vm.is_alive():
+            virsh.destroy(vm_name)
+
+        virsh.attach_disk(vm_name, init_cdrom, \

^ Unnecessary backslash. Comment valid for all other unnecessary use of backslashes.

+ " hdc", " --type cdrom --sourcetype file --config", debug=True)
+
+    def update_cdrom(vm_name, init_iso, options, start_vm ):
+        """
+        Update cdrom iso file for test case
+        """
+
+        cmd = """cat << EOF > %s
+<disk type='file' device='cdrom'>
+<driver name='qemu' type='raw'/>
+<source file='%s'/>
+<target dev='hdc' bus='ide'/>
+<readonly/>
+</disk>
+EOF""" % (update_iso_xml, init_iso)
+        cmd_options = "--force "
+        if os.system(cmd):
+            logging.error("Create update_iso_xml failed!")

^ Gratuitous use of shell commands when you can just write the string to a file. This is python.

+        if options == "--config" or start_vm == "no":
+            cmd_options += " --config"
+
+        #give domain the ISO image file
+        virsh.update_device(domainarg=vm_name, \
+ filearg=update_iso_xml, flagstr=cmd_options, debug=True)
+
+    vm_name = params.get("main_vm")
+    vm = env.get_vm(vm_name)
+    vm_ref = params.get("vm_ref")
+    action = params.get("action")
+    start_vm = params.get("start_vm")
+    options = params.get("options")
+    cdrom_dir = os.path.join(test.tmpdir, "tmp/")
+    if not os.path.exists(cdrom_dir):
+        os.mkdir(cdrom_dir)
+
+    old_iso_name = params.get("old_iso")
+    new_iso_name = params.get("new_iso")
+    old_iso = cdrom_dir + old_iso_name
+    new_iso = cdrom_dir + new_iso_name
+    init_cdrom = params.get("init_cdrom")
+    update_iso_xml_name = params.get("update_iso_xml")
+    update_iso_xml = cdrom_dir + update_iso_xml_name
+    init_iso_name = params.get("init_iso")
+    if not init_iso_name :
+        init_iso = ""
+
+    else:
+        init_iso = cdrom_dir + init_iso_name
+
+    if vm_ref == "name":
+        vm_ref = vm_name
+
+    env_pre(old_iso, new_iso)
+    #check domain's disk device
+    disk_blk =  vm_xml.VMXML.get_disk_blk(vm_name)
+    logging.info("disk_blk %s" % disk_blk)
+    if "hdc" not in  disk_blk:
+        logging.info("Need add CDROM device")
+        add_cdrom_device(vm_name, init_cdrom)
+
+    if vm.is_alive() and start_vm == "no":
+        vm.destroy()
+        logging.info("Destroy guest !")
+
+    elif vm.is_dead() and start_vm == "yes":
+        vm.start()
+        logging.info("Start guest !")
+
+    update_cdrom(vm_name, init_iso, options, start_vm)
+    disk_device = params.get("disk_device")
+    libvirtd = params.get("libvirtd", "on")
+
+    if libvirtd == "off":
+        libvirt_vm.libvirtd_stop()
+
+    source_name = params.get("change_media_source")
+    #libvirt will ignore --source when action is eject
+    if action == "--eject ":
+        source = ""
+
+    else:
+        source = cdrom_dir + source_name
+
+    all_options = action + options + " " + source
+    result = virsh.change_media(vm_ref, disk_device, \
+ all_options, ignore_status=True, debug=True)
+    status = result.exit_status
+    status_error =  params.get("status_error", "no")
+
+    if status_error == "no":
+        if options == "--config" and vm.is_alive():
+            vm.destroy()
+
+        vm.start()
+        session = vm.wait_for_login()
+        check_file = params.get("check_file")
+        check_media(session, check_file, action)
+        session.close()
+
+    #recover libvirtd service start
+    if libvirtd == "off":
+        libvirt_vm.libvirtd_start()
+
+    #clean the cdrom dir  and clean the cdrom device
+    update_cdrom(vm_name, "", options, start_vm)
+    utils.run("rm -rf %s/*" % cdrom_dir)

^ Gratuitous use of shell

+    # Check status_error
+
+    if status_error == "yes":
+        if status:
+            logging.info("It's an expected error")
+
+        else:
+            raise error.TestFail("%d not a expected command "
+                                 "return value", status)
+    elif status_error == "no":
+        if status:
+            raise error.TestFail(result.stderr)
+
+        else:
+            logging.info(result.stdout)
+
+    else:
+        raise error.TestFail("The status_error must be 'yes' or 'no'!")



--
Best Regards!
Wenlong Huang
IRC Account: wenlong
Phone: 62608117 / 15011214521

_______________________________________________
Virt-test-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/virt-test-devel

Reply via email to