[FTP] rework data connection code

The new code doesn't rely on funny workaround flags anymore and also
doesn't set workaround flags unconditionally after any failure.
It now works like this:
1) if a default method exists, try the default method.
2) if the default method failed, try all methods in order until one succeeds.
3) if any method succeeded, make it the new default method.
This way, we take the proper method by default, and have a proper
fallback mechanism for flaky connections/servers that sometimes fail to
connect properly.

Also, it's much easier to add new methods (like active FTP) later.

3 files changed, 166 insertions, 80 deletions

diff --git a/daemon/gvfsbackendftp.c b/daemon/gvfsbackendftp.c
index 18c8a7e4..ba6acd43 100644
--- a/daemon/gvfsbackendftp.c
+++ b/daemon/gvfsbackendftp.c
@@ -54,6 +54,23 @@
 #include "gvfsftpfile.h"
 #include "gvfsftptask.h"
 
+/*** GTK DOC ***/
+
+/**
+ * GVfsFtpMethod:
+ * @G_VFS_FTP_METHOD_UNKNOWN: method has not yet been determined
+ * @G_VFS_FTP_METHOD_EPSV: use EPSV command
+ * @G_VFS_FTP_METHOD_PASV: use PASV command
+ * @G_VFS_FTP_METHOD_PASV_ADDR: use PASV command, but ignore the returned 
+ *                              address and only use it's port
+ * @G_VFS_FTP_METHOD_EPRT: use the EPRT command
+ * @G_VFS_FTP_METHOD_PORT: use the PORT command
+ *
+ * Possible methods for creating data connections to the ftp server. If the 
+ * method is @G_VFS_FTP_METHOD_UNKNOWN, all possibilities are tried.
+ */
+
+
 /*
  * about filename interpretation in the ftp backend
  *
@@ -68,6 +85,8 @@
  * paths exactly match those of a TVFS-using FTP server.
  */
 
+/** CODE ***/
+
 G_DEFINE_TYPE (GVfsBackendFtp, g_vfs_backend_ftp, G_VFS_TYPE_BACKEND)
 
 static gboolean
