On Tue, 30 Mar 2010, Ben Greear wrote:

> More work is needed to make imap useful, including parsing URLs to grab
> folder and/or email ID. Folder list format might need to be changed as
> well..it's pretty raw at the moment.

Allow me to throw in some early feedback:

I really approve of and like the idea, much like POP3 and FTP we can return
listings when the URL ends with a trailing slash == no mailbox is given.

Also, this is another perfect example where provide a set of test cases will
be golden. Not the least to verify that the error cases still work out as they
should.

> +static size_t get_until_nstarts(const char* str, size_t max, const char*
> starts) {

This looks like a line longer than 79 columns and it has the open brace on the
wrong line for curl indent style. This same mistake is found more than once.

> + /* Deal with \n\r and \r\n in case, maybe overly paranoid. */

I don't think the IMAP RFC says they can occur like that in any order. In
fact, it is pretty clear that all lines end with CRLF and nothing else.

It's nothing wrong with being careful, it's just not worth it to make the code
hard-to-read to cover up for something that is a protocol violation anyway -
at least not if the violation isn't in common use.

> +static const char* curl_strcasestr_ln(const char* h, const char* n, size_t*
> consumed) {

You shouldn't use a 'curl_' prefix for a private function, we save those for
public functions.

> + if (strncasecmp(h+i, n, lnn) == 0) {

strncasecmp() is not portable, and it "suffers" from being locale dependent. I
recommend you use Curl_raw_nequal() instead.

> + /* Something like this comes "* LIST (\Flags) "/" "INBOX" */
> + t2 = strstr(ptr, "* LIST ");

This seems a bit strange to me. If you know that the string may in the buffer,
shouldn't it then always start from the beginning? Why would you need to scan
for it with strstr() ? If it doesn't always start the buffer, what other junk
are you intending to pass?

> + if (curl_strcasestr_ln(pp->cache, " OK LIST completed", &i))

This is wrong. The 'OK' is defined in the spec to appear, but the rest is not
and may just as well be something completely different so you cannot use
anything further than the OK to match on (and the response id before the OK is
of course the same id that was used in the command).

> + if (strlen(conn->proto.imapc.mailbox))

I suggest if(conn->proto.imapc.mailbox[0]) instead, as you don't care about
the length only that it isn't zero.

-- 
  / daniel.haxx.se
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette:  http://curl.haxx.se/mail/etiquette.html