On Mon, 06 Sep 2010 00:47:39 +0200 Stefan Monnier wrote: SM> For symmetry, I'd say "Does Emacs use -lgnutls?". Fixed. SM> Use C-u M-x checkdoc-current-buffer which will help you follow the usual SM> coding conventions (e.g. inserting the GPL blurb). Fixed. >> +(defvar starttls-host nil) SM> What is this for? It seems to only ever be set and never read. I think Simon Josefsson intended to do more with it but you're right, right now it's unused. I removed it. >> +DEFUN ("gnutls-init", Fgnutls_init, Sgnutls_init, 2, 2, 0, >> + doc: /* Initializes GNU TLS for process PROC for use as CONNECTION-END. >> +CONNECTION-END is used to indicate if this process is as a server or >> +client. Can be one of `gnutls-client' and `gnutls-server'. Currently >> +only `gnutls-client' is supported. SM> This formulation means that the symbols (rather than the value of the SM> corresponding variables) `gnutls-client' and `gnutls-server' are the SM> valid values. We don't support server mode anyhow so I removed the connection-end argument altogether (hardcoding the init to GNUTLS_CLIENT). `gnutls-bye' still mentions `gnutls-e-again' and `gnutls-e-interrupted'. I'm not sure if I should return a symbol or the numeric value there. SM> This said, while I understand the general desire to just bring the C API SM> of GNU TLS into Elisp, as long as you do it by hand, you might as well SM> use here a Lisp boolean for connection_end. Yeah, I'll do that for `gnutls-bye' with a CONT parameter. It's just a NILP check to handle booleans IIUC (there's no CHECK_BOOLEAN, so it's either NILP or EQ to Qt). >> + state = (gnutls_session_t) XPROCESS(proc)->gnutls_state; >> + gnutls_deinit(state); SM> Please always put a space before the open paren of a macro or SM> function call. Applies to the rest of the code as well, of course. Fixed. SM> return make_number (lret); Fixed everywhere, I think, and I condensed the code when possible (argh, no C99 :) >> +CERTFILE is a PEM encoded file. Returns zero on success. >> +*/) SM> By convention we keep the closing */) at the end of the previous line. Fixed. >> +DEFUN ("gnutls-cred-set", Fgnutls_cred_set, >> + Sgnutls_cred_set, 2, 2, 0, >> + doc: /* Enables GNU TLS authentication for PROCESS. >> +TYPE is an integer indicating the type of the credentials, either >> +`gnutls-anon', `gnutls-srp' or `gnutls-x509pki'. SM> Again, the above formulation means that the caller should pass those SM> symbols rather than value associated with the corresponding variables. In this case I think that's the right approach actually so we shouldn't have defconsts. See new definition, it uses two local Lisp_Objects for the symbol names. Where should I allocate those constant Lisp_Objects globally properly? And should I default to anonymous? >> + if (gnutls_certificate_allocate_credentials (&x509_cred) < 0) >> + memory_full (); SM> Can it really only mean "memory is full"? I think so. >> === added file 'src/gnutls.h' SM> Why add this file? Doesn't seem worth the trouble. As gnutls.c grows, I think it will be necessary. It can be removed now if you want. >> +#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 SM> Rather than hardcode variables in gnutls.el, an alternative could be to SM> define those variables in gnutls.c so you can initialize them to the SM> values taken from gnutls/gnutls.h. I'd like to take the direction of a more Lisp-y API on top of the GnuTLS API. So any constants should be limited to the function bodies and I'd like to stick to symbols (as with gnutls-cred-set in the new patch). On Sun, 05 Sep 2010 10:06:09 +0200 Andreas Schwab wrote: AS> You should remove the debugging output. Fixed. >> +DEFUN ("gnutls-init", Fgnutls_init, Sgnutls_init, 2, 2, 0, ... >> + ret = gnutls_init((gnutls_session_t*)&(XPROCESS(proc)->gnutls_state), AS> Aliasing violation. Can you explain please? AS> IMHO all your functions should return t on success and either some error AS> symbol on failure or even raise an error. Yes, but I'm not sure which one. Can you recommend? AS> No C99. Yes, sorry for leaving that in the patch. Removed. >> === modified file 'src/process.h' >> + >> +#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 AS> None of them should be Lisp_Objects. Also make sure the resources are AS> properly released when the process object is deleted. I don't know enough (the choice of using Lisp_Objects was in the original patch) to know what to do instead of using Lisp_Objects. Why not, first of all? Revised patch is attached (it compiles but the changes are mostly cosmetic so it's still not usable). Thank you for the comments, they were very helpful. I hope to get this in a usable state ASAP. Ted