On Mon, 06 Sep 2010 23:00:51 +0200 Stefan Monnier wrote: >> 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? SM> It's typically declared as global (static or not, depending on whether SM> it's used in other files) and initialized in syms_of_. SM> Look at other syms_of_ to see what it looks like. Done, thanks. >> And should I default to anonymous? SM> I don't know what that means. If the user passes an unknown symbol to `gnutls-cred-set', should it be treated as `gnutls_anon' or generate an error? It could work either way. I'm leaning towards an error but it seems kind of rude to the user. OTOH it could be a serious problem to use encryption the user did not intend because of a typo. SM> the slots you add at the end are ignored by the GC (which is what SM> you want in your case, since they're not Lisp objects and hence the SM> GC wouldn't know what to do with them) and can be of any type. So SM> just use the types you need here such that casts aren't needed. OK. I introduced a new field `gnutls_state_usable' to indicate the session has been initialized. I could have made it a byte but it may be useful to hold Lisp-related state for this patch as it evolves. It's before the GC marker field "pid" so it will be noticed by alloc.c. SM> BTW, if it makes the code simpler, you can use the following trick: use SM> symbols, but associate an integer to each symbol by way of SM> symbol properties. I don't like the properties because they are loosely bound to the symbol (for errors I think it's better to bind meaning to value tightly). Is it OK to do the current approach, where I have the function `gnutls_make_error' to return the right thing, whether it's a known integer-as-symbol or a generic integer? I think it's the right approach and it seems semantically sensible. Plus it's easy to extend `gnutls_make_error' as we need more errors by name. SM> The type you declare should correspond to the type of the objects you SM> store there. Always. If you stick to this principle and avoid freeing SM> live objects (and stay within array bounds, and a few more such things) SM> your code will be more portable and won't dump core (hence will be SM> generally easier to debug). Got it. I think I'm doing it more correctly now and there will be no GC issues, as I mentioned above. On Mon, 06 Sep 2010 19:18:01 +0200 Andreas Schwab wrote: AS> Ted Zlatanov writes: >> +HAVE_GNUTLS=no >> +if test "${with_gnutls}" = "yes" ; then >> + PKG_CHECK_MODULES([LIBGNUTLS], [gnutls >= 2.2.4]) AS> Are you sure you want to make gnutls a required dependency of Emacs? >> + AC_DEFINE(HAVE_GNUTLS) AS> $ autoreconf AS> autoheader: warning: missing template: HAVE_GNUTLS AS> autoheader: Use AC_DEFINE([HAVE_GNUTLS], [], [Description]) AS> autoreconf: /usr/bin/autoheader failed with exit status: 1 No. What would you suggest? On Mon, 06 Sep 2010 17:53:46 +0200 Andreas Schwab wrote: AS> Ted Zlatanov writes: >>>> +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> The function wants to store a value of one type into an object of a AS> different type. BAD. The compiler is allowed to assume the object was AS> never changed. OK, you mean the cast is wrong. I fixed that. That leaves only the transport cast from int in gnutls_{handshake,rehandshake} which I believe is right from the original patch. 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> Take your pick. I don't know anything about gnutls. Well, none of the failures are fatal and there's a lot of ways to retry the connection. I think it's better to return the integer error value or t to simplify the usage. I changed the patch accordingly. >>>> === 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? AS> You never store Lisp_Object values in there, so what's the point? AS> x509_callback is never used, btw. Fixed (also see my response above to Stefan Monnier). I've attached the patch as it stands. Thanks again for all your comments. Getting there... Ted