Re: [Autotest PATCH] KVM test: No need close session when login timeout

Re: [Autotest PATCH] KVM test: No need close session when login timeout

[Michael Goldish]

----- "Amos Kong" <akong@redhat.com> wrote:

> If login timeout, wait_for() returned 'None' and assigned to
> 'session'.
> When call session.close(), this prlblem was caused:
> "AttributeError: 'NoneType' object has no attribute 'close'"
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  client/tests/kvm/tests/timedrift_with_migration.py |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/client/tests/kvm/tests/timedrift_with_migration.py
> b/client/tests/kvm/tests/timedrift_with_migration.py
> index a012db3..0b93183 100644
> --- a/client/tests/kvm/tests/timedrift_with_migration.py
> +++ b/client/tests/kvm/tests/timedrift_with_migration.py
> @@ -76,7 +76,8 @@ def run_timedrift_with_migration(test, params,
> env):
>                                               time_filter_re,
> time_format)
>  
>      finally:
> -        session.close()
> +        if session != None:
> +            session.close()

Agreed, but we can make this simply:

if session:
    session.close()

There's no need to explicitly check for None (and if there was,
the preferred syntax would be 'is not None' rather than '!= None').

Also, just to be safe, we should make the same modification to
timedrift_with_reboot.py.

We can also consider removing the try..finally clauses altogether
because sessions are now closed automatically when they're no longer
needed.

>  
>      # Report results
>      host_delta = ht1 - ht0
> -- 
> 1.5.5.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 PATCH] KVM test: No need close session when login timeout

[Amos Kong]

On Fri, Dec 25, 2009 at 08:28:18AM -0500, Michael Goldish wrote:
> 
> ----- "Amos Kong" <akong@redhat.com> wrote:
> 
> > If login timeout, wait_for() returned 'None' and assigned to
> > 'session'.
> > When call session.close(), this prlblem was caused:
> > "AttributeError: 'NoneType' object has no attribute 'close'"
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  client/tests/kvm/tests/timedrift_with_migration.py |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/client/tests/kvm/tests/timedrift_with_migration.py
> > b/client/tests/kvm/tests/timedrift_with_migration.py
> > index a012db3..0b93183 100644
> > --- a/client/tests/kvm/tests/timedrift_with_migration.py
> > +++ b/client/tests/kvm/tests/timedrift_with_migration.py
> > @@ -76,7 +76,8 @@ def run_timedrift_with_migration(test, params,
> > env):
> >                                               time_filter_re,
> > time_format)
> >  
> >      finally:
> > -        session.close()
> > +        if session != None:
> > +            session.close()
> 
> Agreed, but we can make this simply:
> 
> if session:
>     session.close()

Yes,

 
> There's no need to explicitly check for None (and if there was,
> the preferred syntax would be 'is not None' rather than '!= None').
> 
> Also, just to be safe, we should make the same modification to
> timedrift_with_reboot.py.


In timedrift_with_reboot.py, 'session' has been assigned before 'try'. 
If re-login timout, kvm_test_utils.reboot() returns nothing, the value of 'session' isn't changed.
So session.close() couldn't cause this problem:
"AttributeError: 'NoneType' object has no attribute 'close'"



In other testcases, if session wasn't assigned before 'try',
when calling kvm_test_utils.wait_for_login()/kvm_test_utils.reboot() timeout,
It returns nothing, if close 'session' in finally part, Another problem will occur:
"NameError: name 'session' is not defined"

In this condition,
""" 
if session:
    session.close()
"""
also causes this error.



> We can also consider removing the try..finally clauses altogether
> because sessions are now closed automatically when they're no longer
> needed.
> 
> >  
> >      # Report results
> >      host_delta = ht1 - ht0
> > -- 
> > 1.5.5.6

-- 
Amos Kong
Quality Engineer
Raycom Office(Beijing), Red Hat Inc.

Re: [Autotest PATCH] KVM test: No need close session when login timeout

[Michael Goldish]

