non-blocking active FTP: cleanup multi state usage

Backpedaled out the funny double-change of state in the multi state
machine by adding a new argument to the do_more() function to signal
completion. This way it can remain in the DO_MORE state properly until
done. Long term, the entire DO_MORE logic should be moved into the FTP
code and be hidden from the multi code as the logic is only used for
FTP.

lib/ftp.c

@@ -134,7 +134,7 @@ static CURLcode ftp_done(struct connectdata *conn,
                          CURLcode, bool premature);
 static CURLcode ftp_connect(struct connectdata *conn, bool *done);
 static CURLcode ftp_disconnect(struct connectdata *conn, bool dead_connection);
-static CURLcode ftp_nextconnect(struct connectdata *conn);
+static CURLcode ftp_do_more(struct connectdata *conn, bool *completed);
 static CURLcode ftp_multi_statemach(struct connectdata *conn, bool *done);
 static int ftp_getsock(struct connectdata *conn, curl_socket_t *socks,
                        int numsocks);
@@ -173,7 +173,7 @@ const struct Curl_handler Curl_handler_ftp = {
   ftp_setup_connection,            /* setup_connection */
   ftp_do,                          /* do_it */
   ftp_done,                        /* done */
-  ftp_nextconnect,                 /* do_more */
+  ftp_do_more,                     /* do_more */
   ftp_connect,                     /* connect_it */
   ftp_multi_statemach,             /* connecting */
   ftp_doing,                       /* doing */
@@ -200,7 +200,7 @@ const struct Curl_handler Curl_handler_ftps = {
   ftp_setup_connection,            /* setup_connection */
   ftp_do,                          /* do_it */
   ftp_done,                        /* done */
-  ftp_nextconnect,                 /* do_more */
+  ftp_do_more,                     /* do_more */
   ftp_connect,                     /* connect_it */
   ftp_multi_statemach,             /* connecting */
   ftp_doing,                       /* doing */
@@ -2356,6 +2356,8 @@ static CURLcode ftp_state_stor_resp(struct connectdata *conn,
 
     if(!connected) {
       infof(data, "Data conn was not available immediately\n");
+      /* as there's not necessarily an immediate action on the control
+         connection now, we halt the state machine */
       state(conn, FTP_STOP);
       conn->bits.wait_data_conn = TRUE;
     }
@@ -3615,22 +3617,33 @@ static CURLcode ftp_range(struct connectdata *conn)
 
 
 /*
- * ftp_nextconnect()
+ * ftp_do_more()
  *
  * This function shall be called when the second FTP (data) connection is
  * connected.
  */
 
-static CURLcode ftp_nextconnect(struct connectdata *conn)
+static CURLcode ftp_do_more(struct connectdata *conn, bool *complete)
 {
   struct SessionHandle *data=conn->data;
   struct ftp_conn *ftpc = &conn->proto.ftpc;
   CURLcode result = CURLE_OK;
+  bool connected = FALSE;
 
   /* the ftp struct is inited in ftp_connect() */
   struct FTP *ftp = data->state.proto.ftp;
 
-  DEBUGF(infof(data, "DO-MORE phase starts\n"));
+  /* if the second connection isn't done yet, wait for it */
+  if(!conn->bits.tcpconnect[SECONDARYSOCKET]) {
+    result = Curl_is_connected(conn, SECONDARYSOCKET, &connected);
+
+    /* Ready to do more? */
+    if(connected) {
+      DEBUGF(infof(data, "DO-MORE connected phase starts\n"));
+    }
+    else
+      return result;
+  }
 
   if(ftp->transfer <= FTPTRANSFER_INFO) {
     /* a transfer is about to take place, or if not a file name was given
@@ -3692,7 +3705,11 @@ static CURLcode ftp_nextconnect(struct connectdata *conn)
        too! */
     Curl_setup_transfer(conn, -1, -1, FALSE, NULL, -1, NULL);
 
-  DEBUGF(infof(data, "DO-MORE phase ends with %d\n", (int)result));
+  if(!conn->bits.wait_data_conn) {
+    /* no waiting for the data connection so this is now complete */
+    *complete = TRUE;
+    DEBUGF(infof(data, "DO-MORE phase ends with %d\n", (int)result));
+  }
 
   return result;
 }
@@ -3974,6 +3991,7 @@ static CURLcode ftp_do(struct connectdata *conn, bool *done)
   CURLcode retcode = CURLE_OK;
 
   *done = FALSE; /* default to false */
+  conn->bits.wait_data_conn = FALSE; /* default to no such wait */
 
   /*
     Since connections can be re-used between SessionHandles, this might be a
@@ -4343,8 +4361,10 @@ static CURLcode ftp_dophase_done(struct connectdata *conn,
   struct FTP *ftp = conn->data->state.proto.ftp;
   struct ftp_conn *ftpc = &conn->proto.ftpc;
 
-  if(connected)
-    result = ftp_nextconnect(conn);
+  if(connected) {
+    bool completed;
+    result = ftp_do_more(conn, &completed);
+  }
 
   if(result && (conn->sock[SECONDARYSOCKET] != CURL_SOCKET_BAD)) {
     /* Failure detected, close the second socket if it was created already */

lib/multi.c

@@ -1361,40 +1361,31 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
       break;
 
     case CURLM_STATE_DO_MORE:
-      /* Ready to do more? */
-      easy->result = Curl_is_connected(easy->easy_conn,
-                                       SECONDARYSOCKET,
-                                       &connected);
-      if(connected) {
-        /*
-         * When we are connected, DO MORE and then go DO_DONE
-         */
-        easy->result = Curl_do_more(easy->easy_conn);
-
-        /* No need to remove ourselves from the send pipeline here since that
-           is done for us in Curl_done() */
+      /*
+       * When we are connected, DO MORE and then go DO_DONE
+       */
+      easy->result = Curl_do_more(easy->easy_conn, &dophase_done);
 
-        if(CURLE_OK == easy->result) {
+      /* No need to remove this handle from the send pipeline here since that
+         is done in Curl_done() */
+      if(CURLE_OK == easy->result) {
+        if(dophase_done) {
           multistate(easy, CURLM_STATE_DO_DONE);
           result = CURLM_CALL_MULTI_PERFORM;
         }
-        else {
-          /* failure detected */
-          Curl_posttransfer(data);
-          Curl_done(&easy->easy_conn, easy->result, FALSE);
-          disconnect_conn = TRUE;
-        }
+        else
+          /* stay in DO_MORE */
+          result = CURLM_OK;
+      }
+      else {
+        /* failure detected */
+        Curl_posttransfer(data);
+        Curl_done(&easy->easy_conn, easy->result, FALSE);
+        disconnect_conn = TRUE;
       }
       break;
 
     case CURLM_STATE_DO_DONE:
-
-      if(easy->easy_conn->bits.wait_data_conn == TRUE) {
-        multistate(easy, CURLM_STATE_DO_MORE);
-        result = CURLM_OK;
-        break;
-      }
-
       /* Move ourselves from the send to recv pipeline */
       moveHandleFromSendToRecvPipeline(data, easy->easy_conn);
       /* Check if we can move pending requests to send pipe */

lib/url.c

@@ -5457,14 +5457,25 @@ CURLcode Curl_do(struct connectdata **connp, bool *done)
   return result;
 }
 
-CURLcode Curl_do_more(struct connectdata *conn)
+/*
+ * Curl_do_more() is called during the DO_MORE multi state. It is basically a
+ * second stage DO state which (wrongly) was introduced to support FTP's
+ * second connection.
+ *
+ * TODO: A future libcurl should be able to work away this state.
+ *
+ */
+
+CURLcode Curl_do_more(struct connectdata *conn, bool *completed)
 {
   CURLcode result=CURLE_OK;
 
+  *completed = FALSE;
+
   if(conn->handler->do_more)
-    result = conn->handler->do_more(conn);
+    result = conn->handler->do_more(conn, completed);
 
-  if(result == CURLE_OK && conn->bits.wait_data_conn == FALSE)
+  if(!result && completed)
     /* do_complete must be called after the protocol-specific DO function */
     do_complete(conn);
 

lib/url.h

@@ -37,7 +37,7 @@ CURLcode Curl_close(struct SessionHandle *data); /* opposite of curl_open() */
 CURLcode Curl_connect(struct SessionHandle *, struct connectdata **,
                       bool *async, bool *protocol_connect);
 CURLcode Curl_do(struct connectdata **, bool *done);
-CURLcode Curl_do_more(struct connectdata *);
+CURLcode Curl_do_more(struct connectdata *, bool *completed);
 CURLcode Curl_done(struct connectdata **, CURLcode, bool premature);
 CURLcode Curl_disconnect(struct connectdata *, bool dead_connection);
 CURLcode Curl_protocol_connect(struct connectdata *conn, bool *done);

lib/urldata.h

@@ -512,7 +512,7 @@ struct Curl_async {
 /* These function pointer types are here only to allow easier typecasting
    within the source when we need to cast between data pointers (such as NULL)
    and function pointers. */
-typedef CURLcode (*Curl_do_more_func)(struct connectdata *);
+typedef CURLcode (*Curl_do_more_func)(struct connectdata *, bool *);
 typedef CURLcode (*Curl_done_func)(struct connectdata *, CURLcode, bool);