Re: [Nbd] [PATCHv2 6/6] Add TLS testing to nbd-tester-client.c

[Alex Bligh <alex@...872...>]

On 12 Apr 2016, at 15:15, Wouter Verhelst <w@...112...> wrote:

> On Mon, Apr 11, 2016 at 06:15:39PM +0100, Alex Bligh wrote:
>> This commit adds TLS testing to nbd-tester-client and 'make check'.
>> If TLS is not compiled in, then the test is skipped.
> 
> Alternatively, it could check that nbd-server produces an error in that
> case.

That's quite difficult to do per se as simpletest would need to
read config.h or similar. However, in practice it *is* doing this
as nbd-tester-client.c only exits with the magic code 77 if
one of those options is specified. If it didn't exit with that
code, it would either produce a failure from an immediate exit with
-1, or it would fail as TLS isn't enabled. Unless the failure is
accidentally building in entirely functional TLS support of course,
but simpletest's reading of config.h could be fooled the same way.

>> Signed-off-by: Alex Bligh <alex@...872...>
>> ---
>> nbd.h                           |   2 +
>> tests/run/Makefile.am           |  13 +++-
>> tests/run/certs/ca-cert.pem     |  20 +++++
>> tests/run/certs/ca-key.pem      |  32 ++++++++
>> tests/run/certs/ca.info         |   3 +
>> tests/run/certs/client-cert.pem |  23 ++++++
>> tests/run/certs/client-key.pem  |  32 ++++++++
>> tests/run/certs/client.info     |   8 ++
>> tests/run/certs/server-cert.pem |  22 ++++++
>> tests/run/certs/server-key.pem  |  32 ++++++++
>> tests/run/certs/server.info     |   5 ++
> 
> Might be good to add a little README there to explain what each of the
> files does.

Will do for v4.

>>  * This is the packet used for communication between client and
>>  * server. All data are in network byte order.
>> diff --git a/tests/run/Makefile.am b/tests/run/Makefile.am
>> index d1e28ed..050b51d 100644
>> --- a/tests/run/Makefile.am
>> +++ b/tests/run/Makefile.am
>> @@ -1,11 +1,16 @@
>> +if GNUTLS
>> +TLSSRC = $(top_srcdir)/crypto-gnutls.c $(top_srcdir)/crypto-gnutls.h $(top_srcdir)/buffer.c $(top_srcdir)/buffer.h
>> +else
>> +TLSSRC =
>> +endif
> 
> Same here as in the main Makefile.am, obviously.

Done already in v3.

>> 
>> +/**
>> + * Set a socket to blocking or non-blocking
>> + *
>> + * @param fd The socket's FD
>> + * @param nb non-zero to set to non-blocking, else 0 to set to blocking
>> + * @return 0 - OK, -1 failed
>> + */
>> +int set_nonblocking(int fd, int nb) {
>> +        int sf = fcntl (fd, F_GETFL, 0);
>> +        if (sf == -1)
>> +                return -1;
>> +        return fcntl (fd, F_SETFL, nb ? (sf | O_NONBLOCK) : (sf & ~O_NONBLOCK));
>> +}
> 
> Maybe this should be moved to cliserv.[ch]?

Will do for v4.

> [...]
>> diff --git a/tests/run/simple_test b/tests/run/simple_test
>> index 0c05ea1..80b99dc 100755
>> --- a/tests/run/simple_test
>> +++ b/tests/run/simple_test
>> @@ -284,6 +284,51 @@ EOF
>> 		./nbd-tester-client -N export1 -u ${tmpdir}/unix.sock
>> 		retval=$?
>> 		;;
>> +	*/tls)
>> +		# TLS test
>> +		certdir=`pwd`/certs
> 
> I prefer $()-style command substitution over backticks.

Will do for v4.

> [...]
>> +		cat >${conffile} <<EOF
>> +[generic]
>> +	certfile = $certdir/server-cert.pem
>> +        keyfile = $certdir/server-key.pem
>> +        cacertfile = $certdir/ca-cert.pem
> 
> More whitespace mishaps (and that happens again for tlshuge, too)

Will so. Not sure what the standard for that file is, but whatever it is
it's not emacs' default.

-- 
Alex Bligh