Fix too-often retry bug after 4xx with more hosts than hosts_max_retry.

[exim.git] / src / src / transports / smtp.c

diff --git a/src/src/transports/smtp.c b/src/src/transports/smtp.c
index 345fb951b94e712c50a076a1a2581a4bebb02ddc..ac013470b22ab0963467d02688372e99504b2215 100644 (file)
--- a/src/src/transports/smtp.c
+++ b/src/src/transports/smtp.c
@@ -1,4 +1,4 @@
-/* $Cambridge: exim/src/src/transports/smtp.c,v 1.23 2006/02/28 12:42:47 ph10 Exp $ */
+/* $Cambridge: exim/src/src/transports/smtp.c,v 1.28 2006/10/30 16:41:04 ph10 Exp $ */

 
 /*************************************************
 *     Exim - an Internet mail transport agent    *
 
 /*************************************************
 *     Exim - an Internet mail transport agent    *


@@ -193,6 +193,7 @@ smtp_transport_options_block smtp_transport_option_defaults = {
 
 static uschar *smtp_command;   /* Points to last cmd for error messages */
 static uschar *mail_command;   /* Points to MAIL cmd for error messages */
 
 static uschar *smtp_command;   /* Points to last cmd for error messages */
 static uschar *mail_command;   /* Points to MAIL cmd for error messages */

+static BOOL    update_waiting; /* TRUE to update the "wait" database */

 
 
 /*************************************************
 
 
 /*************************************************


@@ -203,8 +204,8 @@ static uschar *mail_command;   /* Points to MAIL cmd for error messages */
 but before running it in a sub-process. It is used for two things:
 
   (1) To set the fallback host list in addresses, when delivering.
 but before running it in a sub-process. It is used for two things:
 
   (1) To set the fallback host list in addresses, when delivering.

-  (2) To pass back the interface, port, and protocol options, for use during
-      callout verification.
+  (2) To pass back the interface, port, protocol, and other options, for use
+      during callout verification.

 
 Arguments:
   tblock    pointer to the transport instance block
 
 Arguments:
   tblock    pointer to the transport instance block


@@ -241,6 +242,7 @@ if (tf != NULL)
   tf->gethostbyname = ob->gethostbyname;
   tf->qualify_single = ob->dns_qualify_single;
   tf->search_parents = ob->dns_search_parents;
   tf->gethostbyname = ob->gethostbyname;
   tf->qualify_single = ob->dns_qualify_single;
   tf->search_parents = ob->dns_search_parents;

+  tf->helo_data = ob->helo_data;

   }
 
 /* Set the fallback host list for all the addresses that don't have fallback
   }
 
 /* Set the fallback host list for all the addresses that don't have fallback


@@ -598,6 +600,12 @@ if (pending_MAIL)
     if (errno == 0 && buffer[0] != 0)
       {
       uschar flushbuffer[4096];
     if (errno == 0 && buffer[0] != 0)
       {
       uschar flushbuffer[4096];

+      int save_errno = 0;
+      if (buffer[0] == '4')
+        {
+        save_errno = ERRNO_MAIL4XX;
+        addr->more_errno |= ((buffer[1] - '0')*10 + buffer[2] - '0') << 8;
+        }

       while (count-- > 0)
         {
         if (!smtp_read_response(inblock, flushbuffer, sizeof(flushbuffer),
       while (count-- > 0)
         {
         if (!smtp_read_response(inblock, flushbuffer, sizeof(flushbuffer),


@@ -605,6 +613,7 @@ if (pending_MAIL)
             && (errno != 0 || flushbuffer[0] == 0))
           break;
         }
             && (errno != 0 || flushbuffer[0] == 0))
           break;
         }

+      errno = save_errno;

       }
     return -3;
     }
       }
     return -3;
     }


@@ -644,7 +653,7 @@ while (count-- > 0)
       transport_rcpt_address(addr, include_affixes));
     set_errno(addrlist, save_errno, message, DEFER, FALSE);
     retry_add_item(addr, addr->address_retry_key, 0);
       transport_rcpt_address(addr, include_affixes));
     set_errno(addrlist, save_errno, message, DEFER, FALSE);
     retry_add_item(addr, addr->address_retry_key, 0);

-    host->update_waiting = FALSE;
+    update_waiting = FALSE;

     return -1;
     }
 
     return -1;
     }
 


@@ -683,20 +692,18 @@ while (count-- > 0)
 
     else
       {
 
     else
       {

-      int bincode = (buffer[1] - '0')*10 + buffer[2] - '0';
-

       addr->transport_return = DEFER;
       addr->basic_errno = ERRNO_RCPT4XX;
       addr->transport_return = DEFER;
       addr->basic_errno = ERRNO_RCPT4XX;

-      addr->more_errno |= bincode << 8;
+      addr->more_errno |= ((buffer[1] - '0')*10 + buffer[2] - '0') << 8;

 
       /* Log temporary errors if there are more hosts to be tried. */
 
       if (host->next != NULL) log_write(0, LOG_MAIN, "%s", addr->message);
 
 
       /* Log temporary errors if there are more hosts to be tried. */
 
       if (host->next != NULL) log_write(0, LOG_MAIN, "%s", addr->message);
 

