Re: [Qemu-devel] [PATCH v5 7/9] crypto: introduce new module for handling TLS sessions

[Daniel P. Berrange]

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 <address@hidden>
> > ---
> >  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 :|