| Message ID | 1242849851-8029-1-git-send-email-mburns@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Mike Burns wrote: > + > +def __unload_modules(module): > + lsmod = os.popen("lsmod | grep \"^%s \"" % module) > lsmod = len([x for x in file('/proc/modules).splitlines() if x.split()[0] == module]) > 0 Sorry, I just envy python writers. > + > + #line_count = os.popen("lsmod | grep \"^%s \" | wc -l" % module) > + #if line_count > 0: > + #kvm_log.info("Found module %s, checking for dependecies..." %module) > + #dependencies = os.popen("lsmod | grep \"^%s \" | awk '{print $4}'" % module).readline() > + #submodules = dependencies.split(",") > + #for submodule in submodules: > + #__unload_modules(submodule) > +# > + # > + #kvm_log.info("Found module %s" % module) > + #dependencies = os.popen("lsmod | grep \"^%s \" | awk '{print $4}'" % module) > + #for line in dependencies.readlines(): > + #print "Line: %s" % line > + #submodules = line.split(",") > +## for submodule in submodules: > +## __unload_modules(submodule) > + #raise error.TestError, "Aborting..." > + #kvm_log.info("Deleting Module %s" % module) > + #utils.system("/sbin/modprobe -r %s" % module) > + > Please don't add dead code.
Hi Mike, On Wed, 2009-05-20 at 16:04 -0400, Mike Burns wrote: > + > +def __unload_modules(module): > + lsmod = os.popen("lsmod | grep \"^%s \"" % module) > + line_parts = lsmod.readline().split() > + if len(line_parts) == 0: > + kvm_log.info("%s not loaded." % module) > + else: > + kvm_log.info("Found module %s, checking for dependecies..." %module) > + if len(line_parts) == 4: > + submodules = line_parts[3].split(",") > + for submodule in submodules: > + __unload_modules(submodule) > + else: > + kvm_log.info("No modules dependent on %s" % module ) > + > + kvm_log.info("Removing module: %s" % module) > + utils.system("/sbin/modprobe -r %s" % module, ignore_status=False) ignore_status is False by default, so no need to do explicitly pass it to the function. Also, as we are putting something that can throw an exception, you can wrap it in a try/except module that throws a error.TestError exception in case our module removal still fails. Other than that, I agree with Avi that we should avoid adding commented lines, and prefer python constructs over shell ones.
Lucas Meneghel Rodrigues wrote: > Hi Mike, > > On Wed, 2009-05-20 at 16:04 -0400, Mike Burns wrote: > >> + >> +def __unload_modules(module): >> + lsmod = os.popen("lsmod | grep \"^%s \"" % module) >> + line_parts = lsmod.readline().split() >> + if len(line_parts) == 0: >> + kvm_log.info("%s not loaded." % module) >> + else: >> + kvm_log.info("Found module %s, checking for dependecies..." %module) >> + if len(line_parts) == 4: >> + submodules = line_parts[3].split(",") >> + for submodule in submodules: >> + __unload_modules(submodule) >> + else: >> + kvm_log.info("No modules dependent on %s" % module ) >> + >> + kvm_log.info("Removing module: %s" % module) >> + utils.system("/sbin/modprobe -r %s" % module, ignore_status=False) >> > > ignore_status is False by default, so no need to do explicitly pass it > to the function. Also, as we are putting something that can throw an > exception, you can wrap it in a try/except module that throws a > error.TestError exception in case our module removal still fails. > Wrapping every function with try/except defeats the purpose of exceptions. Can't autotest handle arbitrary exceptions?
On Mon, 2009-05-25 at 19:40 +0300, Avi Kivity wrote: > Lucas Meneghel Rodrigues wrote: > > Hi Mike, > > > > On Wed, 2009-05-20 at 16:04 -0400, Mike Burns wrote: > > > >> + > >> +def __unload_modules(module): > >> + lsmod = os.popen("lsmod | grep \"^%s \"" % module) > >> + line_parts = lsmod.readline().split() > >> + if len(line_parts) == 0: > >> + kvm_log.info("%s not loaded." % module) > >> + else: > >> + kvm_log.info("Found module %s, checking for dependecies..." %module) > >> + if len(line_parts) == 4: > >> + submodules = line_parts[3].split(",") > >> + for submodule in submodules: > >> + __unload_modules(submodule) > >> + else: > >> + kvm_log.info("No modules dependent on %s" % module ) > >> + > >> + kvm_log.info("Removing module: %s" % module) > >> + utils.system("/sbin/modprobe -r %s" % module, ignore_status=False) > >> > > > > ignore_status is False by default, so no need to do explicitly pass it > > to the function. Also, as we are putting something that can throw an > > exception, you can wrap it in a try/except module that throws a > > error.TestError exception in case our module removal still fails. > > > > Wrapping every function with try/except defeats the purpose of > exceptions. Can't autotest handle arbitrary exceptions? Yes it can, the point is that a failure trying to unload the module leads to a test failure, so the only idea here is try to mark the test as failed (with error.TestFail exception). The CmdError will mark the test as failed anyway, so my suggestion of wrapping the utils.system call with a try/except block is not necessary, Mike can just remove the ignore_status assignment and it's done.
diff --git a/client/tests/kvm_runtest_2/kvm_install.py b/client/tests/kvm_runtest_2/kvm_install.py index 5429a65..d1aceb2 100755 --- a/client/tests/kvm_runtest_2/kvm_install.py +++ b/client/tests/kvm_runtest_2/kvm_install.py @@ -86,11 +86,11 @@ def run_kvm_install(test, params, env): kvm_log.error(message) raise error.TestError, message for k in params.keys(): - kvm_log.info("Adding KVM_INSTALL_%s to Environment" % (k)) - os.putenv("KVM_INSTALL_%s" % (k), str(params[k])) - kvm_log.info("Running " + script + " to install kvm") + kvm_log.info("Adding KVM_INSTALL_%s to Environment" % (k)) + os.putenv("KVM_INSTALL_%s" % (k), str(params[k])) + kvm_log.info("Running " + script + " to install kvm") os.system("cd %s; %s" % (test.bindir, script)) - kvm_log.info("Completed %s" % (script)) + kvm_log.info("Completed %s" % (script)) # invalid installation mode else: @@ -215,11 +215,11 @@ def __load_kvm_modules(): kvm_log.info("Unloading loaded KVM modules (if present)...") #utils.system("pkill qemu 1>/dev/null 2>&1", ignore_status=True) utils.system("pkill qemu", ignore_status=True) - #if utils.system("grep kvm_%s /proc/modules 1>/dev/null" % vendor, ignore_status=True) == 0: - utils.system("/sbin/rmmod kvm_%s" % vendor, ignore_status=True) - #if utils.system("grep kvm /proc/modules 1>/dev/null", ignore_status=True) == 0: - utils.system("/sbin/rmmod kvm", ignore_status=True) + # Remove existing modules + kvm_log.info("Checking KVM module") + __unload_modules("kvm") + if utils.system("grep kvm /proc/modules 1>/dev/null", ignore_status=True) == 0: message = "Failed to remove old KVM modules" kvm_log.error(message) @@ -241,6 +241,46 @@ def __load_kvm_modules(): kvm_log.error(message) raise error.TestError, message + +def __unload_modules(module): + lsmod = os.popen("lsmod | grep \"^%s \"" % module) + line_parts = lsmod.readline().split() + if len(line_parts) == 0: + kvm_log.info("%s not loaded." % module) + else: + kvm_log.info("Found module %s, checking for dependecies..." %module) + if len(line_parts) == 4: + submodules = line_parts[3].split(",") + for submodule in submodules: + __unload_modules(submodule) + else: + kvm_log.info("No modules dependent on %s" % module ) + + kvm_log.info("Removing module: %s" % module) + utils.system("/sbin/modprobe -r %s" % module, ignore_status=False) + + + + #line_count = os.popen("lsmod | grep \"^%s \" | wc -l" % module) + #if line_count > 0: + #kvm_log.info("Found module %s, checking for dependecies..." %module) + #dependencies = os.popen("lsmod | grep \"^%s \" | awk '{print $4}'" % module).readline() + #submodules = dependencies.split(",") + #for submodule in submodules: + #__unload_modules(submodule) +# + # + #kvm_log.info("Found module %s" % module) + #dependencies = os.popen("lsmod | grep \"^%s \" | awk '{print $4}'" % module) + #for line in dependencies.readlines(): + #print "Line: %s" % line + #submodules = line.split(",") +## for submodule in submodules: +## __unload_modules(submodule) + #raise error.TestError, "Aborting..." + #kvm_log.info("Deleting Module %s" % module) + #utils.system("/sbin/modprobe -r %s" % module) + def __install_kvm(test, srcdir): # create destination dir
Signed-off-by: Mike Burns <mburns@redhat.com> --- client/tests/kvm_runtest_2/kvm_install.py | 56 ++++++++++++++++++++++++---- 1 files changed, 48 insertions(+), 8 deletions(-)