On 12 Apr 2016, at 15:15, Wouter Verhelst <[email protected]> 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 <[email protected]>
>> ---
>> 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





------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Nbd-general mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/nbd-general

Reply via email to