Re: [Qemu-devel] [PATCH 5/6] tests: add iotests helpers for dealing with TLS certificates

[Max Reitz <mreitz@redhat.com>]

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

On 19.11.18 11:27, Daniel P. Berrangé wrote:
> On Fri, Nov 16, 2018 at 10:39:03AM -0600, Eric Blake wrote:
>> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
>>> Add helpers to common.tls for creating TLS certificates for a CA,
>>> server and client.
>>
>> MUCH appreciated!  We NEED this coverage, easily automated.
>>
>>>
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>>   tests/qemu-iotests/common.tls | 139 ++++++++++++++++++++++++++++++++++
>>>   1 file changed, 139 insertions(+)
>>>   create mode 100644 tests/qemu-iotests/common.tls
>>>
>>> diff --git a/tests/qemu-iotests/common.tls b/tests/qemu-iotests/common.tls
>>> new file mode 100644
>>
>> I was a bit surprised that this wasn't 100755, but this matches the fact
>> that none of the other common.* are executable. And after thinking more, it
>> makes sense - they aren't standalone scripts, but designed to be sourced,
>> and 'source' doesn't care about execute bits.
>>
>>> +tls_dir="${TEST_DIR}/tls"
>>> +
>>> +function tls_x509_cleanup()
>>> +{
>>> +    rm -f ${tls_dir}/*.pem
>>> +    rm -f ${tls_dir}/*/*.pem
>>> +    rmdir ${tls_dir}/*
>>> +    rmdir ${tls_dir}
>>
>> Why not just:
>> rm -rf $tls_dir
> 
> Yeah, I guess we could do that for simplicity
> 
>> Also, the quoting is a bit inconsistent. if ${TEST_DIR} can contain spaces,
>> then all uses of ${tls_dir} need to be in "".
> 
> Hmm, yes.

Which by the way is a very good reason *not* to blindly use "rm -r".

So far we only seem to have one instance of "rm -r" in the iotests (and
that is on three files, so I don't even know why that has -r), and I'm
glad about that.

I prefer for scripts to only delete what they have created and not
blindly delete something.  Wildcards are already kind of pushing it.

Maybe the user has created the tls dir beforehand, then I'd prefer for
the iotests not to just delete it and everything in it.  But the worst
of course would be if we get escaping wrong somewhere (as demonstrated
here) and suddenly we delete a completely unrelated directory by
accident.  Anyone remember Steam's 'rm -rf "$STEAMROOT"/*'?

Everyone knows they have to be careful with deleting things, but most of
the time it is a bother if you're in an interactive shell and know your
directory structure and all the arguments you're passing perfectly well.
 But a script doesn't know either, and "it's a bother" is not really an
argument if you have to write the code just once.

Max


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