[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] GnuTLS support on Woe32
From: |
Eli Zaretskii |
Subject: |
Re: [PATCH] GnuTLS support on Woe32 |
Date: |
Sun, 06 Mar 2011 18:58:46 +0200 |
> From: address@hidden (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 <address@hidden>
> +
> + * 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.
- [PATCH] GnuTLS support on Woe32, Claudio Bley, 2011/03/06
- Re: [PATCH] GnuTLS support on Woe32,
Eli Zaretskii <=
- Re: [PATCH] GnuTLS support on Woe32, Robert Pluim, 2011/03/07
- Re: [PATCH] GnuTLS support on Woe32, Claudio Bley, 2011/03/07
- Re: [PATCH] GnuTLS support on Woe32, Robert Pluim, 2011/03/08
- Re: [PATCH] GnuTLS support on Woe32, Eli Zaretskii, 2011/03/08
- Re: [PATCH] GnuTLS support on Woe32, Robert Pluim, 2011/03/08
- Re: [PATCH] GnuTLS support on Woe32, Lars Magne Ingebrigtsen, 2011/03/08
- Re: [PATCH] GnuTLS support on Woe32, Eli Zaretskii, 2011/03/08