Re: [KVM-AUTOTEST][PATCH] timedrift support

[Lucas Meneghel Rodrigues <mrodrigu@redhat.com>]

On Tue, 2009-05-12 at 21:07 +0800, Bear Yang wrote:
> Sorry forgot to attach my new patch.
> Bear Yang wrote:
> > Hi Lucas:
> > First, I want to say really thanks for your kindly,carefully words and 
> > suggestions. now,  I modified my scripts follow your opinions.
> > 1. Add the genload to timedrift, but I am not sure whether it is right 
> > or not to add the information  CVS relevant. If it is not necessary. I 
> > will remove them next time.

Yes, we can remove the CVS related info, they just got mailed to you
because I got the code from a fresh LTP CVS checkout!

> > 2. Replace the API os.system to utils.system
> > 3. Replace the API os.environ.get('HOSTNAME') to socket.gethostname()
> > 4. for the snippet of the code below:
> > +    if utils.system(ntp_cmd, ignore_status=True) != 0:
> > +        raise error.TestFail, "NTP server has not starting correctly..."
> >
> > Your suggestion is "Instead of the if clause we'd put a try/except 
> > block", but I am not clear how to do it. Would you please give me some 
> > guides for this. Sorry.

You could re-write the above if statement using the form:

try:
    utils.system(ntp_cmd)
except:
    raise error.TestFail("NTP server has not started correctly")

Some comments:

 1) The try/except block works because utils.system already throws an
exception when the exit code is different from 0.
 2) The form 

raise error.TestFail("NTP server has not started correctly")

Is preferred on the upstream project over the equivalent

raise error.TestFail, "NTP server has not started correctly"

But on kvm autotest we are adopting the later, so don't worry and keep
the all the raises the way they are on your original patch. This was
just a side comment.

> > Other thing about functional the clauses which to get vm handle below:
> >
> > +    # get vm handle
> > +    vm = kvm_utils.env_get_vm(env,params.get("main_vm"))
> > +    if not vm:
> > +        raise error.TestError, "VM object not found in environment"
> > +    if not vm.is_alive():
> > +        raise error.TestError, "VM seems to be dead; Test requires a 
> > living VM"
> >
> > I agree with you on this point, I remember that somebody to do this 
> > before. but seems upstream not accept his modification.

Ok, will take a look at this. By the way, when you have an updated patch
please let us know!

Thank you very much,

-- 
Lucas Meneghel Rodrigues
Software Engineer (QE)
Red Hat - Emerging Technologies