| Message ID | 20190219161321.15012-3-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix NBD TLS iotests on RHEL-7 | expand |
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>
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
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?
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>
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
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"
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(-)