diff mbox series

[v2,2/2] iotests: avoid broken pipe with certtool

Message ID 20190220145819.30969-3-berrange@redhat.com
State New
Headers show
Series Fix NBD TLS iotests on RHEL-7 | expand

Commit Message

Daniel P. Berrangé Feb. 20, 2019, 2:58 p.m. UTC 
When we run "certtool | head -1" the latter command is likely to
complete and exit before certtool has written everything it wants to
stderr. In at least the RHEL-7 gnutls 3.3.29 this causes certtool to
quit with broken pipe before it has finished writing the desired
output file to disk. This causes non-deterministic failures of the
iotest 233 because the certs are sometimes zero length files.
If certtool fails the "head -1" means we also loose any useful error
message it would have printed.

Thus this patch gets rid of the pipe and post-processes the output in a
more flexible & reliable manner.

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qemu-iotests/common.tls | 48 +++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 16 deletions(-)

Comments

Eric Blake Feb. 20, 2019, 4:11 p.m. UTC | #1 
On 2/20/19 8:58 AM, Daniel P. Berrangé wrote:
> When we run "certtool | head -1" the latter command is likely to
> complete and exit before certtool has written everything it wants to
> stderr. In at least the RHEL-7 gnutls 3.3.29 this causes certtool to

A bit of a mismatch: had we actually used only 'certtool | head -1', it
would be early death before writing all it wants to stdout (not stderr).
 But since the patch itself is replacing 'certtool 2>&1 | head -1',
where stderr is also in the picture due to the additional fd
redirection, I'm not sure if it is better to just s/"certtool
|/"certtool 2>&1 |/ to match the patch, or to s/stderr/stdout/ for
brevity, or to just leave things as written.  Your call.

> quit with broken pipe before it has finished writing the desired
> output file to disk. This causes non-deterministic failures of the
> iotest 233 because the certs are sometimes zero length files.
> If certtool fails the "head -1" means we also loose any useful error

s/loose/lose/

> message it would have printed.
> 
> Thus this patch gets rid of the pipe and post-processes the output in a
> more flexible & reliable manner.
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  tests/qemu-iotests/common.tls | 48 +++++++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 16 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

As the fix is for an iotest using NBD, I can take this through my tree
if no one else picks it up through some other block or iotests tree first.
diff mbox series

Patch

diff --git a/tests/qemu-iotests/common.tls b/tests/qemu-iotests/common.tls
index eae81789bb..3caf989d28 100644
--- a/tests/qemu-iotests/common.tls
+++ b/tests/qemu-iotests/common.tls
@@ -29,6 +29,17 @@  tls_x509_cleanup()
 }
 
 
+tls_certtool()
+{
+    certtool "$@" 1>"${tls_dir}"/certtool.log 2>&1
+    if test "$?" = 0; then
+      head -1 "${tls_dir}"/certtool.log
+    else
+      cat "${tls_dir}"/certtool.log
+    fi
+    rm -f "${tls_dir}"/certtool.log
+}
+
 tls_x509_init()
 {
     (certtool --help) >/dev/null 2>&1 || \
@@ -71,10 +82,11 @@  ca
 cert_signing_key
 EOF
 
-    certtool --generate-self-signed \
-             --load-privkey "${tls_dir}/key.pem" \
-             --template "${tls_dir}/ca.info" \
-             --outfile "${tls_dir}/$name-cert.pem" 2>&1 | head -1
+    tls_certtool \
+        --generate-self-signed \
+        --load-privkey "${tls_dir}/key.pem" \
+        --template "${tls_dir}/ca.info" \
+        --outfile "${tls_dir}/$name-cert.pem"
 
     rm -f "${tls_dir}/ca.info"
 }
@@ -98,12 +110,14 @@  encryption_key
 signing_key
 EOF
 
-    certtool --generate-certificate \
-             --load-ca-privkey "${tls_dir}/key.pem" \
-             --load-ca-certificate "${tls_dir}/$caname-cert.pem" \
-             --load-privkey "${tls_dir}/key.pem" \
-             --template "${tls_dir}/cert.info" \
-             --outfile "${tls_dir}/$name/server-cert.pem" 2>&1 | head -1
+    tls_certtool \
+        --generate-certificate \
+        --load-ca-privkey "${tls_dir}/key.pem" \
+        --load-ca-certificate "${tls_dir}/$caname-cert.pem" \
+        --load-privkey "${tls_dir}/key.pem" \
+        --template "${tls_dir}/cert.info" \
+        --outfile "${tls_dir}/$name/server-cert.pem"
+
     ln -s "${tls_dir}/$caname-cert.pem" "${tls_dir}/$name/ca-cert.pem"
     ln -s "${tls_dir}/key.pem" "${tls_dir}/$name/server-key.pem"
 
@@ -127,12 +141,14 @@  encryption_key
 signing_key
 EOF
 
-    certtool --generate-certificate \
-             --load-ca-privkey "${tls_dir}/key.pem" \
-             --load-ca-certificate "${tls_dir}/$caname-cert.pem" \
-             --load-privkey "${tls_dir}/key.pem" \
-             --template "${tls_dir}/cert.info" \
-             --outfile "${tls_dir}/$name/client-cert.pem" 2>&1 | head -1
+    tls_certtool \
+        --generate-certificate \
+        --load-ca-privkey "${tls_dir}/key.pem" \
+        --load-ca-certificate "${tls_dir}/$caname-cert.pem" \
+        --load-privkey "${tls_dir}/key.pem" \
+        --template "${tls_dir}/cert.info" \
+        --outfile "${tls_dir}/$name/client-cert.pem"
+
     ln -s "${tls_dir}/$caname-cert.pem" "${tls_dir}/$name/ca-cert.pem"
     ln -s "${tls_dir}/key.pem" "${tls_dir}/$name/client-key.pem"