On Thu, Aug 27, 2015 at 08:33:43AM -0600, Eric Blake wrote:
> On 08/26/2015 09:05 AM, Daniel P. Berrange wrote:
> > Introduce a QCryptoTLSSession object that will encapsulate
> > all the code for setting up and using a client/sever TLS
> > session. This isolates the code which depends on the gnutls
> > library, avoiding #ifdefs in the rest of the codebase, as
> > well as facilitating any possible future port to other TLS
> > libraries, if desired. It makes use of the previously
> > defined QCryptoTLSCreds object to access credentials to
> > use with the session. It also includes further unit tests
> > to validate the correctness of the TLS session handshake
> > and certificate validation. This is functionally equivalent
> > to the current TLS session handling code embedded in the
> > VNC server, and will obsolete it.
> >
> > Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
> > ---
> > crypto/Makefile.objs | 1 +
> > crypto/tlssession.c | 583
> > +++++++++++++++++++++++++++++++++++++++++
> > include/crypto/tlssession.h | 322 +++++++++++++++++++++++
> > tests/.gitignore | 4 +
> > tests/Makefile | 3 +
> > tests/test-crypto-tlssession.c | 534 +++++++++++++++++++++++++++++++++++++
> > 6 files changed, 1447 insertions(+)
> > create mode 100644 crypto/tlssession.c
> > create mode 100644 include/crypto/tlssession.h
> > create mode 100644 tests/test-crypto-tlssession.c
> >
>
> > +++ b/crypto/tlssession.c
>
> > +
> > +struct _QCryptoTLSSession {
>
> Why the leading underscore before a capital? This collides with the
> namespace reserved to the compiler/library toolchain.
Just left over from my conversion from libvirt code, so will
remove that.
> > +
> > +void
> > +qcrypto_tls_session_free(QCryptoTLSSession *session)
>
> qemu coding style generally puts the return type and function name on
> the same line; but if checkpatch.pl isn't complaining, I won't insist.
> (I actually like the return type on a separate line, as emacs handles it
> nicer)
I ended up putting the return type on a separate line
because in several places it helped keep under the
80 character limit.
> > +++ b/include/crypto/tlssession.h
>
> > + * sess = qcrypto_tls_session_new(creds,
> > + * "vnc.example.com",
> > + * NULL,
> > + * QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT,
> > + * errp);
> > + * if (sess == NULL) {
> > + * return -1;
> > + * }
>
> Indentation is off
>
> > + *
> > + * qcrypto_tls_session_set_callbacks(sess,
> > + * mysock_send,
> > + * mysock_recv
> > + * GINT_TO_POINTER(fd));
> > + *
> > + * while (1) {
> > + * if (qcrypto_tls_session_handshake(sess, errp) < 0) {
> > + * qcrypto_tls_session_free(sess);
> > + * return -1;
> > + * }
> > + *
> > + * switch(qcrypto_tls_session_get_handshake_status(sess)) {
> > + * case QCRYPTO_TLS_HANDSHAKE_COMPLETE:
> > + * if (qcrypto_tls_session_check_credentials(sess, errp) < )) {
>
> Unusual indentation
Hehe, reviewing code examples in the comments is nice :-)
> > + /* We'll use this for our fake client-server connection */
> > + g_assert(socketpair(AF_UNIX, SOCK_STREAM, 0, channel) == 0);
>
> Evil to stick side-effects in a g_assert() (not as evil as doing it in
> assert(), but still something you should hoist out separately).
yep, will separate.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