-      /* Do not put this message on the list of those waiting for this host,
-      as otherwise it is likely to be tried too often. */
+      /* Do not put this message on the list of those waiting for specific
+      hosts, as otherwise it is likely to be tried too often. */

 
 

-      host->update_waiting = FALSE;
+      update_waiting = FALSE;

 
       /* Add a retry item for the address so that it doesn't get tried
       again too soon. */
 
       /* Add a retry item for the address so that it doesn't get tried
       again too soon. */


@@ -720,7 +727,15 @@ if (pending_DATA != 0 &&
   int code;
   uschar *msg;
   BOOL pass_message;
   int code;
   uschar *msg;
   BOOL pass_message;

-  if (pending_DATA > 0 || (yield & 1) != 0) return -3;
+  if (pending_DATA > 0 || (yield & 1) != 0)
+    {
+    if (errno == 0 && buffer[0] == '4')
+      {
+      errno = ERRNO_DATA4XX;
+      addrlist->more_errno |= ((buffer[1] - '0')*10 + buffer[2] - '0') << 8;
+      }
+    return -3;
+    }

   (void)check_response(host, &errno, 0, buffer, &code, &msg, &pass_message);
   DEBUG(D_transport) debug_printf("%s\nerror for DATA ignored: pipelining "
     "is in use and there were no good recipients\n", msg);
   (void)check_response(host, &errno, 0, buffer, &code, &msg, &pass_message);
   DEBUG(D_transport) debug_printf("%s\nerror for DATA ignored: pipelining "
     "is in use and there were no good recipients\n", msg);


@@ -1340,7 +1355,15 @@ switch(rc)
 
   case +1:                /* Block was sent */
   if (!smtp_read_response(&inblock, buffer, sizeof(buffer), '2',
 
   case +1:                /* Block was sent */
   if (!smtp_read_response(&inblock, buffer, sizeof(buffer), '2',

-    ob->command_timeout)) goto RESPONSE_FAILED;
+       ob->command_timeout))
+    {
+    if (errno == 0 && buffer[0] == '4')
+      {
+      errno = ERRNO_MAIL4XX;
+      addrlist->more_errno |= ((buffer[1] - '0')*10 + buffer[2] - '0') << 8;
+      }
+    goto RESPONSE_FAILED;
+    }

   pending_MAIL = FALSE;
   break;
   }
   pending_MAIL = FALSE;
   break;
   }


@@ -1521,8 +1544,16 @@ if (!ok) ok = TRUE; else
   /* For SMTP, we now read a single response that applies to the whole message.
   If it is OK, then all the addresses have been delivered. */
 
   /* For SMTP, we now read a single response that applies to the whole message.
   If it is OK, then all the addresses have been delivered. */
 

