Re: [Qemu-devel] [PATCH 1/7] iotests: Allow out-of-tree run

[Max Reitz <mreitz@redhat.com>]

On 16.05.2014 00:52, Eric Blake wrote:
> On 05/15/2014 04:26 PM, Max Reitz wrote:
>> As out-of-tree builds are preferred for qemu, running the qemu-iotests
>> in that out-of-tree build should be supported as well. To do so, a
>> symbolic link has to be created pointing to the check script in the
>> source directory. That script will check whether it has been run through
>> a symlink, and if so, will assume it is run in the build tree. All
>> output and temporary operations performed by iotests are then redirected
>> here and, unless specified otherwise by the user, QEMU_PROG etc. will be
>> set to paths appropriate for the build tree.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/check         | 78 ++++++++++++++++++++++++++++++++++------
>>   
>> +if [ -L "$0" ]
>> +then
>> +    # called from the build tree
>> +    source_iotests="$(cd "$(dirname "$(readlink "$0")")"; pwd)"
> This is potentially dangerous. If readlink or dirname fails, you can
> invoke cd "" (which on bash is stupidly a no-op instead of an error),
> and end up calling pwd in the wrong directory.  But in the common case
> it works, so I'm not sure it's worth bending over backwards to make it
> more robust.

I guess using something like

source_iotests="$(dirname "$(readlink "$0")")"; if [ -z 
"$source_iotests" ]; then; /* abort */; fi; source_iotests="$(cd 
"$dirname"; pwd)"

should work better, then?

>> +        if [ -n "$arch" -a -x "$build_root/$arch-softmmu/qemu-system-$arch" ]
> -a and -o are NOT portable inside []; POSIX strongly discourages their
> use because they can cause ambiguous parses:

Hm, I remembered you telling me something about -a before and looked in 
test's manpage but couldn't find anything bad. Well, it's the GNU manpage…

> Is [ ! '(' -o ')' ] true or false? Depends on whether it was parsed as {
> ! '(' } -o ')' (false -o true => true) or as ! { '(' -o ')' } (! (true
> -o true) => false)
>
> But this is bash, so you could do:
>
> if [[ $arch && -x $build_root/$arch-softmmu/qemu-system-$arch ]]
>
> for less typing, and no risk of [] ambiguity.

If you're telling me I'm free to use bashisms, I'll believe you. :-)

>> +++ b/tests/qemu-iotests/common.rc
>> @@ -318,9 +318,9 @@ _do()
>>           status=1; exit
>>       fi
>>   
>> -    (eval "echo '---' \"$_cmd\"") >>$here/$seq.full
>> +    (eval "echo '---' \"$_cmd\"") >>"$OUTPUT_DIR/$seq.full"
>>       (eval "$_cmd") >$tmp._out 2>&1; ret=$?
> Pre-existing, but we're using 'eval'?  That's probably a security risk
> if $_cmd can contain user-controlled text, such as an odd directory name.

Actually, I don't even see that function ("_do") being used anywhere in 
the iotests. We could probably drop it.

But the eval should be of no harm, as it is the intention of this 
function "_do" to execute the function given as a parameter. If anyone 
should make sure, this eval does not execute user-controlled text, it is 
the code using _do, I think.

Max