Re: [AUTOTEST PATCH 2/2] KVM-test: Move two 'remote_login' out of try block in kvm_vm.py

Re: [AUTOTEST PATCH 2/2] KVM-test: Move two 'remote_login' out of try block in kvm_vm.py

[Michael Goldish]

----- "Yolkfull Chow" <yzhou@redhat.com> wrote:

> If vm.remote_login failed 'session.close()' can result in exception
> in finally clause. This patch fix the problem.
> 
> Signed-off-by: Yolkfull Chow <yzhou@redhat.com>
> ---
>  client/tests/kvm/kvm_vm.py |   51
> +++++++++++++++++++++----------------------
>  1 files changed, 25 insertions(+), 26 deletions(-)
> 
> diff --git a/client/tests/kvm/kvm_vm.py b/client/tests/kvm/kvm_vm.py
> index 7229b79..f746331 100755
> --- a/client/tests/kvm/kvm_vm.py
> +++ b/client/tests/kvm/kvm_vm.py
> @@ -827,40 +827,39 @@ class VM:
>          """
>          Get the cpu count of the VM.
>          """
> +        session = self.remote_login()
> +        cmd = self.params.get("cpu_chk_cmd")
>          try:
> -            session = self.remote_login()
> -            if session:
> -                cmd = self.params.get("cpu_chk_cmd")
> -                s, count = session.get_command_status_output(cmd)
> -                if s == 0:
> -                    return int(count)
> +            s, count = session.get_command_status_output(cmd)
> +            if s == 0:
> +                return int(count)
>              return None
>          finally:
> -            session.close()
> +            if session:
> +                session.close()

If self.remote_login() fails, session will be None, and then the attempted
call to session.get_command_status_output() will raise an exception which
will fail the test with a rather ugly "reason" string.

I think this form is preferable:

session = self.remote_login()
if not session:
    return None
try:
    cmd = ...
    s, count = ...
    if s == 0:
        return int(count)
    return None
finally:
    session.close()

There's no need for a second 'if session' in the finally block.

>      def get_memory_size(self):
>          """
>          Get memory size of the VM.
>          """
> +        session = self.remote_login()
> +        cmd = self.params.get("mem_chk_cmd")
>          try:
> -            session = self.remote_login()
> -            if session:
> -                cmd = self.params.get("mem_chk_cmd")
> -                s, mem_str = session.get_command_status_output(cmd)
> -                if s != 0:
> -                    return None
> -                mem = re.findall("([0-9][0-9][0-9]+)", mem_str)
> -                mem_size = 0
> -                for m in mem:
> -                    mem_size += int(m)
> -                if "GB" in mem_str:
> -                    mem_size *= 1024
> -                elif "MB" in mem_str:
> -                    pass
> -                else:
> -                    mem_size /= 1024
> -                return int(mem_size)
> -            return None
> +            s, mem_str = session.get_command_status_output(cmd)
> +            if s != 0:
> +                return None
> +            mem = re.findall("([0-9][0-9][0-9]+)", mem_str)
> +            mem_size = 0
> +            for m in mem:
> +                mem_size += int(m)
> +            if "GB" in mem_str:
> +                mem_size *= 1024
> +            elif "MB" in mem_str:
> +                pass
> +            else:
> +                mem_size /= 1024
> +            return int(mem_size)
>          finally:
> -            session.close()
> +            if session:
> +                session.close()

The same applies to this function.

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

Re: [Autotest] [AUTOTEST PATCH 2/2] KVM-test: Move two 'remote_login' out of try block in kvm_vm.py

[Lucas Meneghel Rodrigues]

On Thu, Dec 31, 2009 at 8:03 AM, Michael Goldish <mgoldish@redhat.com> wrote:
>
> ----- "Yolkfull Chow" <yzhou@redhat.com> wrote:
>
>> If vm.remote_login failed 'session.close()' can result in exception
>> in finally clause. This patch fix the problem.

Hi Yolkfull, please implement Michael's comments and re-submit.

Cheers,

-- 
Lucas

Re: [AUTOTEST PATCH 2/2] KVM-test: Move two 'remote_login' out of try block in kvm_vm.py

