On Tue, May 26, 2009 at 9:37 PM, David Huff <dhuff@xxxxxxxxxx> wrote:
> Michael Goldish wrote:
>> Looks good to me. See some comments below.
>>
>> ----- "David Huff" <dhuff@xxxxxxxxxx> 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...
Please ensure that we have a number of timeout categories
now.(remote_login timeout, test execution timeout, migration timeout
etc) If we wana make timeout a variable let us do it for all the
timeouts.
>
> -D
> --
> 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
>
--
Sudhir Kumar
--
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