diff --git a/daemon/gvfsbackendftp.h b/daemon/gvfsbackendftp.h
index 4e6b4014..89b2769d 100644
--- a/daemon/gvfsbackendftp.h
+++ b/daemon/gvfsbackendftp.h
@@ -46,13 +46,15 @@ typedef enum {
 } GVfsFtpSystem;
 
 typedef enum {
-  /* Server advertises support for EPSV (or we assume that it supports it),
-   * but it does fail to do so, we set this flag so we can fall back to
-   * PASV. */
-  G_VFS_FTP_WORKAROUND_BROKEN_EPSV,
-  /* Server replies with a wrong address in PASV, we use connection IP
-   * instead */
-  G_VFS_FTP_WORKAROUND_PASV_ADDR,
+  G_VFS_FTP_METHOD_ANY = 0,
+  G_VFS_FTP_METHOD_EPSV,
+  G_VFS_FTP_METHOD_PASV,
+  G_VFS_FTP_METHOD_PASV_ADDR,
+  G_VFS_FTP_METHOD_EPRT,
+  G_VFS_FTP_METHOD_PORT
+} GVfsFtpMethod;
+
+typedef enum {
   /* server does not allow querying features before login, so we try after
    * logging in instead. */
   G_VFS_FTP_WORKAROUND_FEAT_AFTER_LOGIN,
@@ -86,6 +88,7 @@ struct _GVfsBackendFtp
   GVfsFtpSystem         system;                 /* the system from the SYST response */
   int                   features;               /* GVfsFtpFeatures that are supported */
   int                   workarounds;            /* GVfsFtpWorkarounds in use - int because it's atomic */
+  int                   method;                 /* preferred GVfsFtpMethod - int because it's atomic */
 
   /* directory cache */
   const GVfsFtpDirFuncs *dir_funcs;             /* functions used in directory cache */
diff --git a/daemon/gvfsftptask.c b/daemon/gvfsftptask.c
index ac479643..118f80da 100644
--- a/daemon/gvfsftptask.c
+++ b/daemon/gvfsftptask.c
@@ -235,7 +235,7 @@ g_vfs_ftp_task_acquire_connection (GVfsFtpTask *task)
           ftp->max_connections = MIN (ftp->max_connections, maybe_max_connections);
           if (ftp->max_connections == 0)
             {
-              g_debug ("no more connections left, exiting...");
+              g_debug ("no more connections left, exiting...\n");
               /* FIXME: shut down properly */
               exit (0);
             }
@@ -715,6 +715,23 @@ g_vfs_ftp_task_receive (GVfsFtpTask *        task,
   return response;
 }
 
+/**
+ * g_vfs_ftp_task_close_data_connection:
+ * @task: a task potentially having an open data connection
+ *
+ * Closes any data connection @task might have opened.
+ */
+void
+g_vfs_ftp_task_close_data_connection (GVfsFtpTask *task)
+{
+  g_return_if_fail (task != NULL);
+
+  if (task->conn == NULL)
+    return;
+
+  g_vfs_ftp_connection_close_data_connection (task->conn);
+}
+
 static GSocketAddress *
 g_vfs_ftp_task_create_remote_address (GVfsFtpTask *task, guint port)
 {
@@ -729,21 +746,18 @@ g_vfs_ftp_task_create_remote_address (GVfsFtpTask *task, guint port)
   return new;
 }
 
-static gboolean
-g_vfs_ftp_task_open_data_connection_epsv (GVfsFtpTask *task)
+static GVfsFtpMethod
+g_vfs_ftp_task_open_data_connection_epsv (GVfsFtpTask *task, GVfsFtpMethod method)
 {
   const char *s;
   char **reply;
   guint port;
   GSocketAddress *addr;
   guint status;
+  gboolean success;
 
   g_assert (task->error == NULL);
 
-  if (!g_vfs_backend_ftp_has_feature (task->backend, G_VFS_FTP_FEATURE_EPSV) ||
-      g_vfs_backend_ftp_uses_workaround (task->backend, G_VFS_FTP_WORKAROUND_BROKEN_EPSV))
-    return FALSE;
-
   status = g_vfs_ftp_task_send_and_check (task, G_VFS_FTP_PASS_500, NULL, NULL, &reply, "EPSV");
   if (G_VFS_FTP_RESPONSE_GROUP (status) != 2)
     goto fail;
@@ -761,41 +775,34 @@ g_vfs_ftp_task_open_data_connection_epsv (GVfsFtpTask *task)
   g_strfreev (reply);
   addr = g_vfs_ftp_task_create_remote_address (task, port);
   if (addr == NULL)
-    return FALSE;
+    return G_VFS_FTP_METHOD_ANY;
 
-  if (!g_vfs_ftp_connection_open_data_connection (task->conn,
-                                                  addr,
-                                                  task->cancellable,
-                                                  &task->error))
-    {
-      g_object_unref (addr);
-      g_debug ("# Successful EPSV response code, but data connection failed. Enabling FTP_WORKAROUND_BROKEN_EPSV.\n");
-      g_vfs_backend_ftp_use_workaround (task->backend, G_VFS_FTP_WORKAROUND_BROKEN_EPSV);
-      g_vfs_ftp_task_clear_error (task);
-      return FALSE;
-    }
- 
+  success = g_vfs_ftp_connection_open_data_connection (task->conn,
+                                                       addr,
+                                                       task->cancellable,
+                                                       &task->error);
   g_object_unref (addr);
-  return TRUE;
+  return success ? G_VFS_FTP_METHOD_EPSV : G_VFS_FTP_METHOD_ANY;
 
 fail:
   g_strfreev (reply);
-  return FALSE;
+  return G_VFS_FTP_METHOD_ANY;
 }
 
-static gboolean
-g_vfs_ftp_task_open_data_connection_pasv (GVfsFtpTask *task)
+static GVfsFtpMethod
+g_vfs_ftp_task_open_data_connection_pasv (GVfsFtpTask *task, GVfsFtpMethod method)
 {
   guint ip1, ip2, ip3, ip4, port1, port2;
   char **reply;
   const char *s;
   GSocketAddress *addr;
   guint status;
+  gboolean success;
 
   /* only binary transfers please */
   status = g_vfs_ftp_task_send_and_check (task, 0, NULL, NULL, &reply, "PASV");
   if (status == 0)
-    return FALSE;
+    return G_VFS_FTP_METHOD_ANY;
 
   /* parse response and try to find the address to connect to.
    * This code does the same as curl.
@@ -812,10 +819,10 @@ g_vfs_ftp_task_open_data_connection_pasv (GVfsFtpTask *task)
     {
       g_set_error_literal (&task->error, G_IO_ERROR, G_IO_ERROR_FAILED,
         		   _("Invalid reply"));
-      return FALSE;
+      return G_VFS_FTP_METHOD_ANY;
     }
 
-  if (!g_vfs_backend_ftp_uses_workaround (task->backend, G_VFS_FTP_WORKAROUND_PASV_ADDR))
+  if (method == G_VFS_FTP_METHOD_PASV || method == G_VFS_FTP_METHOD_ANY)
     {
       guint8 ip[4];
       GInetAddress *inet_addr;
@@ -828,60 +835,86 @@ g_vfs_ftp_task_open_data_connection_pasv (GVfsFtpTask *task)
       addr = g_inet_socket_address_new (inet_addr, port1 << 8 | port2);
       g_object_unref (inet_addr);
 
-      if (g_vfs_ftp_connection_open_data_connection (task->conn,
-                                                     addr,
-                                                     task->cancellable,
-                                                     &task->error))
-        {
-          g_object_unref (addr);
-          return TRUE;
-        }
-        
+      success = g_vfs_ftp_connection_open_data_connection (task->conn,
+                                                           addr,
+                                                           task->cancellable,
+                                                           &task->error);
       g_object_unref (addr);
-      /* set workaround flag (see below), so we don't try this again */
-      g_debug ("# Successfull PASV response but data connection failed. Enabling FTP_WORKAROUND_PASV_ADDR.\n");
-      g_vfs_backend_ftp_use_workaround (task->backend, G_VFS_FTP_WORKAROUND_PASV_ADDR);
+      if (success)
+        return G_VFS_FTP_METHOD_PASV;
+      if (g_vfs_ftp_task_is_in_error (task) && method != G_VFS_FTP_METHOD_ANY)
+        return G_VFS_FTP_METHOD_ANY;
+
       g_vfs_ftp_task_clear_error (task);
     }
 
-  /* Workaround code:
-   * Various ftp servers aren't setup correctly when behind a NAT. They report
-   * their own IP address (like 10.0.0.4) and not the address in front of the
-   * NAT. But this is likely the same address that we connected to with our
-   * command connetion. So if the address given by PASV fails, we fall back
-   * to the address of the command stream.
-   */
-  addr = g_vfs_ftp_task_create_remote_address (task, port1 << 8 | port2);
-  if (addr == NULL)
-    return FALSE;
-  if (!g_vfs_ftp_connection_open_data_connection (task->conn,
-                                                  addr,
-                                                  task->cancellable,
-                                                  &task->error))
+  if (method == G_VFS_FTP_METHOD_PASV_ADDR || method == G_VFS_FTP_METHOD_ANY)
     {
+      /* Workaround code:
+       * Various ftp servers aren't setup correctly when behind a NAT. They report
+       * their own IP address (like 10.0.0.4) and not the address in front of the
+       * NAT. But this is likely the same address that we connected to with our
+       * command connetion. So if the address given by PASV fails, we fall back
+       * to the address of the command stream.
+       */
+      addr = g_vfs_ftp_task_create_remote_address (task, port1 << 8 | port2);
+      if (addr == NULL)
+        return G_VFS_FTP_METHOD_ANY;
+      success = g_vfs_ftp_connection_open_data_connection (task->conn,
+                                                           addr,
+                                                           task->cancellable,
+                                                           &task->error);
       g_object_unref (addr);
-      return FALSE;
+      if (success)
+        return G_VFS_FTP_METHOD_PASV_ADDR;
     }
 
-  g_object_unref (addr);
-  return TRUE;
+  return G_VFS_FTP_METHOD_ANY;
 }
 
