Re: Updated GNU TLS bindings

[Simon Josefsson]

Thanks for looking at the patch, I will need a night or two more to
clean it up some more based on your comments.

"Eli Zaretskii" <address@hidden> writes:

>> From: Simon Josefsson <address@hidden>
>> Date: Fri, 25 Jan 2002 23:08:02 +0100
>> 
>> I have cleaned up my GNU TLS support for Emacs, and written some
>> documentation as well (included below, it can use to help of someone
>> who knows english).
>
> Thanks.  A few comments about this:
>
>   - the changes should include an addition to the help screen printed
>     by the configure script, in the --with-PACKAGE area, which
>     mention the --with-gnutls option;

The AM_PATH_LIBGNUTLS macro is supposed to take care of this, it uses
the M4 macros installed from GNU TLS.  I forgot to include
"aclocal.m4" in my patch -- I will next time.

(How can I do "cvs diff --new-file"?  Right now I'm manually diffing
the new files..)

>   - likewise, INSTALL should mention that, and give a pointer to a
>     place where one could find TLS;

Done.

>   - there should be an entry in NEWS about this addition;

Yes.  In writing these, I discovered that the lisp API could use some
improvements, so I separated gnutls.el into ssl.el and starttls.el
(both compatible with the older versions of the libraries with these
names).

What do you think of gnutls.el?  It contains variable definitions used
by C defuns.  Maybe I should move these defvars into C?  The problem
is that there are maybe a hundred more of them, from the gnutls.h
file.

I have also considered that they shouldn't be integer variables at
all, but rather symbols.  So you would say (gnutls-init myproc
'gnutls-client) instead of (gnutls-init myproc gnutls-client).

>   - aren't there any user-visible aspects of TSL support?  If there
>     are, they should be mentioned in the user manual.

The only visible aspects are that Gnus and smtpmail.el supports TLS
connections.  There is already TLS discussions in the Gnus manual, but
they reflect the external libraries, and smtpmail.el isn't documented
at all (which is another bug, but I'm not ready to fix that one yet).

What is the policy regarding the Gnus manual?  Is anyone editing it
for use in Emacs, or is it incorporated straight from the Gnus
distribution?  The reason I ask is that Gnus works with older versions
of Emacs, so it contains discussion that applies to more than the
current version.  I'd like to write the TLS stuff in the same manner,
"if your emacs support TLS, you don't have to do anything, otherwise
do <insert existing text>".

>> address@hidden gnutls-init proc connection_end
>> address@hidden defun
>> address@hidden gnutls-deinit proc
>> address@hidden defun
>
> These (and other function descriptions) should have descriptions which
> at least explain the meaning of the arguments.

Yes, that was one of the minor issues I left.  I wanted to improve the
docstrings on the functions first, and then adopt the docstrings into
the documentation.

>> +emacs_gnutls_write (fildes, state, buf, nbyte)
>> +     int fildes;
>> +     GNUTLS_STATE state;
>> +     char *buf;
>> +     unsigned int nbyte;
>> +{
>> +  register int rtnval, bytes_written;
>> +
>> +  puts("emacs_gnutls_write");
>
> ??? Is this `puts' a remnant of the debugging stage?  (There are
> other similar puts and printf calls in the diffs.)

Oops.  Yes, debug stuff.  Removed.

>> +  alg = gnutls_mac_get_algo(state);
>> +  XSETINT (ret, alg);
>> +
>> +  return ret;
>
> I think make_number is a slightly better way of doing this:
>
>    return make_number (gnutls_mac_get_algo(state));

I rewrote it into:

  alg = gnutls_mac_get_algo(state);
  ret = make_number (alg);

  return ret;

which I find easier to debug, even though it wastes a few LOCs.

>> +Each authentication type may need additional information in order to
>> +work.  For anonymous (`gnutls-anon'), see also
>> +`gnutls-anon-set-client-cred'.  For SRP (`gnutls-srp'), see also
>
> Shouldn't parentheses in a doc string be escaped with a backslash?

I don't know the answer.  Looking at the docstring for
`compare-strings' doesn't seem to suggest this though.

>> Index: src/process.h
>> ===================================================================
>> RCS file: /cvsroot/emacs/emacs/src/process.h,v
>> retrieving revision 1.18
>> diff -u -r1.18 process.h
>> --- src/process.h    14 Oct 2001 20:14:49 -0000      1.18
>> +++ src/process.h    25 Jan 2002 21:58:46 -0000
>> @@ -91,6 +91,13 @@
>>      /* Flag to set coding-system of the process buffer from the
>>         coding_system used to decode process output.  */
>>      Lisp_Object inherit_coding_system_flag;
>> +#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
>
> I think these objects should exists in the struct even if Emacs was
> compiled without TLS.  IMHO, it's a potential trouble to have binary
> API incompatibilities that depend on the compilation switches.

I removed the #ifdef.  These aren't really Lisp_Objects (they are
casted), so I don't know if they should be there at all.  How can this
be solved?