* Re: [Autotest] [Autotest PATCH] KVM test: No need close session when login timeout
2009-12-26 15:07 ` [Autotest PATCH] KVM test: No need close session when login timeout Michael Goldish
@ 2009-12-27 20:28 ` Lucas Meneghel Rodrigues
2009-12-28 2:54 ` Amos Kong
2009-12-28 8:08 ` [Autotest] " Yolkfull Chow
2 siblings, 0 replies; 7+ messages in thread
From: Lucas Meneghel Rodrigues @ 2009-12-27 20:28 UTC (permalink / raw)
To: Michael Goldish; +Cc: Amos Kong, autotest, kvm
Ok, I have changed statements in both timedrift_with_migration and
timedrift_with_reboot to
if session! Thanks for your comments, Michael and Amos!
On Sat, Dec 26, 2009 at 1:07 PM, Michael Goldish <mgoldish@redhat.com> 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.
>
>>
>>
>>
>> > 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.
> _______________________________________________
> Autotest mailing list
> Autotest@test.kernel.org
> http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
>
--
Lucas
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Autotest PATCH] KVM test: No need close session when login timeout
2009-12-26 15:07 ` [Autotest PATCH] KVM test: No need close session when login timeout Michael Goldish
2009-12-27 20:28 ` [Autotest] " Lucas Meneghel Rodrigues
@ 2009-12-28 2:54 ` Amos Kong
2009-12-28 8:08 ` [Autotest] " Yolkfull Chow
2 siblings, 0 replies; 7+ messages in thread
From: Amos Kong @ 2009-12-28 2:54 UTC (permalink / raw)
To: Michael Goldish; +Cc: kvm, autotest
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Autotest] [Autotest PATCH] KVM test: No need close session when login timeout
2009-12-26 15:07 ` [Autotest PATCH] KVM test: No need close session when login timeout Michael Goldish
2009-12-27 20:28 ` [Autotest] " Lucas Meneghel Rodrigues
2009-12-28 2:54 ` Amos Kong
@ 2009-12-28 8:08 ` Yolkfull Chow
2 siblings, 0 replies; 7+ messages in thread
From: Yolkfull Chow @ 2009-12-28 8:08 UTC (permalink / raw)
To: Michael Goldish; +Cc: Amos Kong, autotest, kvm
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()
Actually we should use 'wait_for_login' instead of 'wait_for' in timedrift_with_migration.py:
session = kvm_test_utils.wait_for_login(vm, timeout=30)
And fix name of 'dest_vm', could be something like 'migrated_vm' in migrate() in kvm_test_utils.py:
...
dest_vm = vm.clone()
dest_vm.name = "migrated_vm"
dest_vm.create(for_migration=True)
...
Then this problem will never happen. :)
> >
> > 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.
> _______________________________________________
> Autotest mailing list
> Autotest@test.kernel.org
> http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
^ permalink raw reply [flat|nested] 7+ messages in thread