----- "Amos Kong" <akong@redhat.com> wrote:

> On Fri, Dec 25, 2009 at 08:28:18AM -0500, Michael Goldish wrote:
> > 
> > ----- "Amos Kong" <akong@redhat.com> wrote:
> > 
> > > If login timeout, wait_for() returned 'None' and assigned to
> > > 'session'.
> > > When call session.close(), this prlblem was caused:
> > > "AttributeError: 'NoneType' object has no attribute 'close'"
> > > 
> > > Signed-off-by: Amos Kong <akong@redhat.com>
> > > ---
> > >  client/tests/kvm/tests/timedrift_with_migration.py |    3 ++-
> > >  1 files changed, 2 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/client/tests/kvm/tests/timedrift_with_migration.py
> > > b/client/tests/kvm/tests/timedrift_with_migration.py
> > > index a012db3..0b93183 100644
> > > --- a/client/tests/kvm/tests/timedrift_with_migration.py
> > > +++ b/client/tests/kvm/tests/timedrift_with_migration.py
> > > @@ -76,7 +76,8 @@ def run_timedrift_with_migration(test, params,
> > > env):
> > >                                               time_filter_re,
> > > time_format)
> > >  
> > >      finally:
> > > -        session.close()
> > > +        if session != None:
> > > +            session.close()
> > 
> > Agreed, but we can make this simply:
> > 
> > if session:
> >     session.close()
> 
> Yes,
> 
>  
> > There's no need to explicitly check for None (and if there was,
> > the preferred syntax would be 'is not None' rather than '!= None').
> > 
> > Also, just to be safe, we should make the same modification to
> > timedrift_with_reboot.py.
> 
> 
> In timedrift_with_reboot.py, 'session' has been assigned before 'try'.
> If re-login timout, kvm_test_utils.reboot() returns nothing, the value
> of 'session' isn't changed.
> So session.close() couldn't cause this problem:
> "AttributeError: 'NoneType' object has no attribute 'close'"

The two tests are nearly identical so I thought we might as well make
the change in both of them, but I agree that it doesn't matter (unless
we change the behavior of kvm_test_utils.reboot() in the future).

> 
> 
> 
> In other testcases, if session wasn't assigned before 'try',
> when calling kvm_test_utils.wait_for_login()/kvm_test_utils.reboot()
> timeout,
> It returns nothing, if close 'session' in finally part, Another
> problem will occur:
> "NameError: name 'session' is not defined"
> 
> In this condition,
> """ 
> if session:
>     session.close()
> """
> also causes this error.

In what tests exactly does this happen?

'if session' is preferable to 'if session is not None' because it's
shorter, not because it's safer.

> 
> 
> 
> > We can also consider removing the try..finally clauses altogether
> > because sessions are now closed automatically when they're no
> longer
> > needed.
> > 
> > >  
> > >      # Report results
> > >      host_delta = ht1 - ht0
> > > -- 
> > > 1.5.5.6
> 
> -- 
> Amos Kong
> Quality Engineer
> Raycom Office(Beijing), Red Hat Inc.

Re: [Autotest PATCH] KVM test: No need close session when login timeout

[Amos Kong]

