Re: Emacs core TLS support

[Ted Zlatanov]

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

>> 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?

SM> It's typically declared as global (static or not, depending on whether
SM> it's used in other files) and initialized in syms_of_<foo>.
SM> Look at other syms_of_<foo> to see what it looks like.

Done, thanks.

>> And should I default to anonymous?

SM> I don't know what that means.

If the user passes an unknown symbol to `gnutls-cred-set', should it be
treated as `gnutls_anon' or generate an error?  It could work either
way.  I'm leaning towards an error but it seems kind of rude to the
user.  OTOH it could be a serious problem to use encryption the user did
not intend because of a typo.

SM> the slots you add at the end are ignored by the GC (which is what
SM> you want in your case, since they're not Lisp objects and hence the
SM> GC wouldn't know what to do with them) and can be of any type.  So
SM> just use the types you need here such that casts aren't needed.

OK.  I introduced a new field `gnutls_state_usable' to indicate the
session has been initialized.  I could have made it a byte but it may be
useful to hold Lisp-related state for this patch as it evolves.  It's
before the GC marker field "pid" so it will be noticed by alloc.c.

SM> BTW, if it makes the code simpler, you can use the following trick: use
SM> symbols, but associate an integer to each symbol by way of
SM> symbol properties.

I don't like the properties because they are loosely bound to the symbol
(for errors I think it's better to bind meaning to value tightly).  Is
it OK to do the current approach, where I have the function
`gnutls_make_error' to return the right thing, whether it's a known
integer-as-symbol or a generic integer?  I think it's the right approach
and it seems semantically sensible.  Plus it's easy to extend
`gnutls_make_error' as we need more errors by name.

SM> The type you declare should correspond to the type of the objects you
SM> store there.  Always.  If you stick to this principle and avoid freeing
SM> live objects (and stay within array bounds, and a few more such things)
SM> your code will be more portable and won't dump core (hence will be
SM> generally easier to debug).

Got it.  I think I'm doing it more correctly now and there will be no GC
issues, as I mentioned above.

On Mon, 06 Sep 2010 19:18:01 +0200 Andreas Schwab <address@hidden> wrote: 

AS> Ted Zlatanov <address@hidden> writes:

>> +HAVE_GNUTLS=no
>> +if test "${with_gnutls}" = "yes" ; then
>> +  PKG_CHECK_MODULES([LIBGNUTLS], [gnutls >= 2.2.4])

AS> Are you sure you want to make gnutls a required dependency of Emacs?

>> +  AC_DEFINE(HAVE_GNUTLS)

AS> $ autoreconf
AS> autoheader: warning: missing template: HAVE_GNUTLS
AS> autoheader: Use AC_DEFINE([HAVE_GNUTLS], [], [Description])
AS> autoreconf: /usr/bin/autoheader failed with exit status: 1

No.  What would you suggest?

On Mon, 06 Sep 2010 17:53:46 +0200 Andreas Schwab <address@hidden> wrote: 

AS> Ted Zlatanov <address@hidden> writes:
>>>> +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> The function wants to store a value of one type into an object of a
AS> different type.  BAD.  The compiler is allowed to assume the object was
AS> never changed.

OK, you mean the cast is wrong.  I fixed that.  That leaves only the
transport cast from int in gnutls_{handshake,rehandshake} which I
believe is right from the original patch.

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> Take your pick.  I don't know anything about gnutls.

Well, none of the failures are fatal and there's a lot of ways to retry
the connection.  I think it's better to return the integer error value
or t to simplify the usage.  I changed the patch accordingly.

>>>> === 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?

AS> You never store Lisp_Object values in there, so what's the point?
AS> x509_callback is never used, btw.

Fixed (also see my response above to Stefan Monnier).  I've attached the
patch as it stands.

Thanks again for all your comments.  Getting there...

Ted

tls.patch
Description: Text Data