On Fri, 13 Aug 2010 11:57:45 -0400 Chong Yidong <cyd@stupidchicken.com> wrote: 

CY> Ted Zlatanov <tzz@lifelogs.com> writes:

>> +  do {
>> +    rtnval = gnutls_read( state, buf, nbyte);
>> +    printf("read %d bytes\n", rtnval);
>> +  } while( rtnval==GNUTLS_E_INTERRUPTED || rtnval==GNUTLS_E_AGAIN);

CY> You should use the GNU style here.

Fixed.

>> +DEFUN ("gnutls-init", Fgnutls_init, Sgnutls_init, 2, 2, 0,
>> +       doc: /* Initializes GNU TLS for process PROC for use as CONNECTION-END.

CY> This should be "Initialize" instead of "Initializes".

Fixed.

CY> In general, this docstring is not very informative.  I have not been
CY> following this patch closely; just from reading the docstring, I'm not
CY> sure what gnutls-init is supposed to do.  I assume that it means that,
CY> once it is called, all data sent from Emacs to the process PROC, and
CY> vice versa, will be encrypted using the GnuTLS library.  Is that right?
CY> Does `gnutls-handshake' need to be called before, or after, this?  What
CY> happens if you try to send data to PROC before `gnutls-handshake'?
CY> These issues should be explained in the docstring.

CY> More generally, why do we need to a separate `gnutls-init' call, instead
CY> of making `gnutls-handshake' and other functions automatically enable
CY> GnuTLS functionality for the process?

Simon's code included a gnutls.el library, attached here.  It shows how
to use it.  It's a straightforward port of the GnuTLS calls (hence the
docstrings assume familiarity with that library) so you have to call
them in order just like a C client would.  We could try to collect that
sequence (as seen in `open-ssl-stream' and `starttls-negotiate' twice,
why I don't know):

1) global init (set up static structures)
2) init the client
3) set protocol, cipher, compression, kx, mac priority
4) set credentials

into one C function call.  gnutls-handshake is called repeatedly (while
EAGAIN is returned) until either an error happens or it succeeds.  You
can even rehandshake().  So I think the idea is that it should be a
repeatable call while the rest of the initialization is supposed to be
done just once.  All of that could certainly be wrapped in a single C
call, but Simon did it the other way.

Writing to the stream before the handshake has succeeded will return -1
for number of bytes written, but the exact process is up to GnuTLS and
errno is passing the error code back to us:

(from `emacs_gnutls_write')
...
rtnval = gnutls_write (state, buf, nbyte);

if (rtnval == -1)
{
  if (errno == EINTR)
    continue;
  else
    return (bytes_written ? bytes_written : -1);
}
...

This won't happen unless the process' gnutls_state is set by
`gnutls_init', but it could potentially happen before a successful
handshake.

That's a lot to put into the docstrings for all those functions.  It's
probably better to point people to the gnutls.el docs.

>> +DEFUN ("gnutls-deinit", Fgnutls_deinit, Sgnutls_deinit, 1, 1, 0,

CY> I think this should be called `gnutls-stop' or something like that;
CY> "deinit" is not a proper word.  Maybe rename `gnutls-init' to
CY> `gnutls-start'.

>> +DEFUN ("gnutls-global-init", Fgnutls_global_init,

CY> This is again not very informative.  Does it mean that it is equivalent
CY> to calling `gnutls-init' on every process by default?

>> +DEFUN ("gnutls-global-deinit", Fgnutls_global_deinit,

CY> Again, "deinit" should not be used.

But those are the GnuTLS names for the functions.  So it's confusing if
the Emacs core process.c functions map to different names in GnuTLS.
Unless we abstract the process creation at the C layer (aggregating what
`open-ssl-stream' does and eliminating the one-to-one function
mappings), I think it's worse to rename these functions.

>> +DEFUN ("gnutls-protocol-set-priority", Fgnutls_protocol_set_priority,
>> +       Sgnutls_protocol_set_priority, 1, MANY, 0,
>> +       doc: /* Sets the priority on the protocol versions supported by GNU TLS for PROCESS.
>> +The first parameter must be a process.	Subsequent parameters should
>> +be integers.  Priority is higher for protocols specified before

CY> Use the word "argument" instead of "parameter".  Also, there is some
CY> formatting mix-up in this and other docstrings.

That's fixed and I removed the TAB characters that came from the
original patch.

On Fri, 13 Aug 2010 17:53:36 +0200 David Kastrup <dak@gnu.org> wrote: 

DK> int xxx(void);

Thanks, applied.

Also note there's some ambition in this patch to have Emacs provide
server-side SSL.  I don't know if that should be removed completely or
considered.

Ted