-  if (!lmtp) ok = smtp_read_response(&inblock, buffer, sizeof(buffer), '2',
-    ob->final_timeout);
+  if (!lmtp)
+    {
+    ok = smtp_read_response(&inblock, buffer, sizeof(buffer), '2',
+      ob->final_timeout);
+    if (!ok && errno == 0 && buffer[0] == '4')
+      {
+      errno = ERRNO_DATA4XX;
+      addrlist->more_errno |= ((buffer[1] - '0')*10 + buffer[2] - '0') << 8;
+      }
+    }

 
   /* For LMTP, we get back a response for every RCPT command that we sent;
   some may be accepted and some rejected. For those that get a response, their
 
   /* For LMTP, we get back a response for every RCPT command that we sent;
   some may be accepted and some rejected. For those that get a response, their


@@ -1574,7 +1605,8 @@ if (!ok) ok = TRUE; else
 
       /* LMTP - if the response fails badly (e.g. timeout), use it for all the
       remaining addresses. Otherwise, it's a return code for just the one
 
       /* LMTP - if the response fails badly (e.g. timeout), use it for all the
       remaining addresses. Otherwise, it's a return code for just the one

-      address. */
+      address. For temporary errors, add a retry item for the address so that
+      it doesn't get tried again too soon. */

 
       if (lmtp)
         {
 
       if (lmtp)
         {


@@ -1584,7 +1616,16 @@ if (!ok) ok = TRUE; else
           if (errno != 0 || buffer[0] == 0) goto RESPONSE_FAILED;
           addr->message = string_sprintf("LMTP error after %s: %s",
             big_buffer, string_printing(buffer));
           if (errno != 0 || buffer[0] == 0) goto RESPONSE_FAILED;
           addr->message = string_sprintf("LMTP error after %s: %s",
             big_buffer, string_printing(buffer));

-          addr->transport_return = (buffer[0] == '5')? FAIL : DEFER;
+          setflag(addr, af_pass_message);   /* Allow message to go to user */
+          if (buffer[0] == '5')
+            addr->transport_return = FAIL;
+          else
+            {
+            errno = ERRNO_DATA4XX;
+            addr->more_errno |= ((buffer[1] - '0')*10 + buffer[2] - '0') << 8;
+            addr->transport_return = DEFER;
+            retry_add_item(addr, addr->address_retry_key, 0);
+            }

           continue;
           }
         completed_address = TRUE;   /* NOW we can set this flag */
           continue;
           }
         completed_address = TRUE;   /* NOW we can set this flag */


@@ -1686,61 +1727,83 @@ if (!ok)
       }
     }
 
       }
     }
 

-  /* If there was an I/O error or timeout or other transportation error,
-  indicated by errno being non-zero, defer all addresses and yield DEFER,
-  except for the case of failed add_headers expansion, or a transport filter
-  failure, when the yield should be ERROR, to stop it trying other hosts.
-
-  However, handle timeouts after MAIL FROM or "." and loss of connection after
+  /* We want to handle timeouts after MAIL or "." and loss of connection after

   "." specially. They can indicate a problem with the sender address or with
   "." specially. They can indicate a problem with the sender address or with

-  the contents of the message rather than a real error on the connection.
-  Therefore, treat these cases in the same way as a 4xx response.
-
-  The following condition tests for NOT these special cases. */
+  the contents of the message rather than a real error on the connection. These
+  cases are treated in the same way as a 4xx response. This next bit of code
+  does the classification. */

 
 

