Re: [PATCH] GnuTLS support on Woe32

[Stefan Monnier <monnier@IRO.UMontreal.CA>]

SM> Also further down you define Qgnutls_hostname but never use it, but here
SM> would be a good place to use it (otherwise, don't define it).
SM> Finally, if you want to avoid Fsymbol_value, you can use DEFVAR_LISP to
SM> define Vgnutls_hostname so you can then just do SSDATA (Vgnutls_hostname).
> Fixed.  I wanted to define that variable in gnutls.el so I can make it
> buffer-local there too (right before it's used).  If you think that's
> better in gnutls.c, I'll change it.

You can call Fmake_variable_buffer_local from C code just as well.
Grep for `fontification_functions' for an example.

BTW, I had not noticed this part in gnutls.el, which seems like an
error: why would you want it to be buffer-local?  Gnutls is about
processes, so binding this var to buffers makes no sense to me.

Whether to define it in C or in Elisp is mainly a question of what's
more convenient and whether you'd rather think that the functionality
associated with this variable is implemented in C or in Elisp.

Now that I look at it, I don't understand what this gnutls-hostname
variable is about.  Why isn't it an additional keyword argument instead?
It needs better documentation than the current "Remote hostname.".

> where I thought removing the braces looked confusing and ugly because of
> the nesting.

Fine (I personally prefer this code without the internal braces, but
it's no big deal).  I'm not opposed to braces, but in the previous code
there was a lot of them around repetitive and "simple" code which lead
to the code being much too diluted.

SM> Shouldn't that be "Iowait"?
> No, see gnutls_transport_set_lowat() for instance.

OK, thanks.

> I've attached an updated patch.  Sorry if I have missed anything.  It
> would be nice to have an automatic way to catch these formatting issues.

We could come up with some font-lock rules to highlight "offending"
code, but I'm not sure it's worth the trouble.

> Unfortunately the validation is tightly coupled to the C-level GnuTLS
> functions so it would require writing a lot of glue code.  All the
> session data initialization and certificate validation are done with
> GnuTLS C functions and the data passed around has to be at the C level.
> Breaking up the validation into chunks could help but then more
> intermediate results have to be stored in each buffer and the
> error-handling logic would get even more complicated.

I saw that, and I'm OK with the patch as it is in this regard.

> I am excited that this patch finally achieves the base functionality
> Emacs needs to do SSL and TLS connections without helper applications on
> most platforms we support.  So I hope I can make it acceptable soon :)

Looks pretty good, yes.  A few more nitpicks below.

> +:verify-flags is a bitset as per gnutls_certificate_set_verify_flags().

In the GNU system we use the convention that "funname()" is a function
call and denotes the result of calling that function, rather than the
function itself.  To refer to the function, just say "funname".

> +:verify-hostname-error determines if a hostname mismatch is a warning
> +or an error.

Try to use the form "if non-nil blabla", so it's clear which value gives
you which behavior.


        Stefan