Re: [fetchmail-devel] fetchmail IMAP bug - causes some mails to not be delivered when using IDLE

[Matthias A. <mat...@gm...>]

Am 14.01.2010 11:45, schrieb Sunil Shetye:
> [switching to fetchmail-devel]
> 
> Hi Matthias,
> 
> Quoting from Sunil Shetye's mail on Wed, Jan 13, 2010:
>> I agree that many servers do not send it. Also, fetchmail can and should handle
>> the counts better. However, in my opinion, the fix from dovecot side should be
>> trivial. Whereas, the fix from fetchmail side will be non-trivial as a major
>> rewrite of the imap driver will be required.
> 
> I was thinking of rewriting the imap driver so that all imap server
> responses are routed via the imap_ok() function. This is so that all
> the unexpected 'EXISTS', 'EXPUNGE' responses get trapped properly.
> This is how it will work:
> 
> Any imap command will go through this kind of loop:
> 
> ===============================================================================
> #define PS_UNTAGGED 30 /* untagged response on imap command (internal use) */
> 
> ok = gen_send(sock, "SEARCH UNSEEN");
> while ((ok = imap_ok(sock, buf)) == PS_UNTAGGED)
> {
> 
>     /* if we have reached here, we have got an untagged response not parsable by imap_ok(). */
> 
>     if ((cp = strstr(buf, "* SEARCH")))
>     {
> 	/* parse the response */
>     }

Perhaps a separate imap_untagged_parse() function would be more suitable here,
and possibly called from inside imap_ok()? See below.

> 
>     /* go back to get the remaining response */
> }
> if (ok != 0)
> {
>     report(stderr, GT_("search for unseen messages failed\n"));
>     return(ok);
> }
> ===============================================================================
> 
> In essence, gen_recv() at many places in imap.c will get replaced by
> imap_ok().
> 
> Are all the above changes okay? Should I start with the changes in
> imap_getrange() only? imap_getpartialsizes() can be the next function
> if this works out.

Yes, the changes you propose are not only OK, but also desirable and
appreciated. (And as bug fix also fit for 6.3.14.)

The only thing I'm wondering about is PS_UNTAGGED: is it okay to have imap_ok()
return PS_UNTAGGED? It sort of breaks the assumption that imap_ok() fetches the
server's final verdict on the requested previous operation -- and that
propagates to all uses of gen_transact(), by reference.  Reason: imap_ok() is -
in spite of the "static" attribute - externally callable through the "struct
method imap", namely if other code calls method->parse_response when method is
imap -- for instance, driver.c and transact.c call imap_ok: (from cscope)

  File        Function     Line
0 fetchmail.h <global>      218 int (*parse_response)(int , char *);
1 driver.c    do_session   1154 err =
                                (ctl->server.base_protocol->parse_response)
                                (mailserver_socket, buf);
2 transact.c  gen_transact 1599 ok = (protocol->parse_response)(sock, buf);


Which raises another question: does it make sense to have caller-specific
parsers for untagged replies? Perhaps those functions that need them - for
instance for * SEARCH, should call some new imap_ok_with_untagged() function
instead, and the normal imap_ok handles all untagged implicitly (we can rename
imap_ok, and uses dummy functions that just pass in an additional argument
whether or not returning PS_UNTAGGED is requested).

I think (unless I'm mistaken) we need to deal with asynchronous (non-requested)
untagged messages anyways, so perhaps all of the untagged logic needs to be in
imap_ok, but I'm not sure about that.  Since I haven't looked at the imap.c code
lately, please only consider this brainstorming or "thinking aloud" and not a
plea or request to do things in a certain way.  Design it the way you think fits
the needs best.

Thanks for participating in the solution of this issue!

Best regards
Matthias