From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lucas Meneghel Rodrigues Subject: Re: [KVM-AUTOTEST][PATCH] timedrift support Date: Wed, 13 May 2009 15:34:55 -0300 Message-ID: <1242239695.2620.18.camel@localhost.localdomain> References: <4A010BCD.8060307@redhat.com> <20090506130247.GA5048@amt.cnet> <4A08008E.8060105@redhat.com> <1242046789.2930.8.camel@localhost.localdomain> <4A096C28.1060900@redhat.com> <4A09748C.6040909@redhat.com> Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , uril@redhat.com, kvm@vger.kernel.org To: Bear Yang Return-path: Received: from mx2.redhat.com ([66.187.237.31]:52019 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757006AbZEMSe5 (ORCPT ); Wed, 13 May 2009 14:34:57 -0400 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n4DIYw1U014636 for ; Wed, 13 May 2009 14:34:58 -0400 In-Reply-To: <4A09748C.6040909@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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