-  else if (save_errno != 0 &&
-           (save_errno != ETIMEDOUT ||
-             (Ustrncmp(smtp_command,"MAIL",4) != 0 &&
-              Ustrncmp(smtp_command,"end ",4) != 0)) &&
-           (save_errno != ERRNO_SMTPCLOSED ||
-              Ustrncmp(smtp_command,"end ",4) != 0))
+  else

     {
     {

-    yield = (save_errno == ERRNO_CHHEADER_FAIL ||
-             save_errno == ERRNO_FILTER_FAIL)? ERROR : DEFER;
-    set_errno(addrlist, save_errno, message, DEFER, pass_message);
-    }
+    BOOL message_error;

 
 

-  /* Otherwise we have a message-specific error response from the remote
-  host. This is one of
-    (a) negative response or timeout after "mail from"
-    (b) negative response after "data"
-    (c) negative response or timeout or dropped connection after "."
-  It won't be a negative response or timeout after "rcpt to", as that is dealt
-  with separately above. The action in all cases is to set an appropriate
-  error code for all the addresses, but to leave yield set to OK because
-  the host itself has not failed. [It might in practice have failed for a
-  timeout after MAIL FROM, or "." but if so, we'll discover that at the next
-  delivery attempt.] For a temporary error, set the message_defer flag, and
-  write to the logs for information if this is not the last host. The error for
-  the last host will be logged as part of the address's log line. */
+    switch(save_errno)
+      {
+      case 0:
+      case ERRNO_MAIL4XX:
+      case ERRNO_DATA4XX:
+      message_error = TRUE;
+      break;

 
 

-  else
-    {
-    if (mua_wrapper) code = '5';  /* Force hard failure in wrapper mode */
+      case ETIMEDOUT:
+      message_error = Ustrncmp(smtp_command,"MAIL",4) == 0 ||
+                      Ustrncmp(smtp_command,"end ",4) == 0;
+      break;
+
+      case ERRNO_SMTPCLOSED:
+      message_error = Ustrncmp(smtp_command,"end ",4) == 0;
+      break;
+
+      default:
+      message_error = FALSE;
+      break;
+      }
+
+    /* Handle the cases that are treated as message errors. These are:
+
+      (a) negative response or timeout after MAIL
+      (b) negative response after DATA
+      (c) negative response or timeout or dropped connection after "."
+
+    It won't be a negative response or timeout after RCPT, as that is dealt
+    with separately above. The action in all cases is to set an appropriate
+    error code for all the addresses, but to leave yield set to OK because the
+    host itself has not failed. Of course, it might in practice have failed
+    when we've had a timeout, but if so, we'll discover that at the next
+    delivery attempt. For a temporary error, set the message_defer flag, and
+    write to the logs for information if this is not the last host. The error
+    for the last host will be logged as part of the address's log line. */
+
+    if (message_error)
+      {
+      if (mua_wrapper) code = '5';  /* Force hard failure in wrapper mode */
+      set_errno(addrlist, save_errno, message, (code == '5')? FAIL : DEFER,
+        pass_message);

 
 

-    set_errno(addrlist, save_errno, message, (code == '5')? FAIL : DEFER,
-      pass_message);
+      /* If there's an errno, the message contains just the identity of
+      the host. */

 
 

-    /* If there's an errno, the message contains just the identity of
-    the host. */
+      if (code != '5')     /* Anything other than 5 is treated as temporary */
+        {
+        if (save_errno > 0)
+          message = US string_sprintf("%s: %s", message, strerror(save_errno));
+        if (host->next != NULL) log_write(0, LOG_MAIN, "%s", message);
+        deliver_msglog("%s %s\n", tod_stamp(tod_log), message);
+        *message_defer = TRUE;
+        }
+      }
+
+    /* Otherwise, we have an I/O error or a timeout other than after MAIL or
+    ".", or some other transportation error. We defer all addresses and yield
+    DEFER, except for the case of failed add_headers expansion, or a transport
+    filter failure, when the yield should be ERROR, to stop it trying other
+    hosts. */

 
 

-    if (code != '5')     /* Anything other than 5 is treated as temporary */
+    else

       {
       {

-      if (save_errno > 0)
-        message = US string_sprintf("%s: %s", message, strerror(save_errno));
-      if (host->next != NULL) log_write(0, LOG_MAIN, "%s", message);
-      deliver_msglog("%s %s\n", tod_stamp(tod_log), message);
-      *message_defer = TRUE;
+      yield = (save_errno == ERRNO_CHHEADER_FAIL ||
+               save_errno == ERRNO_FILTER_FAIL)? ERROR : DEFER;
+      set_errno(addrlist, save_errno, message, DEFER, pass_message);

       }
     }
   }
       }
     }
   }


@@ -2040,6 +2103,13 @@ DEBUG(D_transport)
       continue_hostname, continue_host_address);
   }
 
       continue_hostname, continue_host_address);
   }
 

+/* Set the flag requesting that these hosts be added to the waiting
+database if the delivery fails temporarily or if we are running with
+queue_smtp or a 2-stage queue run. This gets unset for certain
+kinds of error, typically those that are specific to the message. */
+
+update_waiting =  TRUE;
+

 /* If a host list is not defined for the addresses - they must all have the
 same one in order to be passed to a single transport - or if the transport has
 a host list with hosts_override set, use the host list supplied with the
 /* If a host list is not defined for the addresses - they must all have the
 same one in order to be passed to a single transport - or if the transport has
 a host list with hosts_override set, use the host list supplied with the


@@ -2226,13 +2296,6 @@ for (cutoff_retry = 0; expired &&
 
     nexthost = host->next;
 
 
     nexthost = host->next;
 

-    /* Set the flag requesting that this host be added to the waiting
-    database if the delivery fails temporarily or if we are running with
-    queue_smtp or a 2-stage queue run. This gets unset for certain
-    kinds of error, typically those that are specific to the message. */
-
-    host->update_waiting = TRUE;
-

     /* If the address hasn't yet been obtained from the host name, look it up
     now, unless the host is already marked as unusable. If it is marked as
     unusable, it means that the router was unable to find its IP address (in
     /* If the address hasn't yet been obtained from the host name, look it up
     now, unless the host is already marked as unusable. If it is marked as
     unusable, it means that the router was unable to find its IP address (in


@@ -2249,7 +2312,7 @@ for (cutoff_retry = 0; expired &&
 
     if (host->address == NULL)
       {
 
     if (host->address == NULL)
       {

-      int new_port;
+      int new_port, flags;

       host_item *hh;
       uschar *canonical_name;
 
       host_item *hh;
       uschar *canonical_name;
 


@@ -2274,16 +2337,15 @@ for (cutoff_retry = 0; expired &&
       /* Find by name if so configured, or if it's an IP address. We don't
       just copy the IP address, because we need the test-for-local to happen. */
 
       /* Find by name if so configured, or if it's an IP address. We don't
       just copy the IP address, because we need the test-for-local to happen. */
 

