Re: [Qemu-devel] [PATCH v2 7/7] qemu-iotests: add 039 qcow2 lazy refcounts test

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]

On 07/25/2012 04:45 PM, Stefan Hajnoczi wrote:

>> Since you are assuming bash (and even if you were to assume POSIX
>> /bin/sh)...
>>
>>> +
>>> +seq=`basename $0`
>>
>> I prefer $() over ``.
>>
>>> +echo "QA output created by $seq"
>>> +
>>> +here=`pwd`
>>
>> POSIX (and therefore bash) guarantees that $PWD is sane, and faster to
>> access than $(pwd).
>>
>>> +tmp=/tmp/$$
>>
>> That's not very secure.  It may be worth using bash's $RANDOM, or using
>> mkstemp(1).
>>
>> Beyond that, the series seemed reasonable to me.
> 
> All qemu-iotests scripts do these things in the same way and I'd like
> for them to be consistent.

Good argument.

> 
> If we make these changes they should be applied to all qemu-iotests
> scripts.  I agree with your points but also think the value in making
> the change now is small.

Indeed - what you have is technically correct, even if not the most
efficient.  Any such cleanups should, as you say, be a separate patch
globally applied to the qemu-iotests, and not this test in isolation.

> 
> Do you want to send a patch that fixes these issues in qemu-iotests?

Up to you; or read another way, it bothered me enough to comment, but
not enough to write the patch myself, so I'm fine living with status quo
if it doesn't bother anyone else either.  Old-school techniques aren't
wrong, per se, just inefficient; and while the insecure temp file name
could be exploited, people running the testsuite tend to be on personal
platforms rather than enterprise systems, and the cost of exploiting a
testsuite is not as severe as the cost of exploiting an installed script.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]