[PATCH][KVM-AUTOTEST] Make code to remove kvm modules more robust

[PATCH][KVM-AUTOTEST] Make code to remove kvm modules more robust

[Mike Burns]

Signed-off-by: Mike Burns <mburns@redhat.com>
---
 client/tests/kvm_runtest_2/kvm_install.py |   56 ++++++++++++++++++++++++----
 1 files changed, 48 insertions(+), 8 deletions(-)

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
 
-- 
1.5.5.6

Re: [PATCH][KVM-AUTOTEST] Make code to remove kvm modules more robust

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH][KVM-AUTOTEST] Make code to remove kvm modules more robust

[Lucas Meneghel Rodrigues]

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
Software Engineer (QE)
Red Hat - Emerging Technologies

Re: [PATCH][KVM-AUTOTEST] Make code to remove kvm modules more robust

[Avi Kivity]

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?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH][KVM-AUTOTEST] Make code to remove kvm modules more robust

[Lucas Meneghel Rodrigues]

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.

-- 
Lucas Meneghel Rodrigues
Software Engineer (QE)
Red Hat - Emerging Technologies