+      flags = HOST_FIND_BY_A;
+      if (ob->dns_qualify_single) flags |= HOST_FIND_QUALIFY_SINGLE;
+      if (ob->dns_search_parents) flags |= HOST_FIND_SEARCH_PARENTS;
+

       if (ob->gethostbyname || string_is_ip_address(host->name, NULL) != 0)
       if (ob->gethostbyname || string_is_ip_address(host->name, NULL) != 0)

-        rc = host_find_byname(host, NULL, &canonical_name, TRUE);
+        rc = host_find_byname(host, NULL, flags, &canonical_name, TRUE);

       else
       else

-        {
-        int flags = HOST_FIND_BY_A;
-        if (ob->dns_qualify_single) flags |= HOST_FIND_QUALIFY_SINGLE;
-        if (ob->dns_search_parents) flags |= HOST_FIND_SEARCH_PARENTS;

         rc = host_find_bydns(host, NULL, flags, NULL, NULL, NULL,
           &canonical_name, NULL);
         rc = host_find_bydns(host, NULL, flags, NULL, NULL, NULL,
           &canonical_name, NULL);

-        }

 
       /* Update the host (and any additional blocks, resulting from
       multihoming) with a host-specific port, if any. */
 
       /* Update the host (and any additional blocks, resulting from
       multihoming) with a host-specific port, if any. */


@@ -2443,9 +2505,9 @@ for (cutoff_retry = 0; expired &&
 
         /* If there was a retry message key, implying that previously there
         was a message-specific defer, we don't want to update the list of
 
         /* If there was a retry message key, implying that previously there
         was a message-specific defer, we don't want to update the list of

-        messages waiting for this host. */
+        messages waiting for these hosts. */

 
 

-        if (retry_message_key != NULL) host->update_waiting = FALSE;
+        if (retry_message_key != NULL) update_waiting = FALSE;

         continue;   /* With the next host or IP address */
         }
       }
         continue;   /* With the next host or IP address */
         }
       }


@@ -2677,7 +2739,7 @@ for (cutoff_retry = 0; expired &&
     to the retry chain. Note that if there was a message defer but now there is
     a host defer, the message defer record gets deleted. That seems perfectly
     reasonable. Also, stop the message from being remembered as waiting
     to the retry chain. Note that if there was a message defer but now there is
     a host defer, the message defer record gets deleted. That seems perfectly
     reasonable. Also, stop the message from being remembered as waiting

-    for this host. */
+    for specific hosts. */

 
     if (message_defer || retry_message_key != NULL)
       {
 
     if (message_defer || retry_message_key != NULL)
       {


@@ -2691,7 +2753,7 @@ for (cutoff_retry = 0; expired &&
         }
       retry_add_item(addrlist, retry_message_key,
         rf_message | rf_host | delete_flag);
         }
       retry_add_item(addrlist, retry_message_key,
         rf_message | rf_host | delete_flag);

-      host->update_waiting = FALSE;
+      update_waiting = FALSE;

       }
 
     /* Any return other than DEFER (that is, OK or ERROR) means that the
       }
 
     /* Any return other than DEFER (that is, OK or ERROR) means that the


@@ -2870,11 +2932,11 @@ for (addr = addrlist; addr != NULL; addr = addr->next)
   }
 
 /* Update the database which keeps information about which messages are waiting
   }
 
 /* Update the database which keeps information about which messages are waiting

-for which hosts to become available. Each host in the list has a flag which is
-set if the data is to be updated. For some message-specific errors, the flag is
-turned off because we don't want follow-on deliveries in those cases. */
+for which hosts to become available. For some message-specific errors, the
+update_waiting flag is turned off because we don't want follow-on deliveries in
+those cases. */

 
 

-transport_update_waiting(hostlist, tblock->name);
+if (update_waiting) transport_update_waiting(hostlist, tblock->name);

 
 END_TRANSPORT:
 
 
 END_TRANSPORT: