----- "Yolkfull Chow" <yzhou@xxxxxxxxxx> wrote:
> On Sat, Dec 26, 2009 at 10:07:58AM -0500, Michael Goldish 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()
>
> 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)
I think we should use wait_for() because we want custom error messages
here ("Could not log in after migration").
Alternatively we can make wait_for_login() accept a parameter that
controls the error message, so the call would look like:
session = kvm_test_utils.wait_for_login(vm, "Could not log in after migration", timeout=30)
but unless we do this, we should keep using wait_for() where custom error
messages are required.
> 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. :)
What problem is solved by this?
This may not be a good idea because dest_vm gets registered in env with
the name of the source VM (e.g. "vm1"), which means it replaces the
source VM. I'm not sure it's a good idea to name dest_vm "migrated_vm"
while registering it as "vm1". It may cause unexpected little problems.
Anyway, what reason do we have to do this?
>
> > >
> > > 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
> --
> 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
--
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
