Re: Emacs core TLS support

[Ted Zlatanov]

On Mon, 06 Sep 2010 00:47:39 +0200 Stefan Monnier <address@hidden> wrote: 

SM> For symmetry, I'd say "Does Emacs use -lgnutls?".

Fixed.
 
SM> Use C-u M-x checkdoc-current-buffer which will help you follow the usual
SM> coding conventions (e.g. inserting the GPL blurb).

Fixed.

>> +(defvar starttls-host nil)

SM> What is this for?  It seems to only ever be set and never read.

I think Simon Josefsson intended to do more with it but you're right,
right now it's unused.  I removed it.

>> +DEFUN ("gnutls-init", Fgnutls_init, Sgnutls_init, 2, 2, 0,
>> +       doc: /* Initializes GNU TLS for process PROC for use as 
>> CONNECTION-END.
>> +CONNECTION-END is used to indicate if this process is as a server or
>> +client. Can be one of `gnutls-client' and `gnutls-server'.  Currently
>> +only `gnutls-client' is supported.

SM> This formulation means that the symbols (rather than the value of the
SM> corresponding variables) `gnutls-client' and `gnutls-server' are the
SM> valid values.

We don't support server mode anyhow so I removed the connection-end
argument altogether (hardcoding the init to GNUTLS_CLIENT).

`gnutls-bye' still mentions `gnutls-e-again' and `gnutls-e-interrupted'.
I'm not sure if I should return a symbol or the numeric value there.

SM> This said, while I understand the general desire to just bring the C API
SM> of GNU TLS into Elisp, as long as you do it by hand, you might as well
SM> use here a Lisp boolean for connection_end.

Yeah, I'll do that for `gnutls-bye' with a CONT parameter.  It's just a
NILP check to handle booleans IIUC (there's no CHECK_BOOLEAN, so it's
either NILP or EQ to Qt).

>> +  state = (gnutls_session_t) XPROCESS(proc)->gnutls_state;
>> +  gnutls_deinit(state);

SM> Please always put a space before the open paren of a macro or
SM> function call.  Applies to the rest of the code as well, of course.

Fixed.

SM>    return make_number (lret);

Fixed everywhere, I think, and I condensed the code when possible (argh,
no C99 :)

>> +CERTFILE is a PEM encoded file.  Returns zero on success.
>> +*/)

SM> By convention we keep the closing */) at the end of the previous line.

Fixed.

>> +DEFUN ("gnutls-cred-set", Fgnutls_cred_set, 
>> +       Sgnutls_cred_set, 2, 2, 0,
>> +       doc: /* Enables GNU TLS authentication for PROCESS.
>> +TYPE is an integer indicating the type of the credentials, either
>> +`gnutls-anon', `gnutls-srp' or `gnutls-x509pki'.

SM> Again, the above formulation means that the caller should pass those
SM> symbols rather than value associated with the corresponding variables.

In this case I think that's the right approach actually so we shouldn't
have defconsts.  See new definition, it uses two local Lisp_Objects for
the symbol names.  Where should I allocate those constant Lisp_Objects
globally properly?  And should I default to anonymous?

>> +      if (gnutls_certificate_allocate_credentials (&x509_cred) < 0)
>> +    memory_full ();

SM> Can it really only mean "memory is full"?

I think so.

>> === added file 'src/gnutls.h'
SM> Why add this file?  Doesn't seem worth the trouble.

As gnutls.c grows, I think it will be necessary.  It can be removed now
if you want.

>> +#ifdef HAVE_GNUTLS
>> +    /* XXX Store GNU TLS state and auth mechanisms in Lisp_Objects. */
>> +    Lisp_Object gnutls_state;
>> +    Lisp_Object x509_cred, x509_callback;
>> +    Lisp_Object anon_cred;
>> +    Lisp_Object srp_cred;
>> +#endif

SM> Rather than hardcode variables in gnutls.el, an alternative could be to
SM> define those variables in gnutls.c so you can initialize them to the
SM> values taken from gnutls/gnutls.h.

I'd like to take the direction of a more Lisp-y API on top of the GnuTLS
API.  So any constants should be limited to the function bodies and I'd
like to stick to symbols (as with gnutls-cred-set in the new patch).

On Sun, 05 Sep 2010 10:06:09 +0200 Andreas Schwab <address@hidden> wrote: 

AS> You should remove the debugging output.

Fixed.

>> +DEFUN ("gnutls-init", Fgnutls_init, Sgnutls_init, 2, 2, 0,
...
>> +  ret = gnutls_init((gnutls_session_t*)&(XPROCESS(proc)->gnutls_state), 

AS> Aliasing violation.

Can you explain please?

AS> IMHO all your functions should return t on success and either some error
AS> symbol on failure or even raise an error.

Yes, but I'm not sure which one.  Can you recommend?

AS> No C99.

Yes, sorry for leaving that in the patch.  Removed.

>> === modified file 'src/process.h'
>> +
>> +#ifdef HAVE_GNUTLS
>> +    /* XXX Store GNU TLS state and auth mechanisms in Lisp_Objects. */
>> +    Lisp_Object gnutls_state;
>> +    Lisp_Object x509_cred, x509_callback;
>> +    Lisp_Object anon_cred;
>> +    Lisp_Object srp_cred;
>> +#endif

AS> None of them should be Lisp_Objects.  Also make sure the resources are
AS> properly released when the process object is deleted.

I don't know enough (the choice of using Lisp_Objects was in the
original patch) to know what to do instead of using Lisp_Objects.  Why
not, first of all?

Revised patch is attached (it compiles but the changes are mostly
cosmetic so it's still not usable).  Thank you for the comments, they
were very helpful.  I hope to get this in a usable state ASAP.

Ted

tls.patch
Description: Text Data