From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Huff Subject: Re: [PATCH] [KVM_Autotest] Added functionality to the preprocessor to run scripts Date: Tue, 26 May 2009 12:07:34 -0400 Message-ID: <4A1C13C6.4020603@redhat.com> References: <1844402991.226901242993492364.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org To: Michael Goldish Return-path: Received: from mx2.redhat.com ([66.187.237.31]:56368 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753995AbZEZQIN (ORCPT ); Tue, 26 May 2009 12:08:13 -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 n4QG8FHT008615 for ; Tue, 26 May 2009 12:08:15 -0400 In-Reply-To: <1844402991.226901242993492364.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Michael Goldish wrote: > Looks good to me. See some comments below. > > ----- "David Huff" wrote: > >> This patch will run pre and post scripts >> defined in config file with the parameter pre_command >> and post_command post_command. >> >> Also exports all the prameters in preprocess for passing >> arguments to the script. > > Why not do this for post_command as well? I didn't do post_command b/c I figured that they would already be exported in the pre_command, however I guess that there can be the case where there is no pre and only post and I guess exporting twice will not hurt anything.... I will add this to the patch. > >> --- >> client/tests/kvm_runtest_2/kvm_preprocessing.py | 31 >> +++++++++++++++++++++- >> 1 files changed, 29 insertions(+), 2 deletions(-) >> >> diff --git a/client/tests/kvm_runtest_2/kvm_preprocessing.py >> b/client/tests/kvm_runtest_2/kvm_preprocessing.py >> index c9eb35d..02df615 100644 >> --- a/client/tests/kvm_runtest_2/kvm_preprocessing.py >> +++ b/client/tests/kvm_runtest_2/kvm_preprocessing.py >> @@ -135,8 +135,7 @@ def postprocess_vm(test, params, env, name): >> "Waiting for VM to kill itself..."): >> kvm_log.debug("'kill_vm' specified; killing VM...") >> vm.destroy(gracefully = params.get("kill_vm_gracefully") == >> "yes") >> - >> - >> + > > I hate to be petty, but we usually keep two blank lines between top > level functions. > > Also, you have some trailing whitespace there... good catch, I will take care of this > It wouldn't hurt to make this timeout user-configurable with a default > value of 600 or so: > > timeout = int(params.get("pre_commmand_timeout", "600")) > (status, pid, output) = kvm_utils.run_bg(..., timeout=timeout) > > We can also do that in a separate patch. > I'll go ahead and add this while I rework the patch... -D