Patchworkβ [KVM-AUTOTEST,5/7] KVM test: minor pci_hotplug fixes

- Put the PCI device removal code in a finally clause.
- Use kvm_vm.get_image_filename() instead of os.path.join().
It's a bit cleaner because if we ever change the names of image parameters
we'll only have to change the code in one place.
Also, the way os.path.join() was used lead to image filenames being prefixed
with 'images/' twice, e.g. 'images/images/foo.qcow2'.
- Make some failure messages clearer.
- Remove 'only Fedora Ubuntu Windows' from the fmt_raw variant.
'only' works for things that have already been defined, but the guests are
defined later.
- Remove unused 'modprobe_acpiphp' parameter.
- Change 'online disk' to 'online' in pci_test_cmd for Windows ('online disk'
doesn't seem to work).
- Remove the unneeded telnet/ssh/guest_port parameters from the Windows
block_hotplug parameters.

Signed-off-by: Michael Goldish <mgoldish@redhat.com>
---
 client/tests/kvm/kvm_tests.cfg.sample |    7 +--
 client/tests/kvm/tests/pci_hotplug.py |  112 +++++++++++++++++----------------
 2 files changed, 58 insertions(+), 61 deletions(-)

On Thu, Nov 05, 2009 at 12:01:10PM +0200, Michael Goldish wrote:
> - Put the PCI device removal code in a finally clause.

Hi Michael,

I have a little concern with the removal procedure. Thinking about if
pci_add failed, the output will not contain right information including PCI
ID. The slice operation during removing therefore could involve traceback
and removal will be failed at the end. Thus I would not place it in a finally
clause at that time.

But looking from opposite side, if we don't place it in a finally
clause and, pci_add is succeeded whereas verification is failed, the
device will not be removed finally. 

We may need to balance both if possible. Do you have any idea about
this? I can agree on applying the method proposed by this patch first.

> - Use kvm_vm.get_image_filename() instead of os.path.join().
> It's a bit cleaner because if we ever change the names of image parameters
> we'll only have to change the code in one place.
> Also, the way os.path.join() was used lead to image filenames being prefixed
> with 'images/' twice, e.g. 'images/images/foo.qcow2'.
> - Make some failure messages clearer.
> - Remove 'only Fedora Ubuntu Windows' from the fmt_raw variant.
> 'only' works for things that have already been defined, but the guests are
> defined later.
> - Remove unused 'modprobe_acpiphp' parameter.
> - Change 'online disk' to 'online' in pci_test_cmd for Windows ('online disk'
> doesn't seem to work).
> - Remove the unneeded telnet/ssh/guest_port parameters from the Windows
> block_hotplug parameters.
> 
> Signed-off-by: Michael Goldish <mgoldish@redhat.com>
> ---
>  client/tests/kvm/kvm_tests.cfg.sample |    7 +--
>  client/tests/kvm/tests/pci_hotplug.py |  112 +++++++++++++++++----------------
>  2 files changed, 58 insertions(+), 61 deletions(-)
> 
> diff --git a/client/tests/kvm/kvm_tests.cfg.sample b/client/tests/kvm/kvm_tests.cfg.sample
> index f271a09..326ae20 100644
> --- a/client/tests/kvm/kvm_tests.cfg.sample
> +++ b/client/tests/kvm/kvm_tests.cfg.sample
> @@ -181,7 +181,6 @@ variants:
>      - nic_hotplug:  install setup unattended_install
>          type = pci_hotplug
>          pci_type = nic
> -        modprobe_acpiphp = no
>          reference_cmd = lspci
>          find_pci_cmd = 'lspci | tail -n1'
>          pci_test_cmd = 'nslookup www.redhat.com'
> @@ -223,7 +222,6 @@ variants:
>                  image_format_stg = qcow2
>              - fmt_raw:
>                  image_format_stg = raw
> -                only Fedora Ubuntu Windows
>  
>      - system_reset: install setup unattended_install
>          type = boot
> @@ -538,13 +536,10 @@ variants:
>              nic_virtio:
>                  match_string = "VirtIO Ethernet"
>          block_hotplug:
> -            use_telnet = yes
> -            ssh_port = 23
> -            guest_port_ssh = 23
>              wait_secs_for_hook_up = 10
>              reference_cmd = wmic diskdrive list brief
>              find_pci_cmd = wmic diskdrive list brief
> -            pci_test_cmd = echo select disk 1 > dt && echo online disk >> dt && echo detail disk >> dt && echo exit >> dt && diskpart /s dt
> +            pci_test_cmd = echo select disk 1 > dt && echo online >> dt && echo detail disk >> dt && echo exit >> dt && diskpart /s dt
>  
>          variants:
>              - Win2000:
> diff --git a/client/tests/kvm/tests/pci_hotplug.py b/client/tests/kvm/tests/pci_hotplug.py
> index 3ad9ea2..876d8b8 100644
> --- a/client/tests/kvm/tests/pci_hotplug.py
> +++ b/client/tests/kvm/tests/pci_hotplug.py
> @@ -1,6 +1,6 @@
>  import logging, os
>  from autotest_lib.client.common_lib import error
> -import kvm_subprocess, kvm_test_utils, kvm_utils
> +import kvm_subprocess, kvm_test_utils, kvm_utils, kvm_vm
>  
>  
>  def run_pci_hotplug(test, params, env):
> @@ -21,8 +21,8 @@ def run_pci_hotplug(test, params, env):
>      session = kvm_test_utils.wait_for_login(vm)
>  
>      # Modprobe the module if specified in config file
> -    if params.get("modprobe_module"):
> -        module = params.get("modprobe_module")
> +    module = params.get("modprobe_module")
> +    if module:
>          if session.get_command_status("modprobe %s" % module):
>              raise error.TestError("Modprobe module '%s' failed" % module)
>  
> @@ -38,61 +38,63 @@ def run_pci_hotplug(test, params, env):
>      if test_type == "nic":
>          pci_add_cmd = "pci_add pci_addr=auto nic model=%s" % tested_model
>      elif test_type == "block":
> -        image_name = params.get("image_name_stg")
> -        image_filename = "%s.%s" % (image_name, params.get("image_format_stg"))
> -        image_dir = os.path.join(test.bindir, "images")
> -        storage_name = os.path.join(image_dir, image_filename)
> +        image_params = kvm_utils.get_sub_dict(params, "stg")
> +        image_filename = kvm_vm.get_image_filename(image_params, test.bindir)
>          pci_add_cmd = ("pci_add pci_addr=auto storage file=%s,if=%s" %
> -                                    (storage_name, tested_model))
> +                                    (image_filename, tested_model))
>  
>      # Implement pci_add
>      s, add_output = vm.send_monitor_cmd(pci_add_cmd)
>      if not "OK domain" in add_output:
>          raise error.TestFail("Add device failed. Hypervisor command is: %s. "
> -                             "Output: %s" % (pci_add_cmd, add_output))
> -
> -    # Compare the output of 'info pci'
> -    s, after_add = vm.send_monitor_cmd("info pci")
> -    if after_add == info_pci_ref:
> -        raise error.TestFail("No new PCI device shown after executing "
> -                             "hypervisor command: 'info pci'")
> -
> -    # Define a helper function to compare the output
> -    def new_shown():
> -        o = session.get_command_output(params.get("reference_cmd"))
> -        if reference == o:
> -            return False
> -        return True
> -
> -    secs = int(params.get("wait_secs_for_hook_up"))
> -    if not kvm_utils.wait_for(new_shown, 30, secs, 3):
> -        raise error.TestFail("No new device shown in output of command "
> -                             "executed inside the guest: %s" %
> -                             params.get("reference_cmd"))
> -
> -    # Define a helper function to catch PCI device string
> -    def find_pci():
> -        output = session.get_command_output(params.get("find_pci_cmd"))
> -        if not params.get("match_string") in output:
> -            return False
> -        return True
> -
> -    if not kvm_utils.wait_for(find_pci, 30, 3, 3):
> -        raise error.TestFail("PCI model not found: %s. Command is: %s" %
> -                             (tested_model, params.get("find_pci_cmd")))
> -
> -    # Test the newly added device
> -    s, o = session.get_command_status_output(params.get("pci_test_cmd"))
> -    if s != 0:
> -        raise error.TestFail("Check for %s device failed after PCI hotplug. "
> -                             "Output: %s" % (test_type, o))
> -
> -    # Delete the added pci device
> -    slot_id = "0" + add_output.split(",")[2].split()[1]
> -    cmd = "pci_del pci_addr=%s" % slot_id
> -    s, after_del = vm.send_monitor_cmd(cmd)
> -    if after_del == after_add:
> -        raise error.TestFail("Failed to hot remove PCI device: %s. "
> -                             "Hypervisor command: %s" % (tested_model, cmd))
> -
> -    session.close()
> +                             "Output: %r" % (pci_add_cmd, add_output))
> +
> +    try:
> +        # Compare the output of 'info pci'
> +        s, after_add = vm.send_monitor_cmd("info pci")
> +        if after_add == info_pci_ref:
> +            raise error.TestFail("No new PCI device shown after executing "
> +                                 "hypervisor command: 'info pci'")
> +
> +        # Define a helper function to compare the output
> +        def new_shown():
> +            o = session.get_command_output(params.get("reference_cmd"))
> +            if reference == o:
> +                return False
> +            return True
> +
> +        secs = int(params.get("wait_secs_for_hook_up"))
> +        if not kvm_utils.wait_for(new_shown, 30, secs, 3):
> +            raise error.TestFail("No new device shown in output of command "
> +                                 "executed inside the guest: %s" %
> +                                 params.get("reference_cmd"))
> +
> +        # Define a helper function to catch PCI device string
> +        def find_pci():
> +            output = session.get_command_output(params.get("find_pci_cmd"))
> +            if not params.get("match_string") in output:
> +                return False
> +            return True
> +
> +        if not kvm_utils.wait_for(find_pci, 30, 3, 3):
> +            raise error.TestFail("PCI %s %s device not found in guest. "
> +                                 "Command was: %s" %
> +                                 (tested_model, test_type,
> +                                  params.get("find_pci_cmd")))
> +
> +        # Test the newly added device
> +        s, o = session.get_command_status_output(params.get("pci_test_cmd"))
> +        if s != 0:
> +            raise error.TestFail("Check for %s device failed after PCI "
> +                                 "hotplug. Output: %r" % (test_type, o))
> +
> +    finally:
> +        # Delete the added pci device
> +        slot_id = "0" + add_output.split(",")[2].split()[1]
> +        cmd = "pci_del pci_addr=%s" % slot_id
> +        s, after_del = vm.send_monitor_cmd(cmd)
> +        if after_del == after_add:
> +            raise error.TestFail("Failed to hot remove PCI device: %s. "
> +                                 "Hypervisor command: %s" % (tested_model,
> +                                                             cmd))
> +        session.close()
> -- 
> 1.5.4.1
> 
> _______________________________________________
> Autotest mailing list
> Autotest@test.kernel.org
> http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
--
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  http://vger.kernel.org/majordomo-info.html