-/**
- * g_vfs_ftp_task_close_data_connection:
- * @task: a task potentially having an open data connection
- *
- * Closes any data connection @task might have opened.
- */
-void
-g_vfs_ftp_task_close_data_connection (GVfsFtpTask *task)
+typedef GVfsFtpMethod (* GVfsFtpOpenDataConnectionFunc) (GVfsFtpTask *task, GVfsFtpMethod method);
+
+static GVfsFtpMethod
+g_vfs_ftp_task_open_data_connection_any (GVfsFtpTask *task, GVfsFtpMethod unused)
 {
-  g_return_if_fail (task != NULL);
+  static const struct {
+    GVfsFtpFeature required_feature;
+    GVfsFtpOpenDataConnectionFunc func;
+  } funcs_ordered[] = {
+    { G_VFS_FTP_FEATURE_EPSV, g_vfs_ftp_task_open_data_connection_epsv },
+    { 0,                      g_vfs_ftp_task_open_data_connection_pasv }
+  };
+  GVfsFtpMethod method;
+  guint i;
+
+  /* first try all advertised features */
+  for (i = 0; i < G_N_ELEMENTS (funcs_ordered); i++)
+    {
+      if (funcs_ordered[i].required_feature &&
+          !g_vfs_backend_ftp_has_feature (task->backend, funcs_ordered[i].required_feature))
+        continue;
+      method = funcs_ordered[i].func (task, G_VFS_FTP_METHOD_ANY);
+      if (method != G_VFS_FTP_METHOD_ANY)
+        return method;
+      
+      g_vfs_ftp_task_clear_error (task);
+    }
 
-  if (task->conn == NULL)
-    return;
+  /* then try if the non-advertised features work */
+  for (i = 0; i < G_N_ELEMENTS (funcs_ordered); i++)
+    {
+      if (!funcs_ordered[i].required_feature ||
+          g_vfs_backend_ftp_has_feature (task->backend, funcs_ordered[i].required_feature))
+        continue;
+      method = funcs_ordered[i].func (task, G_VFS_FTP_METHOD_ANY);
+      if (method != G_VFS_FTP_METHOD_ANY)
+        return method;
+      
+      g_vfs_ftp_task_clear_error (task);
+    }
 
-  g_vfs_ftp_connection_close_data_connection (task->conn);
+  /* finally, just give up */
+  return G_VFS_FTP_METHOD_ANY;
 }
 
 /**
@@ -894,17 +927,48 @@ g_vfs_ftp_task_close_data_connection (GVfsFtpTask *task)
 void
 g_vfs_ftp_task_open_data_connection (GVfsFtpTask *task)
 {
+  static const GVfsFtpOpenDataConnectionFunc connect_funcs[] = {
+    [G_VFS_FTP_METHOD_ANY] = g_vfs_ftp_task_open_data_connection_any,
+    [G_VFS_FTP_METHOD_EPSV] = g_vfs_ftp_task_open_data_connection_epsv,
+    [G_VFS_FTP_METHOD_PASV] = g_vfs_ftp_task_open_data_connection_pasv,
+    [G_VFS_FTP_METHOD_PASV_ADDR] = g_vfs_ftp_task_open_data_connection_pasv,
+    [G_VFS_FTP_METHOD_EPRT] = NULL,
+    [G_VFS_FTP_METHOD_PORT] = NULL
+  };
+  GVfsFtpMethod method, result;
+
   g_return_if_fail (task != NULL);
 
+  /* FIXME: get the method from elsewhere */
+  method = g_atomic_int_get (&task->backend->method);
+  
+  g_assert (method < G_N_ELEMENTS (connect_funcs) && connect_funcs[method]);
+
   if (g_vfs_ftp_task_is_in_error (task))
     return;
 
