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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
