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 ebl...@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature