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

[Avi Kivity <avi@redhat.com>]

Michael Goldish wrote:
> ----- "Avi Kivity" <avi@redhat.com> wrote:
>
>   
>> 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.
>>>  
>>> +   #execute any pre_commands
>>> +    pre_command = params.get("pre_command")
>>> +    if pre_command:
>>> +        # export environment vars
>>> +        for k in params.keys():
>>> +            kvm_log.info("Adding KVM_TEST_%s to Environment" %
>>>       
>> (k))
>>     
>>> +            os.putenv("KVM_TEST_%s" % (k), str(params[k]))
>>> +        # execute command
>>> +        kvm_log.info("Executing command '%s'..." % pre_command)
>>> +        timeout = int(params.get("pre_commmand_timeout", "600"))
>>> +        (status, pid, output) = kvm_utils.run_bg("cd %s; %s" %
>>>       
>> (test.bindir, pre_command),
>>     
>>> +                                                 None,
>>>       
>> kvm_log.debug, "(pre_command) ", timeout=timeout)
>>     
>>> +        if status != 0:
>>> +            kvm_utils.safe_kill(pid, signal.SIGTERM)
>>> +            raise error.TestError, "Custom processing pre_command
>>>       
>> failed"
>>     
>>>   
>>>       
>> kvm_utils.run_bg should throw an exception instead of returning
>> status.
>>     
>
> - What if we're interested in the status for some reason? Its value
> may indicate what went wrong with the child process.
>   
Put it in the exception string.

> - If we throw an exception we should add a parameter that controls
> whether an exception should be thrown (something like "ignore_status")
> because in some cases we don't want to throw an exception.
>   

I'll complain every time I see it.  If you don't care if the command 
succeeds or not, why run it in the first place?

> - A run_bg call has 3 possible outcomes: success (status == 0), failure
> (status != 0) and timeout (process still running). If we throw an exception
> upon failure, what do we do upon timeout? Sometimes it's good (qemu should
> keep running unless something went wrong) and sometimes bad (pre_command
> should probably not time out). So we end up needing an ignore_timeout
> parameter as well.
>   

A run_bg() call should return an object with a .join() method, or throw 
an exception.  At some point you must call the join() method, which 
throws an exception or returns None.

> - What if we want the test to fail with an informative test-specific
> exception such as "something failed after migration"?
>   

Chained exceptions can provide detailed information.

>> But if status != 0, will there actually be a pid to kill?
>>     
>
> If the timeout expires and the process is still running, status is None.
>   

functions which can return with three possible outcomes are difficult to 
use.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.