[2/2] iotests: avoid broken pipe with certtool
diff mbox series

Message ID 20190219161321.15012-3-berrange@redhat.com
State New
Headers show
Series
  • Fix NBD TLS iotests on RHEL-7
Related show

Commit Message

Daniel P. Berrangé Feb. 19, 2019, 4:13 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

Thomas Huth Feb. 19, 2019, 4:19 p.m. UTC | #1
On 19/02/2019 17.13, 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
> 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(-)
> 
> diff --git a/tests/qemu-iotests/common.tls b/tests/qemu-iotests/common.tls
> index eae81789bb..6cf11ed383 100644
> --- a/tests/qemu-iotests/common.tls
> +++ b/tests/qemu-iotests/common.tls
> @@ -29,6 +29,17 @@ tls_x509_cleanup()
>  }
>  
>  
> +tls_certtool()
> +{
> +    certtool "$@" 1>certtool.log 2>&1
> +    if test "$?" = 0; then
> +      head -1 certtool.log
> +    else
> +      cat certtool.log
> +    fi
> +    rm -f certtool.log
> +}

I assume this is running in a unique directory so that there can not be
a clash with a test running in parallel? Otherwise I'd recommend an
mktemp file name here...

Anyway, this fixes the issue for me, too, thus:

Tested-by: Thomas Huth <thuth@redhat.com>
Daniel P. Berrangé Feb. 19, 2019, 4:21 p.m. UTC | #2
On Tue, Feb 19, 2019 at 05:19:46PM +0100, Thomas Huth wrote:
> On 19/02/2019 17.13, 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
> > 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(-)
> > 
> > diff --git a/tests/qemu-iotests/common.tls b/tests/qemu-iotests/common.tls
> > index eae81789bb..6cf11ed383 100644
> > --- a/tests/qemu-iotests/common.tls
> > +++ b/tests/qemu-iotests/common.tls
> > @@ -29,6 +29,17 @@ tls_x509_cleanup()
> >  }
> >  
> >  
> > +tls_certtool()
> > +{
> > +    certtool "$@" 1>certtool.log 2>&1
> > +    if test "$?" = 0; then
> > +      head -1 certtool.log
> > +    else
> > +      cat certtool.log
> > +    fi
> > +    rm -f certtool.log
> > +}
> 
> I assume this is running in a unique directory so that there can not be
> a clash with a test running in parallel? Otherwise I'd recommend an
> mktemp file name here...

Hmm, could be a problem.  Whatever happened to the plan to make every
iotests run in a private directory ? Did that ever get done ?

> 
> Anyway, this fixes the issue for me, too, thus:
> 
> Tested-by: Thomas Huth <thuth@redhat.com>

Regards,
Daniel
Eric Blake Feb. 19, 2019, 4:37 p.m. UTC | #3
On 2/19/19 10:21 AM, Daniel P. Berrangé wrote:

>>> +tls_certtool()
>>> +{
>>> +    certtool "$@" 1>certtool.log 2>&1
>>> +    if test "$?" = 0; then
>>> +      head -1 certtool.log
>>> +    else
>>> +      cat certtool.log
>>> +    fi
>>> +    rm -f certtool.log
>>> +}
>>
>> I assume this is running in a unique directory so that there can not be
>> a clash with a test running in parallel? Otherwise I'd recommend an
>> mktemp file name here...
> 
> Hmm, could be a problem.  Whatever happened to the plan to make every
> iotests run in a private directory ? Did that ever get done ?

Nope, still not done :(

Jeff left Red Hat, and so far no one else has revived his series.
Volunteers?
Eric Blake Feb. 19, 2019, 4:42 p.m. UTC | #4
On 2/19/19 10:13 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
> 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.
> 

Better than my attempt.

> 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(-)

As Thomas pointed out, it is not parallel-safe, but that's a bigger
issue with all of the iotests, so not this patch's problem.

> 
> diff --git a/tests/qemu-iotests/common.tls b/tests/qemu-iotests/common.tls
> index eae81789bb..6cf11ed383 100644
> --- a/tests/qemu-iotests/common.tls
> +++ b/tests/qemu-iotests/common.tls
> @@ -29,6 +29,17 @@ tls_x509_cleanup()
>  }
>  
>  
> +tls_certtool()
> +{
> +    certtool "$@" 1>certtool.log 2>&1

I tend to use '>' instead of '1>', but they are identical.

> +    if test "$?" = 0; then
> +      head -1 certtool.log

Technically, POSIX says 'head -1' is not portable (it was historical
practice, but a wording change in POSIX 2001 made it invalid, which
wasn't fixed until POSIX 2008 - and there are historical versions of GNU
coreutils which went out of their way to reject the usage, although that
has since been fixed); the portable spelling these days is 'head -n1'.
But as the test was already using 'head -1', I'm not going to make you
switch it.

Reviewed-by: Eric Blake <eblake@redhat.com>
John Snow March 18, 2019, 11:16 p.m. UTC | #5
On 2/19/19 11:37 AM, Eric Blake wrote:
> On 2/19/19 10:21 AM, Daniel P. Berrangé wrote:
> 
>>>> +tls_certtool()
>>>> +{
>>>> +    certtool "$@" 1>certtool.log 2>&1
>>>> +    if test "$?" = 0; then
>>>> +      head -1 certtool.log
>>>> +    else
>>>> +      cat certtool.log
>>>> +    fi
>>>> +    rm -f certtool.log
>>>> +}
>>>
>>> I assume this is running in a unique directory so that there can not be
>>> a clash with a test running in parallel? Otherwise I'd recommend an
>>> mktemp file name here...
>>
>> Hmm, could be a problem.  Whatever happened to the plan to make every
>> iotests run in a private directory ? Did that ever get done ?
> 
> Nope, still not done :(
> 
> Jeff left Red Hat, and so far no one else has revived his series.
> Volunteers?
> 

I might work on this during the freeze as a testing enhancement.

--js

Patch
diff mbox series

diff --git a/tests/qemu-iotests/common.tls b/tests/qemu-iotests/common.tls
index eae81789bb..6cf11ed383 100644
--- a/tests/qemu-iotests/common.tls
+++ b/tests/qemu-iotests/common.tls
@@ -29,6 +29,17 @@  tls_x509_cleanup()
 }
 
 
+tls_certtool()
+{
+    certtool "$@" 1>certtool.log 2>&1
+    if test "$?" = 0; then
+      head -1 certtool.log
+    else
+      cat certtool.log
+    fi
+    rm -f 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"