On Sat, Dec 26, 2009 at 10:07:58AM -0500, Michael Goldish wrote:
> 
> ----- "Amos Kong" <akong@redhat.com> wrote:
> 
> > On Fri, Dec 25, 2009 at 08:28:18AM -0500, Michael Goldish wrote:
> > > 
> > > ----- "Amos Kong" <akong@redhat.com> wrote:
> > > 
> > > > If login timeout, wait_for() returned 'None' and assigned to
> > > > 'session'.
> > > > When call session.close(), this prlblem was caused:
> > > > "AttributeError: 'NoneType' object has no attribute 'close'"
> > > > 
> > > > Signed-off-by: Amos Kong <akong@redhat.com>
> > > > ---
> > > >  client/tests/kvm/tests/timedrift_with_migration.py |    3 ++-
> > > >  1 files changed, 2 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/client/tests/kvm/tests/timedrift_with_migration.py
> > > > b/client/tests/kvm/tests/timedrift_with_migration.py
> > > > index a012db3..0b93183 100644
> > > > --- a/client/tests/kvm/tests/timedrift_with_migration.py
> > > > +++ b/client/tests/kvm/tests/timedrift_with_migration.py
> > > > @@ -76,7 +76,8 @@ def run_timedrift_with_migration(test, params,
> > > > env):
> > > >                                               time_filter_re,
> > > > time_format)
> > > >  
> > > >      finally:
> > > > -        session.close()
> > > > +        if session != None:
> > > > +            session.close()
> > > 
> > > Agreed, but we can make this simply:
> > > 
> > > if session:
> > >     session.close()
> > 
> > Yes,
> > 
> >  
> > > There's no need to explicitly check for None (and if there was,
> > > the preferred syntax would be 'is not None' rather than '!= None').
> > > 
> > > Also, just to be safe, we should make the same modification to
> > > timedrift_with_reboot.py.
> > 
> > 
> > In timedrift_with_reboot.py, 'session' has been assigned before 'try'.
> > If re-login timout, kvm_test_utils.reboot() returns nothing, the value
> > of 'session' isn't changed.
> > So session.close() couldn't cause this problem:
> > "AttributeError: 'NoneType' object has no attribute 'close'"
> 
> The two tests are nearly identical so I thought we might as well make
> the change in both of them, but I agree that it doesn't matter (unless
> we change the behavior of kvm_test_utils.reboot() in the future).
>
> > In other testcases, if session wasn't assigned before 'try',
> > when calling kvm_test_utils.wait_for_login()/kvm_test_utils.reboot()
> > timeout,
> > It returns nothing, if close 'session' in finally part, Another
> > problem will occur:
> > "NameError: name 'session' is not defined"
> > 
> > In this condition,
> > """ 
> > if session:
> >     session.close()
> > """
> > also causes this error.
> 
> In what tests exactly does this happen?
> 
> 'if session' is preferable to 'if session is not None' because it's
> shorter, not because it's safer.

There is no this kind of test in upstream testcases,
---

I touch this problem, when I write new testcase.
For example,

def run_test_close_session(test, params, env):

    vm = kvm_test_utils.get_living_vm(env, params.get("main_vm"))
    try:
       ...
       session = kvm_test_utils.reboot(vm, session)
       ...
    finally:
        session.close()


> > > We can also consider removing the try..finally clauses altogether
> > > because sessions are now closed automatically when they're no
> > longer
> > > needed.
> > > 
> > > >  
> > > >      # Report results
> > > >      host_delta = ht1 - ht0
> > > > -- 
> > > > 1.5.5.6
> > 
> > -- 
> > Amos Kong
> > Quality Engineer
> > Raycom Office(Beijing), Red Hat Inc.

-- 
Amos Kong
Quality Engineer
Raycom Office(Beijing), Red Hat Inc.
Phone: +86-10-62608183

[Autotest PATCH] KVM test: No need close session when login timeout

[Amos Kong]

If login timeout, wait_for() returned 'None' and assigned to 'session'.
When call session.close(), this prlblem was caused:
"AttributeError: 'NoneType' object has no attribute 'close'"

Signed-off-by: Amos Kong <akong@redhat.com>
---
 client/tests/kvm/tests/timedrift_with_migration.py |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/client/tests/kvm/tests/timedrift_with_migration.py b/client/tests/kvm/tests/timedrift_with_migration.py
index a012db3..0b93183 100644
--- a/client/tests/kvm/tests/timedrift_with_migration.py
+++ b/client/tests/kvm/tests/timedrift_with_migration.py
@@ -76,7 +76,8 @@ def run_timedrift_with_migration(test, params, env):
                                              time_filter_re, time_format)
 
     finally:
-        session.close()
+        if session != None:
+            session.close()
 
     # Report results
     host_delta = ht1 - ht0
-- 
1.5.5.6