Re: Emacs core TLS support

[Stefan Monnier <monnier@iro.umontreal.ca>]

> 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).

Yes, NILP is the way to test it (anything that's not nil is considered
as a way to say "true").

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

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

> And should I default to anonymous?

I don't know what that means.

>>> +#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

These extra slots don't actually Lisp objects, so this type is wrong
(and it forces you to use casts: any use of cast is a sign of
a problem, tho admittedly C often makes problems unavoidable).
If you check the "struct Lisp_Process" in process.h you'll see this:

    /* After this point, there are no Lisp_Objects any more.  */
    /* alloc.c assumes that `pid' is the first such non-Lisp slot.  */

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

> 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).

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

   static Lisp_Object Qgnutls_foo;

   gnutls_function (Lisp_Object arg1, ...)
   {
      arg1 = Fget (arg1, Qgnutls_code);
      int op = INTEGERP (arg1) ? XINT (arg1) : ....;
      if (op < n1 || op > n2)
         error ("Invalid op code");
      gnutls_foo (..., op, ...);
   }

   syms_of_gnutls ()
   {
      ...
      Qgnutls_foo = intern ("gnutls-foo");
      ...
      Fput (Qgnutls_foo, Qgnutls_code, make_number (GNUTLS_FOO));
   }

It's not super-lightweight, but it can be more convenient than a bunch
of (EQ (foo, Qgnutls_bar)) if there are many different possible values.

>>> +  ret = gnutls_init((gnutls_session_t*)&(XPROCESS(proc)->gnutls_state), 
AS> Aliasing violation.
> Can you explain please?

Casts are evil, try real hard to stay away from them (he said, and then
added a few casts).

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?

 error ("some informative string");

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

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


        Stefan