Re: [PATCH] GnuTLS support on Woe32

[Eli Zaretskii <eliz@gnu.org>]

> From: claudio.bley@gmail.com (Claudio Bley)
> Date: Sun, 06 Mar 2011 16:16:34 +0100
> 
> Please find attached a patch which makes building Emacs with GnuTLS
> support on Woe32 possible.

Thanks.

I have a few initial comments, based on reading through the patch.

> --- lisp/gnus/starttls.el	2011-01-25 04:08:28 +0000
> +++ lisp/gnus/starttls.el	2011-03-06 14:57:51 +0000
> @@ -195,37 +195,46 @@
>    :type 'regexp
>    :group 'starttls)
>  
> +(eval-and-compile
> +  (when (fboundp 'gnutls-boot) (require 'gnutls)))

Can you explain why are these fboundp calls needed?

> --- lisp/net/gnutls.el	2011-01-25 04:08:28 +0000
> +++ lisp/net/gnutls.el	2011-03-06 14:57:51 +0000
> @@ -78,7 +78,8 @@
>  KEYFILES is a list of client keys."
>    (let* ((type (or type 'gnutls-x509pki))
>           (trustfiles (or trustfiles
> -                        '("/etc/ssl/certs/ca-certificates.crt")))
> +                         (when (file-exists-p "/etc/ssl/certs/ca-certificates.crt")
> +                              '("/etc/ssl/certs/ca-certificates.crt"))))

Can a file name that starts with a slash work reliably on Windows?

> +2011-03-06  Claudio Bley  <claudio.bley@gmail.com>
> +
> +	* configure.bat: New options --without-gnutls and --lib, new build
> +	variable USER_LIBS, automatically detect GnuTLS.
> +	* INSTALL: Add instructions for GnuTLS support.
> +	* gmake.defs: Prefix USER_LIB's with -l.

Why do we need the --lib switch?  We don't require it for any other
optional libraries.  Can we arrange for GnuTLS support configury to
work like the other optional libraries?

> +* Optional GnuTLS support
> +
> +  To build Emacs with GnuTLS support, make sure that the
> +  gnutls/gnutls.h header file can be found in the include path and
> +  link to the appropriate libraries (e.g. gnutls.dll and gcrypt.dll)
> +  using the --lib option.

Is it possible to mention here good sites to look for the GnuTLS
header and libraries?

> +#ifdef WINDOWSNT
> +#  include "sys/socket.h"
> +#  include "systime.h"
> +
> +/* we need to translate Winsock errors because GnuTLS only checks
> + * for EAGAIN or EINTR */
> +static int
> +wsaerror_to_errno(int err)
> +{
> +  switch (err)
> +    {
> +    case WSAEWOULDBLOCK:
> +      return EAGAIN;
> +    case WSAEINTR:
> +      return EINTR;
> +    default:
> +      return err;
> +    }
> +}

Why is this function needed?  Can you extend w32.c:set_errno instead
(if it doesn't already support all the values of WSA* errors that you
need)?

> +static ssize_t
> +emacs_gnutls_pull(gnutls_transport_ptr_t p, void* buf, size_t sz)

Can we move the Windows-specific functions to w32.c, and only call
them from gnutls.c?  I think we want to keep the Windows-related code
outside w32*.c to the bare minimum.

> +      /* On Windows we cannot transfer socket handles between
> +       * different runtime libraries.
> +       *
> +       * We must handle reading / writing ourselves.
> +       */

This is not the Emacs style of comments.

> -  ret = gnutls_handshake (state);
> +  do
> +    {
> +      ret = gnutls_handshake (state);
> +      emacs_gnutls_handle_error (state, ret);
> +    }
> +  while (ret < 0 && gnutls_error_is_fatal (ret) == 0);

This change is not Windows-specific.  What problem(s) does it solve,
and are those problems relevant to platforms other than Windows?

> +  else
> +    {
> +        gnutls_alert_send_appropriate (state, ret);
> +    }
> +  return ret;

Likewise here.

> -            return (bytes_written ? bytes_written : -1);
> +            {
> +              emacs_gnutls_handle_error (state, rtnval);
> +
> +              return (bytes_written ? bytes_written : -1);
> +            }

And here.  Why do you introduce emacs_gnutls_handle_error?

> --- src/process.c	2011-02-18 17:37:30 +0000
> +++ src/process.c	2011-03-06 14:57:51 +0000
> @@ -4785,6 +4785,21 @@
>               &Available,
>               (check_write ? &Writeok : (SELECT_TYPE *)0),
>               (SELECT_TYPE *)0, &timeout);
> +
> +#ifdef HAVE_GNUTLS
> +          /*
> +           * GnuTLS buffers data internally. In lowat mode it leaves some data
> +           * in the TCP buffers so that select works, but with custom pull/push
> +           * functions we need to check if some data is available in the buffers
> +           * manually.
> +           */
> +          if (nfds == 0 && wait_proc && wait_proc->gnutls_p
> +              && gnutls_record_check_pending(wait_proc->gnutls_state) > 0)
> +          {
> +              FD_SET (wait_proc->infd, &Available);
> +              nfds = 1;
> +          }
> +#endif

Is this for Windows only?  If so, please mention that in a comment.
If not, what problems does it solve on other platforms?

Last, but not least: I don't see your name on file with the FSF
copyright assignments.  A contribution of this size will require you
to sign legal papers.

Thanks again for working on this.