Re: [PATCH] [KVM_Autotest] Added functionality to the preprocessor to run scripts

[David Huff <dhuff@redhat.com>]

Michael Goldish wrote:
> Looks good to me. See some comments below.
> 
> ----- "David Huff" <dhuff@redhat.com> 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