-  if (g_vfs_ftp_task_open_data_connection_epsv (task))
-    return;
+  result = connect_funcs[method] (task, method);
 
-  if (g_vfs_ftp_task_is_in_error (task))
-    return;
+  /* be sure to try all possibilities if one failed */
+  if (result == G_VFS_FTP_METHOD_ANY &&
+      method != G_VFS_FTP_METHOD_ANY &&
+      !g_vfs_ftp_task_is_in_error (task))
+    result = g_vfs_ftp_task_open_data_connection_any (task, G_VFS_FTP_METHOD_ANY);
 
-  g_vfs_ftp_task_open_data_connection_pasv (task);
+  g_assert (result < G_N_ELEMENTS (connect_funcs) && connect_funcs[result]);
+  if (result != method)
+    {
+      static const char *methods[] = {
+        [G_VFS_FTP_METHOD_ANY] = "any",
+        [G_VFS_FTP_METHOD_EPSV] = "EPSV",
+        [G_VFS_FTP_METHOD_PASV] = "PASV",
+        [G_VFS_FTP_METHOD_PASV_ADDR] = "PASV with workaround",
+        [G_VFS_FTP_METHOD_EPRT] = "EPRT",
+        [G_VFS_FTP_METHOD_PORT] = "PORT"
+      };
+      g_atomic_int_set (&task->backend->method, result);
+      g_debug ("# set default data connection method from %s to %s\n",
+               methods[method], methods[result]);
+    }
 }