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@xxxxxxxxxx> wrote: > > ----- "Amos Kong" <akong@xxxxxxxxxx> wrote: > >> On Fri, Dec 25, 2009 at 08:28:18AM -0500, Michael Goldish wrote: >> > >> > ----- "Amos Kong" <akong@xxxxxxxxxx> 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@xxxxxxxxxx> >> > > --- >> > > 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@xxxxxxxxxxxxxxx > http://test.kernel.org/cgi-bin/mailman/listinfo/autotest > -- Lucas -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
