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

[Qemu-devel] [PATCH v2 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

Changed in v2:

 - Put certtool.log into the $tls_dir instead of cwd

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 v2 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.

Reviewed-by: Eric Blake <eblake@redhat.com>
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

[Qemu-devel] [PATCH v2 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..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"
 
-- 
2.20.1

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

[Eric Blake]

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.

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

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

[Max Reitz]

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

On 20.02.19 15:58, 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.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 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

I'm not sure how well the iotests currently cope with spaced dir names
anyway, but it looks weird to not use quotes here right after a line
that does.

Max

>  
>  # success, all done
>  echo "*** done"
> 



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

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

[Daniel P. Berrangé]

On Fri, Feb 22, 2019 at 04:06:32PM +0100, Max Reitz wrote:
> On 20.02.19 15:58, 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.
> > 
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > 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
> 
> I'm not sure how well the iotests currently cope with spaced dir names
> anyway, but it looks weird to not use quotes here right after a line
> that does.

Yes, that is a mistake since we tried quoting throughout the file


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 v2 1/2] iotests: ensure we print nbd server log on error

[Eric Blake]

On 2/22/19 9:16 AM, Daniel P. Berrangé wrote:
> On Fri, Feb 22, 2019 at 04:06:32PM +0100, Max Reitz wrote:
>> On 20.02.19 15:58, 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.
>>>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

>>> @@ -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
>>
>> I'm not sure how well the iotests currently cope with spaced dir names
>> anyway, but it looks weird to not use quotes here right after a line
>> that does.
> 
> Yes, that is a mistake since we tried quoting throughout the file

Can fix that while staging through my NBD queue. I'll probably send a PR
on Monday.

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

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

[Max Reitz]

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

On 22.02.19 16:54, Eric Blake wrote:
> On 2/22/19 9:16 AM, Daniel P. Berrangé wrote:
>> On Fri, Feb 22, 2019 at 04:06:32PM +0100, Max Reitz wrote:
>>> On 20.02.19 15:58, 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.
>>>>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> 
>>>> @@ -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
>>>
>>> I'm not sure how well the iotests currently cope with spaced dir names
>>> anyway, but it looks weird to not use quotes here right after a line
>>> that does.
>>
>> Yes, that is a mistake since we tried quoting throughout the file
> 
> Can fix that while staging through my NBD queue. I'll probably send a PR
> on Monday.

That's OK for me.

Max



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

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

[Daniel P. Berrangé]

On Fri, Feb 22, 2019 at 09:54:25AM -0600, Eric Blake wrote:
> On 2/22/19 9:16 AM, Daniel P. Berrangé wrote:
> > On Fri, Feb 22, 2019 at 04:06:32PM +0100, Max Reitz wrote:
> >> On 20.02.19 15:58, 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.
> >>>
> >>> Reviewed-by: Eric Blake <eblake@redhat.com>
> >>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> >>> @@ -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
> >>
> >> I'm not sure how well the iotests currently cope with spaced dir names
> >> anyway, but it looks weird to not use quotes here right after a line
> >> that does.
> > 
> > Yes, that is a mistake since we tried quoting throughout the file
> 
> Can fix that while staging through my NBD queue. I'll probably send a PR
> on Monday.

Great, thanks.

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 :|