[Qemu-devel] [PATCH 0/2] Fix NBD TLS iotests on RHEL-7

[Qemu-devel] [PATCH 0/2] Fix NBD TLS iotests on RHEL-7

[Daniel P. Berrangé]

This fixes a failure of iotest 233 due to certtool problesm in RHEL7 wrt
to SIGPIPE

Daniel P. Berrangé (2):
  iotests: ensure we print nbd server log on error
  iotests: avoid broken pipe with certtool

 tests/qemu-iotests/233        |  3 +++
 tests/qemu-iotests/common.tls | 48 +++++++++++++++++++++++------------
 2 files changed, 35 insertions(+), 16 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/2] iotests: ensure we print nbd server log on error

[Daniel P. Berrangé]

If we abort the iotest early the server.log file might contain useful
information for diagnosing the problem. Ensure its contents are
displayed in this case.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qemu-iotests/233 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/233 b/tests/qemu-iotests/233
index fc345a1a46..27932df075 100755
--- a/tests/qemu-iotests/233
+++ b/tests/qemu-iotests/233
@@ -30,6 +30,8 @@ _cleanup()
 {
     nbd_server_stop
     _cleanup_test_img
+    # If we aborted early we want to see this log for diagnosis
+    test -f "$TEST_DIR/server.log" && cat "$TEST_DIR/server.log"
     rm -f "$TEST_DIR/server.log"
     tls_x509_cleanup
 }
@@ -120,6 +122,7 @@ $QEMU_IO -f $IMGFMT -r -U -c 'r -P 0x22 1m 1m' "$TEST_IMG" | _filter_qemu_io
 echo
 echo "== final server log =="
 cat "$TEST_DIR/server.log"
+rm -f $TEST_DIR/server.log
 
 # success, all done
 echo "*** done"
-- 
2.20.1

Re: [Qemu-devel] [PATCH 1/2] iotests: ensure we print nbd server log on error

[Eric Blake]

On 2/19/19 10:13 AM, Daniel P. Berrangé wrote:
> If we abort the iotest early the server.log file might contain useful
> information for diagnosing the problem. Ensure its contents are
> displayed in this case.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  tests/qemu-iotests/233 | 3 +++
>  1 file changed, 3 insertions(+)

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

> 
> diff --git a/tests/qemu-iotests/233 b/tests/qemu-iotests/233
> index fc345a1a46..27932df075 100755
> --- a/tests/qemu-iotests/233
> +++ b/tests/qemu-iotests/233
> @@ -30,6 +30,8 @@ _cleanup()
>  {
>      nbd_server_stop
>      _cleanup_test_img
> +    # If we aborted early we want to see this log for diagnosis
> +    test -f "$TEST_DIR/server.log" && cat "$TEST_DIR/server.log"
>      rm -f "$TEST_DIR/server.log"
>      tls_x509_cleanup
>  }
> @@ -120,6 +122,7 @@ $QEMU_IO -f $IMGFMT -r -U -c 'r -P 0x22 1m 1m' "$TEST_IMG" | _filter_qemu_io
>  echo
>  echo "== final server log =="
>  cat "$TEST_DIR/server.log"
> +rm -f $TEST_DIR/server.log
>  
>  # success, all done
>  echo "*** done"
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[Qemu-devel] [PATCH 2/2] iotests: avoid broken pipe with certtool

[Daniel P. Berrangé]

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
+}
+
 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"
 
-- 
2.20.1

Re: [Qemu-devel] [PATCH 2/2] iotests: avoid broken pipe with certtool

[Thomas Huth]

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>

Re: [Qemu-devel] [PATCH 2/2] iotests: avoid broken pipe with certtool

[Daniel P. Berrangé]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 2/2] iotests: avoid broken pipe with certtool

[Eric Blake]

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, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 2/2] iotests: avoid broken pipe with certtool

[Eric Blake]

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>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org