Re: [PATCH/RFC] git-imap-send: use libcurl for implementation
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Aug 13, 2014, 01:59 UTC
- Message-ID
- <20140813015917.GA30756@google.com>
- In-Reply-To
- <53EA8C3E.1080500@raz.or.at>
Bernhard Reiter wrote:
> Use libcurl's high-level API functions to implement git-imap-send > instead of the previous low-level OpenSSL-based functions.
Wow! This sounds lovely. Thanks for working on this.
[...]
Show 9 quoted lines
> Since version 7.30.0, libcurl's API has been able to communicate with > IMAP servers. Using those high-level functions instead of the current > ones reduces imap-send.c by some 1200 lines of code. > > As I don't have access to that many IMAP servers, I haven't been able to > test a variety of parameter combinations. I did test both secure and > insecure (imaps:// and imap://) connections -- this e-mail was generated > that way -- but could e.g. neither test the authMethod nor the tunnel > parameter.
The above two paragraphs belong in the commit message, too, since they'll be just as useful for someone looking back through the history as for someone reading the patch on-list today.
[...]
> --- a/INSTALL > +++ b/INSTALL
[...]
> - - "libcurl" library is used by git-http-fetch and git-fetch. You > + - "libcurl" library is used by git-http-fetch, git-fetch, and > + git-impap-send. You might also want the "curl" executable for
Typo: s/impap-send/imap-send/
Show 11 quoted lines
> --- a/Makefile > +++ b/Makefile > @@ -2067,9 +2067,9 @@ endif > git-%$X: %.o GIT-LDFLAGS $(GITLIBS) > $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS) > > -git-imap-send$X: imap-send.o GIT-LDFLAGS $(GITLIBS) > +git-imap-send$X: imap-send.o http.o GIT-LDFLAGS $(GITLIBS) > $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \ > - $(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO) > + $(LIBS) $(CURL_LIBCURL)
7.30.0 is only ~1 year old. Does this mean users would need to update curl in order to build imap-send?
For example, Debian 7.6 ships curl 7.26.0 and CentOS 7 has curl 7.29.0.
Ideally this could be done as an optional feature:
1. Introduce a USE_CURL_FOR_IMAP_SEND makefile variable to take
advantage of the nice new API. 2. (optional) Use the curl_check makefile variable to turn on
USE_CURL_FOR_IMAP_SEND automatically when appropriate. 3. In a few years, when everyone has upgraded, we could simplify by
getting rid of the USE_OPENSSL_FOR_IMAP_SEND code path.What do you think?
[...]
Show 7 quoted lines
> --- a/imap-send.c > +++ b/imap-send.c > @@ -22,47 +22,13 @@ > * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > */ > > -#include "cache.h"
Usual style is to start with a #include of "cache.h" or "git-compat-util.h". "http.h" including cache.h for itself was an old mistake. (I'll reply with a patch to fix that.)
[...]
> +#include <curl/curl.h>
http.h already #includes this. Do you use other helpers from http.h/http.c or do you use libcurl directly? (Just curious.)
Some style nits:
[...]
> +static curl_socket_t opensocket(void *clientp, curlsocktype purpose, > + struct curl_sockaddr *address)
Long line. Do you have ts=4? (Git uses 8-space tabs. There's some emacs magic in Documentation/CodingGuidelines. It should be possible to add similar hints there for other editors if they don't do the right thing by default.)
> +{
> + curl_socket_t sockfd;
> + (void)purpose;
> + (void)address;Elsewhere git lets unused parameters be. The unused parameter warning is too noisy in callback-heavy code (e.g., for_each_ref) so we don't turn it on.
> + sockfd = *(curl_socket_t *)clientp; > + /* the actual externally set socket is passed in via the OPENSOCKETDATA > + option */
(style nit) Comments in git look like this:
/* * The actual, externally set socket is passed in via the * OPENSOCKETDATA option. */ return sockfd;
[...]
Show 5 quoted lines
> +static int sockopt_callback(void *clientp, curl_socket_t curlfd,
> + curlsocktype purpose)
> +{
> + /* This return code was added in libcurl 7.21.5 */
> + return CURL_SOCKOPT_ALREADY_CONNECTED;I'd drop the comment, unless there's some subtlety involved. (E.g., is there some other return code that would be more appropriate but was introudced later or something?)
[...]
Show 6 quoted lines
> @@ -1368,12 +218,14 @@ static int git_imap_config(const char *key, const char *val, void *cb)
> int main(int argc, char **argv)
> {
> struct strbuf all_msgs = STRBUF_INIT;
> - struct strbuf msg = STRBUF_INIT;
> + struct buffer msg = { STRBUF_INIT, 0 };Ah, ok --- we do use http.c stuff.
[...]
> + char path[8192]; > + int pathlen;
I realize the old code only had 8192 for the IMAP command buffer, but could this be a strbuf now, or is there some underlying limit somewhere else?
[...]
Show 5 quoted lines
> @@ -1417,31 +269,89 @@ int main(int argc, char **argv) > return 1; > } > > + curl_global_init(CURL_GLOBAL_ALL);
http.c seems to make the same mistake, but should the return value from this be checked?
Show 9 quoted lines
> - /* write it to the imap server */
> - ctx = imap_open_store(&server);
> - if (!ctx) {
> - fprintf(stderr, "failed to open store\n");
> + curl = curl_easy_init();
> +
> + if (!curl) {
> + fprintf(stderr, "failed to init curl\n");
> return 1;Could do
die("failed to init curl");for more consistent message format and exit codes (128 for internal errors, with an error message starting with 'fatal: ').
[...]
Show 9 quoted lines
> + if (ends_with(server.host, "/"))
> + pathlen = snprintf (path, sizeof(path), "%s%s", server.host, imap_folder);
> + else
> + pathlen = snprintf (path, sizeof(path), "%s/%s", server.host, imap_folder);
> +
> + if (pathlen < 0)
> + die("Fatal: Out of memory");
> + if (pathlen >= sizeof(path))
> + die("imap URL overflow!");With a strbuf, this could be something like
strbuf_addstr(&path, server.host); if (!path.len || path.buf[path.len - 1] != '/') strbuf_addch(&path, '/'); strbuf_addstr(&path, imap_folder);
or
if (ends_with(...)) strbuf_addf(&path, "%s%s", ...); else strbuf_addf(...);
Killing the unused ctx->prefix handling is nice. :)
[...]
> + if (server.tunnel) {
> + const char *argv[] = { server.tunnel, NULL };
> + struct child_process tunnel = {NULL};(not about this patch) Could use the child_proccess's internal argv_array:
struct child_process tunnel = {NULL};
argv_array_push(&tunnel.args, server.tunnel);(about this patch) Would there be a way to make this part reuse the existing code? The only difference I see is that *srvc has been renamed to server, which doesn't seem to be related to the change of transport API from OpenSSL to libcurl.
[...]
> + curl_socket_t sockfd = tunnel.out; // what about tunnel.in ?
Hmm. curl expects to get a socket it can send(), recv(), setsockopt(), etc on instead of a pair of fds to read() and write().
I wonder why someone would want to use SSL through a tunnel, though. Currently it's impossible to get to the SSL codepath when a tunnel is active (it's in the 'else' block an 'if (srvc->tunnel)'). If that property is preserved, then we should be safe.
To summarize:
* I like this idea a lot! Using libcurl's imaps:// support directly means one less dependency to worry about, and using alternate SSL libraries like gnutls or nss becomes much easier (e.g., see http://fedoraproject.org/wiki/FedoraCryptoConsolidation for how that makes configuring certificate trust simpler).
* This would be easier to take if guarded by an #ifdef, so people stuck on ancient libcurl would still be able to use git (and ideally still use imap over SSL).
* This shouldn't have to touch the imap.tunnel support. imap-send's imap.tunnel configuration expects the tunnel to take care of securing the channel (e.g. by using 'openssl s_client').
* Any potential cleanups noticed along the way are very welcome, as separate patches.
* As soon as you're ready to roll this out to a wider audience of testers, let me know, and we can try to get it into shape for Junio's "next" branch (and hence Debian experimental).
Thanks and hope that helps, Jonathan