[Yolkfull Chow]

On Tue, Jan 12, 2010 at 08:37:25PM -0200, Lucas Meneghel Rodrigues wrote:
> On Thu, Dec 31, 2009 at 8:03 AM, Michael Goldish <mgoldish@redhat.com> wrote:
> >
> > ----- "Yolkfull Chow" <yzhou@redhat.com> wrote:
> >
> >> If vm.remote_login failed 'session.close()' can result in exception
> >> in finally clause. This patch fix the problem.
> 
> Hi Yolkfull, please implement Michael's comments and re-submit.

Sorry, hadn't seen the reply from Michael. I will post the updated patch soon.

Thanks, Lucas. :)

> 
> Cheers,
> 
> -- 
> Lucas
> --
> 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

Re: [AUTOTEST PATCH 2/2] KVM-test: Move two 'remote_login' out of try block in kvm_vm.py

[Yolkfull Chow]

On Thu, Dec 31, 2009 at 05:03:21AM -0500, Michael Goldish wrote:
> 
> ----- "Yolkfull Chow" <yzhou@redhat.com> wrote:
> 
> > If vm.remote_login failed 'session.close()' can result in exception
> > in finally clause. This patch fix the problem.
> > 
> > Signed-off-by: Yolkfull Chow <yzhou@redhat.com>
> > ---
> >  client/tests/kvm/kvm_vm.py |   51
> > +++++++++++++++++++++----------------------
> >  1 files changed, 25 insertions(+), 26 deletions(-)
> > 
> > diff --git a/client/tests/kvm/kvm_vm.py b/client/tests/kvm/kvm_vm.py
> > index 7229b79..f746331 100755
> > --- a/client/tests/kvm/kvm_vm.py
> > +++ b/client/tests/kvm/kvm_vm.py
> > @@ -827,40 +827,39 @@ class VM:
> >          """
> >          Get the cpu count of the VM.
> >          """
> > +        session = self.remote_login()
> > +        cmd = self.params.get("cpu_chk_cmd")
> >          try:
> > -            session = self.remote_login()
> > -            if session:
> > -                cmd = self.params.get("cpu_chk_cmd")
> > -                s, count = session.get_command_status_output(cmd)
> > -                if s == 0:
> > -                    return int(count)
> > +            s, count = session.get_command_status_output(cmd)
> > +            if s == 0:
> > +                return int(count)
> >              return None
> >          finally:
> > -            session.close()
> > +            if session:
> > +                session.close()
> 
> If self.remote_login() fails, session will be None, and then the attempted
> call to session.get_command_status_output() will raise an exception which
> will fail the test with a rather ugly "reason" string.
> 
> I think this form is preferable:
> 
> session = self.remote_login()
> if not session:
>     return None

Reasonable, thanks Michael. :)
Will post the updated patch soon.

> try:
>     cmd = ...
>     s, count = ...
>     if s == 0:
>         return int(count)
>     return None
> finally:
>     session.close()
> 
> There's no need for a second 'if session' in the finally block.
> 
> >      def get_memory_size(self):
> >          """
> >          Get memory size of the VM.
> >          """
> > +        session = self.remote_login()
> > +        cmd = self.params.get("mem_chk_cmd")
> >          try:
> > -            session = self.remote_login()
> > -            if session:
> > -                cmd = self.params.get("mem_chk_cmd")
> > -                s, mem_str = session.get_command_status_output(cmd)
> > -                if s != 0:
> > -                    return None
> > -                mem = re.findall("([0-9][0-9][0-9]+)", mem_str)
> > -                mem_size = 0
> > -                for m in mem:
> > -                    mem_size += int(m)
> > -                if "GB" in mem_str:
> > -                    mem_size *= 1024
> > -                elif "MB" in mem_str:
> > -                    pass
> > -                else:
> > -                    mem_size /= 1024
> > -                return int(mem_size)
> > -            return None
> > +            s, mem_str = session.get_command_status_output(cmd)
> > +            if s != 0:
> > +                return None
> > +            mem = re.findall("([0-9][0-9][0-9]+)", mem_str)
> > +            mem_size = 0
> > +            for m in mem:
> > +                mem_size += int(m)
> > +            if "GB" in mem_str:
> > +                mem_size *= 1024
> > +            elif "MB" in mem_str:
> > +                pass
> > +            else:
> > +                mem_size /= 1024
> > +            return int(mem_size)
> >          finally:
> > -            session.close()
> > +            if session:
> > +                session.close()
> 
> The same applies to this function.
> 
> > -- 
> > 1.6.6
> > 
> > --
> > 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

