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

[Avi Kivity <avi@redhat.com>]

Michael Goldish wrote:
>> I think it's very rare to want to let the test continue even if some 
>> command fails.
>>
>> Can you give examples?
>>     
>
> Some commands are not critical, like one that converts the screendumps
> from PPM to PNG format. We don't want to fail the test if you don't have
> ImageMagick installed.
>   

I think you don't want to run the test at all in that case.  But if you 
do, you shouldn't invoke ImageMagick if you don't have it installed.  
Otherwise you can't distinguish between failure due to a corrupted 
source image, out of disk space, or ImageMagick not installed.

Ignoring errors is usually a bug.

> A similar function used for remote commands is sometimes used for checking
> if a file exists on the guest and getting its size at the same time,
> using ls. If the file doesn't exist we should SCP it into the guest, not
> fail the test.
>   

Again it doesn't distinguish between real errors and 'file does not 
exist'.  Use something like 'if test -f file; then ls -l file; else echo 
missing; fi'.

You can use rsync to implement 'copy unless exists'.

>>
>>   try:
>>      code
>>      code
>>      code
>>   except e:
>>      raise ChainedException('exception while running migration test',
>> e)
>>
>> Instead of checking each code line, you provide a wrapper for the
>> entire test.
>>     
>
> It's not informative to add it for the entire test, because we already know
> what test failed -- we don't need the exception string for that.
>
> Instead, we'd have to wrap small sections of the test separately and provide
> a different exception string for each. The migration test may be split to
> four sections: setup, pre-migration, the migration itself and post-migration.
> This may not be so bad, but do you still think it's a good solution?
>   

You can have a 'stage' variable and assign it your current stage, and 
use it when reporting errors.

Anything is better than a maze of ifs.

> There's also another issue: the utility functions that should raise exceptions
> don't know what they're being used for, so there can be exception string
> collisions. For example, both run_bg(), used for local processes, and
> kvm_spawn.get_command_status_output(), used for SSH commands, would say
> something like "command %s failed". It would be incorrect to hardcode a more
> specific message into any of them (in my opinion).
>
>   

The message should explain what happened, no more, no less.

>> The caller has to test for three possible outcomes.  Success, failure,
>> and 'command is running in the background'.  If your callsites don't 
>> check for all three, something's wrong.
>>     
>
> When we want the command to succeed on time, we just test for status == 0.
> If it's a nonzero integer or None, something went wrong, and we should kill
> the process just in case it's still running.
>   

But if it's nonzero it has exited already!

However, I feel I'm not making any progress, so I'll stop.  I just wish 
I could use exceptions in the code I write.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.