Menu
▾
▴
fetchmail-devel
|
From: Sunil S. <sh...@bo...> - 2010-01-14 11:45:30
|
[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 */
}
/* 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.
--
Sunil Shetye.
|
|
From: Matthias A. <mat...@gm...> - 2010-01-14 13:34:39
|
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
|
|
From: Sunil S. <sh...@bo...> - 2010-01-18 09:22:46
Attachments:
fetchmail-6.3.13-imapok.patch
|
Quoting from Matthias Andree's mail on Thu, Jan 14, 2010: > Perhaps a separate imap_untagged_parse() function would be more suitable here, > and possibly called from inside imap_ok()? See below. Ok, I have made a separate imap_untagged_response() to parse the untagged response. Other key changes: - use stage == STAGE_GETAUTH && !strncmp(buf, "* CAPABILITY", 12) - instead of strstr(buf, " CAPABILITY") - use stage == STAGE_GETAUTH && !strncmp(buf, "* PREAUTH", 9) - instead of strstr(buf, " PREAUTH") - use stage == STAGE_GETRANGE && !check_only && strstr(buf, "[READ-ONLY]") - instead of !check_only && strstr(buf, "[READ-ONLY]") - log "* BYE" message These changes should save a lot of string comparisons during runtime. > 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). In order to address this, I have made one more intermediate function imap_response(). Caller-specific parsers will call imap_response() in a loop and others will continue calling imap_ok(). On reading the RFCs, it looks like the [READ-ONLY] flag is actually part of a tagged response. Should the [READ-ONLY] check be moved to imap_response()? Functions cleaned up: imap_getrange() imap_getpartialsizes() imap_trail() > 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. Please check if this patch addresses all the issues raised. -- Sunil Shetye. |
|
From: Matthias A. <mat...@gm...> - 2010-01-22 02:20:46
|
Dear Sunil, dear readers, Am 18.01.2010, 09:32 Uhr, schrieb Sunil Shetye <sh...@bo...>: > Quoting from Matthias Andree's mail on Thu, Jan 14, 2010: >> Perhaps a separate imap_untagged_parse() function would be more >> suitable here, >> and possibly called from inside imap_ok()? See below. > > Ok, I have made a separate imap_untagged_response() to parse the > untagged response. Other key changes: > > - use stage == STAGE_GETAUTH && !strncmp(buf, "* CAPABILITY", 12) > - instead of strstr(buf, " CAPABILITY") > > - use stage == STAGE_GETAUTH && !strncmp(buf, "* PREAUTH", 9) > - instead of strstr(buf, " PREAUTH") > > - use stage == STAGE_GETRANGE && !check_only && strstr(buf, > "[READ-ONLY]") > - instead of !check_only && strstr(buf, "[READ-ONLY]") > > - log "* BYE" message > > > These changes should save a lot of string comparisons during runtime. OK, makes sense (plus we don't want these anywhere on the line but just after the non-tag). > In order to address this, I have made one more intermediate function > imap_response(). Caller-specific parsers will call imap_response() in > a loop and others will continue calling imap_ok(). Good. > Please check if this patch addresses all the issues raised. To me it seems fine, based on the testing that you and Matt have been doing, I've committed the change (r5459). Thanks to the two of you, and to Timo Sirainen, for the help with resolving this issue. Best regards Matthias |
|
From: Matt D. <mat...@pa...> - 2010-01-19 04:20:41
|
Old UID list from mail.papercut.com: <empty> Scratch list of UIDs: <empty> fetchmail: 6.3.13 querying mail.papercut.com (protocol IMAP) at Tue 19 Jan 2010 14:10:14 EST: poll started Trying to connect to 216.92.193.84/143...connected. fetchmail: IMAP< * OK Dovecot ready. fetchmail: IMAP> A0001 CAPABILITY fetchmail: IMAP< * CAPABILITY IMAP4rev1 SASL-IR SORT THREAD=REFERENCES MULTIAPPEND UNSELECT LITERAL+ IDLE CHILDREN NAMESPACE LOGIN-REFERRALS UIDPLUS LIST-EXTENDED I18NLEVEL=1 QUOTA STARTTLS AUTH=PLAIN AUTH=LOGIN fetchmail: IMAP< A0001 OK Capability completed. fetchmail: Protocol identified as IMAP4 rev 1 fetchmail: will idle after poll fetchmail: IMAP> A0002 STARTTLS fetchmail: IMAP< A0002 OK Begin TLS negotiation now. fetchmail: Issuer Organization: The USERTRUST Network fetchmail: Issuer CommonName: UTN-USERFirst-Hardware fetchmail: Server CommonName: *.pair.com fetchmail: Subject Alternative Name: *.pair.com fetchmail: Subject Alternative Name: pair.com fetchmail: mail.papercut.com key fingerprint: 46:1A:36:52:64:4B:71:ED:E8:D6:15:90:45:D7:74:0E fetchmail: IMAP> A0003 CAPABILITY fetchmail: IMAP< * CAPABILITY IMAP4rev1 SASL-IR SORT THREAD=REFERENCES MULTIAPPEND UNSELECT LITERAL+ IDLE CHILDREN NAMESPACE LOGIN-REFERRALS UIDPLUS LIST-EXTENDED I18NLEVEL=1 QUOTA AUTH=PLAIN AUTH=LOGIN fetchmail: IMAP< A0003 OK Capability completed. fetchmail: Protocol identified as IMAP4 rev 1 fetchmail: will idle after poll fetchmail: mail.papercut.com: upgrade to TLS succeeded. fetchmail: IMAP> A0004 LOGIN "matt" * fetchmail: IMAP< A0004 OK Logged in. fetchmail: selecting or re-polling default folder fetchmail: IMAP> A0005 SELECT "INBOX" fetchmail: IMAP< * FLAGS (\Answered \Flagged \Deleted \Seen \Draft Junk NonJunk) fetchmail: IMAP< * OK [PERMANENTFLAGS (\Answered \Flagged \Deleted \Seen \Draft Junk NonJunk \*)] Flags permitted. fetchmail: IMAP< * 0 EXISTS fetchmail: IMAP< * 0 RECENT fetchmail: IMAP< * OK [UIDVALIDITY 1227629703] UIDs valid fetchmail: IMAP< * OK [UIDNEXT 78307] Predicted next UID fetchmail: IMAP< A0005 OK [READ-WRITE] Select completed. fetchmail: 0 messages waiting after first poll fetchmail: IMAP> A0006 IDLE fetchmail: IMAP< + idling fetchmail: IMAP< * 1 EXISTS fetchmail: IMAP> DONE fetchmail: IMAP< * 1 RECENT fetchmail: IMAP< A0006 OK Idle completed. fetchmail: 1 message waiting after re-poll fetchmail: IMAP> A0007 SEARCH UNSEEN NOT DELETED fetchmail: IMAP< * SEARCH 1 fetchmail: 1 is unseen fetchmail: IMAP< * 2 EXISTS fetchmail: IMAP< A0007 OK Search completed (0.000 secs). fetchmail: 1 is first unseen 1 message for matt at mail.papercut.com. fetchmail: IMAP> A0008 FETCH 1 RFC822.SIZE fetchmail: IMAP< * 1 FETCH (RFC822.SIZE 544) fetchmail: IMAP< A0008 OK Fetch completed. fetchmail: IMAP> A0009 FETCH 1 RFC822.HEADER fetchmail: IMAP< * 1 FETCH (RFC822.HEADER {535} reading message ma...@pa...:1 of 1 (535 header octets) About to rewrite Return-Path: <pap...@na...> Rewritten version is Return-Path: <pap...@na...> About to rewrite From: pap...@na... Rewritten version is From: pap...@na... About to rewrite To: mat...@pa... Rewritten version is To: mat...@pa... Trying to connect to 127.0.0.1/25...connected. fetchmail: SMTP< 220 smaug.papercutsoftware.com ESMTP Exim 4.69 Tue, 19 Jan 2010 14:10:30 +1100 fetchmail: SMTP> EHLO smaug.papercutsoftware.com fetchmail: SMTP< 250-smaug.papercutsoftware.com Hello matt at localhost [127.0.0.1] fetchmail: SMTP< 250-SIZE 52428800 fetchmail: SMTP< 250-PIPELINING fetchmail: SMTP< 250 HELP fetchmail: forwarding to localhost fetchmail: SMTP> MAIL FROM:<pap...@na...> SIZE=544 fetchmail: SMTP< 250 OK fetchmail: SMTP> RCPT TO:<matt@localhost> fetchmail: SMTP< 250 Accepted fetchmail: SMTP> DATA fetchmail: SMTP< 354 Enter message, ending with "." on a line by itself # fetchmail: IMAP< ) fetchmail: IMAP< A0009 OK Fetch completed. fetchmail: IMAP> A0010 FETCH 1 BODY.PEEK[TEXT] fetchmail: IMAP< * 1 FETCH (BODY[TEXT] {9} (9 body octets) ** fetchmail: IMAP< ) fetchmail: IMAP< A0010 OK Fetch completed. fetchmail: SMTP>. (EOM) fetchmail: SMTP< 250 OK id=1NX4UA-0000Qs-Le flushed fetchmail: IMAP> A0011 STORE 1 +FLAGS (\Seen \Deleted) fetchmail: IMAP< * 1 FETCH (FLAGS (\Deleted \Seen \Recent)) fetchmail: IMAP< * 2 RECENT fetchmail: IMAP< A0011 OK Store completed. fetchmail: IMAP> A0012 EXPUNGE fetchmail: IMAP< * 1 EXPUNGE fetchmail: IMAP< * 1 RECENT fetchmail: IMAP< A0012 OK Expunge completed. fetchmail: selecting or re-polling default folder fetchmail: 1 message waiting after re-poll fetchmail: IMAP> A0013 SEARCH UNSEEN NOT DELETED fetchmail: IMAP< * SEARCH 1 fetchmail: 1 is unseen fetchmail: IMAP< A0013 OK Search completed (0.000 secs). fetchmail: 1 is first unseen 1 message for matt at mail.papercut.com. fetchmail: IMAP> A0014 FETCH 1 RFC822.SIZE fetchmail: IMAP< * 1 FETCH (RFC822.SIZE 544) fetchmail: IMAP< A0014 OK Fetch completed. fetchmail: IMAP> A0015 FETCH 1 RFC822.HEADER fetchmail: IMAP< * 1 FETCH (RFC822.HEADER {535} reading message ma...@pa...:1 of 1 (535 header octets) About to rewrite Return-Path: <pap...@na...> Rewritten version is Return-Path: <pap...@na...> About to rewrite From: pap...@na... Rewritten version is From: pap...@na... About to rewrite To: mat...@pa... Rewritten version is To: mat...@pa... fetchmail: forwarding to localhost fetchmail: SMTP> MAIL FROM:<pap...@na...> SIZE=544 fetchmail: SMTP< 250 OK fetchmail: SMTP> RCPT TO:<matt@localhost> fetchmail: SMTP< 250 Accepted fetchmail: SMTP> DATA fetchmail: SMTP< 354 Enter message, ending with "." on a line by itself # fetchmail: IMAP< ) fetchmail: IMAP< A0015 OK Fetch completed. fetchmail: IMAP> A0016 FETCH 1 BODY.PEEK[TEXT] fetchmail: IMAP< * 1 FETCH (BODY[TEXT] {9} (9 body octets) ** fetchmail: IMAP< ) fetchmail: IMAP< A0016 OK Fetch completed. fetchmail: SMTP>. (EOM) fetchmail: SMTP< 250 OK id=1NX4UC-0000Qs-IR flushed fetchmail: IMAP> A0017 STORE 1 +FLAGS (\Seen \Deleted) fetchmail: IMAP< * 1 FETCH (FLAGS (\Deleted \Seen \Recent)) fetchmail: IMAP< A0017 OK Store completed. fetchmail: IMAP> A0018 EXPUNGE fetchmail: IMAP< * 1 EXPUNGE fetchmail: IMAP< * 0 RECENT fetchmail: IMAP< A0018 OK Expunge completed. fetchmail: selecting or re-polling default folder fetchmail: SMTP> QUIT fetchmail: SMTP< 221 smaug.papercutsoftware.com closing connection fetchmail: IMAP> A0019 IDLE fetchmail: IMAP< + idling |
|
From: Matt D. <mat...@pa...> - 2010-01-21 12:51:32
|
On 19/01/2010 2:13 PM, Matt Doran wrote: > Sunil Shetye wrote: >> Quoting from Matthias Andree's mail on Thu, Jan 14, 2010: >> >>> Perhaps a separate imap_untagged_parse() function would be more suitable here, >>> and possibly called from inside imap_ok()? See below. >>> >> >> Ok, I have made a separate imap_untagged_response() to parse the >> untagged response. Other key changes: >> >> - use stage == STAGE_GETAUTH&& !strncmp(buf, "* CAPABILITY", 12) >> - instead of strstr(buf, " CAPABILITY") >> >> - use stage == STAGE_GETAUTH&& !strncmp(buf, "* PREAUTH", 9) >> - instead of strstr(buf, " PREAUTH") >> >> - use stage == STAGE_GETRANGE&& !check_only&& strstr(buf, "[READ-ONLY]") >> - instead of !check_only&& strstr(buf, "[READ-ONLY]") >> >> - log "* BYE" message >> >> >> These changes should save a lot of string comparisons during runtime. >> >> >>> 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). >>> >> >> In order to address this, I have made one more intermediate function >> imap_response(). Caller-specific parsers will call imap_response() in >> a loop and others will continue calling imap_ok(). >> >> On reading the RFCs, it looks like the [READ-ONLY] flag is actually >> part of a tagged response. Should the [READ-ONLY] check be moved to >> imap_response()? >> >> Functions cleaned up: >> >> imap_getrange() >> imap_getpartialsizes() >> imap_trail() >> >> >>> 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. >>> >> >> Please check if this patch addresses all the issues raised. >> >> > OK, tested and it looks good. See -v -v output attached. > > Just letting you know I've been running this for a few days now with no problems. Thanks, Matt |
|
From: Matthias A. <mat...@gm...> - 2010-01-21 14:47:49
|
Am 21.01.2010, 12:51 Uhr, schrieb Matt Doran <mat...@pa...>: > On 19/01/2010 2:13 PM, Matt Doran wrote: >> Sunil Shetye wrote: >>> Quoting from Matthias Andree's mail on Thu, Jan 14, 2010: >>> >>>> Perhaps a separate imap_untagged_parse() function would be more >>>> suitable here, >>>> and possibly called from inside imap_ok()? See below. >>>> >>> >>> Ok, I have made a separate imap_untagged_response() to parse the >>> untagged response. Other key changes: >>> >>> - use stage == STAGE_GETAUTH&& !strncmp(buf, "* CAPABILITY", 12) >>> - instead of strstr(buf, " CAPABILITY") >>> >>> - use stage == STAGE_GETAUTH&& !strncmp(buf, "* PREAUTH", 9) >>> - instead of strstr(buf, " PREAUTH") >>> >>> - use stage == STAGE_GETRANGE&& !check_only&& strstr(buf, >>> "[READ-ONLY]") >>> - instead of !check_only&& strstr(buf, "[READ-ONLY]") >>> >>> - log "* BYE" message >>> >>> >>> These changes should save a lot of string comparisons during runtime. >>> >>> >>>> 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). >>>> >>> >>> In order to address this, I have made one more intermediate function >>> imap_response(). Caller-specific parsers will call imap_response() in >>> a loop and others will continue calling imap_ok(). >>> >>> On reading the RFCs, it looks like the [READ-ONLY] flag is actually >>> part of a tagged response. Should the [READ-ONLY] check be moved to >>> imap_response()? >>> >>> Functions cleaned up: >>> >>> imap_getrange() >>> imap_getpartialsizes() >>> imap_trail() >>> >>> >>>> 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. >>>> >>> >>> Please check if this patch addresses all the issues raised. >>> >>> >> OK, tested and it looks good. See -v -v output attached. >> >> > > Just letting you know I've been running this for a few days now with no > problems. Hi Matt, thanks for the continued testing of this new feature, and helping us proceed with the work. Dear Sunil, thanks a lot for your work on this. I hope to audit and possibly commit the patch tonight. Best regards Matthias -- Matthias Andree |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X