[AUTOTEST PATCH 1/2] KVM-test: Fix a bug that about list slice in scan_results.py

[Yolkfull Chow]

If 'info_list' is empty slice operation can result in traceback:

...
      info_list[-1] = parts[5]
  IndexError: list assignment index out of range

Signed-off-by: Yolkfull Chow <yzhou@redhat.com>
---
 client/tests/kvm/scan_results.py |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/client/tests/kvm/scan_results.py b/client/tests/kvm/scan_results.py
index f7bafa9..1daff2f 100755
--- a/client/tests/kvm/scan_results.py
+++ b/client/tests/kvm/scan_results.py
@@ -45,6 +45,7 @@ def parse_results(text):
         # Found a FAIL/ERROR/GOOD line -- get failure/success info
         elif (len(parts) >= 6 and parts[3].startswith("timestamp") and
               parts[4].startswith("localtime")):
+            info_list.append("")
             info_list[-1] = parts[5]
 
     return result_list
-- 
1.6.6

[AUTOTEST PATCH 2/2] KVM-test: Move two 'remote_login' out of try block in kvm_vm.py

[Yolkfull Chow]

If vm.remote_login failed 'session.close()' can result in exception
in finally clause. This patch fix the problem.

Signed-off-by: Yolkfull Chow <yzhou@redhat.com>
---
 client/tests/kvm/kvm_vm.py |   51 +++++++++++++++++++++----------------------
 1 files changed, 25 insertions(+), 26 deletions(-)

diff --git a/client/tests/kvm/kvm_vm.py b/client/tests/kvm/kvm_vm.py
index 7229b79..f746331 100755
--- a/client/tests/kvm/kvm_vm.py
+++ b/client/tests/kvm/kvm_vm.py
@@ -827,40 +827,39 @@ class VM:
         """
         Get the cpu count of the VM.
         """
+        session = self.remote_login()
+        cmd = self.params.get("cpu_chk_cmd")
         try:
-            session = self.remote_login()
-            if session:
-                cmd = self.params.get("cpu_chk_cmd")
-                s, count = session.get_command_status_output(cmd)
-                if s == 0:
-                    return int(count)
+            s, count = session.get_command_status_output(cmd)
+            if s == 0:
+                return int(count)
             return None
         finally:
-            session.close()
+            if session:
+                session.close()
 
 
     def get_memory_size(self):
         """
         Get memory size of the VM.
         """
+        session = self.remote_login()
+        cmd = self.params.get("mem_chk_cmd")
         try:
-            session = self.remote_login()
-            if session:
-                cmd = self.params.get("mem_chk_cmd")
-                s, mem_str = session.get_command_status_output(cmd)
-                if s != 0:
-                    return None
-                mem = re.findall("([0-9][0-9][0-9]+)", mem_str)
-                mem_size = 0
-                for m in mem:
-                    mem_size += int(m)
-                if "GB" in mem_str:
-                    mem_size *= 1024
-                elif "MB" in mem_str:
-                    pass
-                else:
-                    mem_size /= 1024
-                return int(mem_size)
-            return None
+            s, mem_str = session.get_command_status_output(cmd)
+            if s != 0:
+                return None
+            mem = re.findall("([0-9][0-9][0-9]+)", mem_str)
+            mem_size = 0
+            for m in mem:
+                mem_size += int(m)
+            if "GB" in mem_str:
+                mem_size *= 1024
+            elif "MB" in mem_str:
+                pass
+            else:
+                mem_size /= 1024
+            return int(mem_size)
         finally:
-            session.close()
+            if session:
+                session.close()
-- 
1.6.6