GNOME Bugzilla – Bug 504837
When SMTP HELO fails, Evolution silently ignores the error and continues.
Last modified: 2013-09-14 16:50:03 UTC
This is moved from bug 341579, comment 8: When HELO fails, Evolution silently ignores the error and continues. So imagine the following SMTP conversation: << 220 ESMTP >> HELO [10.0.0.1] << 451-Sorry, this server is currently down for maintenance. << 451-Scheduled maintenance downtime is on the last Friday of each month, << 451 between the hours of 22:00-23:00 UTC. >> MAIL FROM:<dwmw2@infradead.org> << 550 You must identify yourself with HELO or EHLO before sending mail Evolution will currently report that final error message, instead of the more useful first error.
Created attachment 101394 [details] [review] proposed eds patch for evolution-data-server; David, there is only one thing I'm not much confident about. Consider the server will always require ESMTP, so it will return something else for the non-ESMTP, but the logic into the code (see my changes) is that if ESMTP fails, then fallback to non-ESMTP. This fallback code is either odd or fine. I do not know. So I kept it there. In case the fallback is nonsense, then I can do this simpler (no new parameter, just disconnect always). Please comment.
The fallback seems correct -- if the server doesn't accept EHLO you should try HELO (unless you need AUTH or TLS, in which case you know HELO isn't good enough). Perhaps it would be cleaner if you disconnect from the caller though, rather than in smtp_helo()? Then you don't need the extra argument.
I'm ok with the extra argument, I just wasn't sure if the fallback is correct. And if you say it is, then I'm just fine. (It's easier to have there that argument than check on all 3 places for the error.) Jeff, can you review/comment this, please? Thanks in advance.
I agree with David's comment, it'd probably be cleaner if the caller did the disconnect on fail rather than the smtp_helo() code itself.
Thx David and Fejj for the comments. Setting the status to needs-work.
Created attachment 101999 [details] [review] proposed eds patch ][ for evolution-data-server; I think the previous patch was better because it kept there the logic of each smtp function which disconnects when the write to transport stream fails, which is just fine, because you do not call the EHLO and then HELO command twice to broken transport stream. On the other hand, this patch is simpler a bit. You decide.
jeff, can you review this, please? Thanks in advance.
it's not necessary to remove the call to camel_service_disconnect() in smtp_helo() as disconnect() is safe to call more than once for smtp it seems. for consistency's sake, it's probably best to leave it as-is. as far as the other changes, I believe it would be better to pass TRUE as the 'clean' argument to the camel_store_disconnect() calls that you added - this way, if smtp_helo() failed with some sort of bad response from the server rather than an i/o error, that we will disconnect cleanly. does that make sense?
To be honest, I didn't get it whole. Your first and second paragraph is against comment #4. About third paragraph, I copied the function call from the smtp_helo itself, as it was before, so it works exactly same, just the function isn't inside, but outside.
not really... let me explain again: smtp_helo() currently only calls camel_service_disconnect() in the event of an i/o error which is not recoverable (afaik) which is also why it sends FALSE as the 'clean' argument, because likely any attempt at sending further commands to the SMTP server will fail anyway. so that should be left alone. your changes to connect_to_server() look correct except that it would be best to call camel_service_disconnect() with a TRUE value for the 'clean' argument in the off chance that the HELO failed by sending an SMTP fail code rather than failing due to an i/o error. by leaving the camel_service_disconnect() call with clean=FALSE in smtp_helo() for i/o errors, calling camel_service_disconnect() at the higher level in connect_to_server() with a clean=TRUE argument, it basically equates to: if (i/o error) camel_service_disconnect (transport, FALSE, ex); else camel_service_disconnect (transport, TRUE, ex); because smtp_disconnect() will only send a QUIT command if it hasn't already been disconnected earlier. hope that explains what I mean a bit better ;)
Created attachment 102155 [details] [review] proposed eds patch ]I[ for evolution-data-server; say something like this? (Note: I didn't test it) I just think that outside (of smtp_helo) call to camel_service_disconnect is there for nothing, because in my initial patch I added it there only to indicate a failure, it was there for nothing else (I didn't want to change the code I changed in the second and third attempt of the patch.) So it can be avoided at all?
yes, this patch looks correct
Committed to trunk. Committed revision 8342.