ftpgetput: add comment about EPSV (extended PASV).

Fix bug where we were using lstat instead of stat.
Added many TODOs.

1 files changed, 65 insertions, 39 deletions

diff --git a/networking/ftpgetput.c b/networking/ftpgetput.c
index a1ee05426..3a9930cd7 100644
--- a/networking/ftpgetput.c
+++ b/networking/ftpgetput.c
@@ -68,29 +68,6 @@ static int ftpcmd(const char *s1, const char *s2, FILE *stream, char *buf)
 	return n;
 }
 
-static int xconnect_ftpdata(ftp_host_info_t *server, char *buf)
-{
-	char *buf_ptr;
-	unsigned port_num;
-
-	/* Response is "NNN garbageN1,N2,N3,N4,P1,P2[)garbage]
-	 * Server's IP is N1.N2.N3.N4 (we ignore it)
-	 * Server's port for data connection is P1*256+P2 */
-	buf_ptr = strrchr(buf, ')');
-	if (buf_ptr) *buf_ptr = '\0';
-
-	buf_ptr = strrchr(buf, ',');
-	*buf_ptr = '\0';
-	port_num = xatoul_range(buf_ptr + 1, 0, 255);
-
-	buf_ptr = strrchr(buf, ',');
-	*buf_ptr = '\0';
-	port_num += xatoul_range(buf_ptr + 1, 0, 255) * 256;
-
-	set_nport(server->lsa, htons(port_num));
-	return xconnect_stream(server->lsa);
-}
-
 static FILE *ftp_login(ftp_host_info_t *server)
 {
 	FILE *control_stream;
@@ -125,6 +102,29 @@ static FILE *ftp_login(ftp_host_info_t *server)
 	return control_stream;
 }
 
+static int xconnect_ftpdata(ftp_host_info_t *server, char *buf)
+{
+	char *buf_ptr;
+	unsigned port_num;
+
+	/* Response is "NNN garbageN1,N2,N3,N4,P1,P2[)garbage]
+	 * Server's IP is N1.N2.N3.N4 (we ignore it)
+	 * Server's port for data connection is P1*256+P2 */
+	buf_ptr = strrchr(buf, ')');
+	if (buf_ptr) *buf_ptr = '\0';
+
+	buf_ptr = strrchr(buf, ',');
+	*buf_ptr = '\0';
+	port_num = xatoul_range(buf_ptr + 1, 0, 255);
+
+	buf_ptr = strrchr(buf, ',');
+	*buf_ptr = '\0';
+	port_num += xatoul_range(buf_ptr + 1, 0, 255) * 256;
+
+	set_nport(server->lsa, htons(port_num));
+	return xconnect_stream(server->lsa);
+}
+
 #if !ENABLE_FTPGET
 int ftp_receive(ftp_host_info_t *server, FILE *control_stream,
 		const char *local_path, char *server_path);
@@ -141,7 +141,25 @@ int ftp_receive(ftp_host_info_t *server, FILE *control_stream,
 	int fd_local = -1;
 	off_t beg_range = 0;
 
-	/* Connect to the data socket */
+/*
+TODO: PASV command will not work for IPv6. RFC2428 describes
+IPv6-capable "extended PASV" - EPSV.
+
+"EPSV [protocol]" asks server to bind to and listen on a data port
+in specified protocol. Protocol is 1 for IPv4, 2 for IPv6.
+If not specified, defaults to "same as used for control connection".
+If server understood you, it should answer "229 <some text>(|||port|)"
+where "|" are literal pipe chars and "port" is ASCII decimal port#.
+
+There is also an IPv6-capable replacement for PORT (EPRT),
+but we don't need that.
+
+TODO: fold in sending of PASV/EPSV and parsing of response into
+xconnect_ftpdata(). (Also, need to stop ignoring IP address in PASV
+response).
+*/
+
+	/* connect to the data socket */
 	if (ftpcmd("PASV", NULL, control_stream, buf) != 227) {
 		ftp_die("PASV", buf);
 	}
@@ -162,8 +180,9 @@ int ftp_receive(ftp_host_info_t *server, FILE *control_stream,
 
 	if (do_continue) {
 		struct stat sbuf;
-		if (lstat(local_path, &sbuf) < 0) {
-			bb_perror_msg_and_die("lstat");
+		/* lstat would be wrong here! */
+		if (stat(local_path, &sbuf) < 0) {
+			bb_perror_msg_and_die("stat");
 		}
 		if (sbuf.st_size > 0) {
 			beg_range = sbuf.st_size;
@@ -186,22 +205,25 @@ int ftp_receive(ftp_host_info_t *server, FILE *control_stream,
 		ftp_die("RETR", buf);
 	}
 
-	/* only make a local file if we know that one exists on the remote server */
+	/* make local _after_ we know that remote file exists */
 	if (fd_local == -1) {
-		if (do_continue) {
-			fd_local = xopen(local_path, O_APPEND | O_WRONLY);
-		} else {
-			fd_local = xopen(local_path, O_CREAT | O_TRUNC | O_WRONLY);
-		}
+		fd_local = xopen(local_path,
+			do_continue ? (O_APPEND | O_WRONLY)
+			            : (O_CREAT | O_TRUNC | O_WRONLY)
+		);
 	}
 
-	/* Copy the file */
-	if (filesize != -1) {
+// TODO: merge tail of ftp_receive and ftp_send starting from here
+
+	/* copy the file */
+	if (filesize != -1) { // NEVER HAPPENS, filesize is always -1
 		if (bb_copyfd_size(fd_data, fd_local, filesize) == -1)
 			return EXIT_FAILURE;
 	} else {
-		if (bb_copyfd_eof(fd_data, fd_local) == -1)
+		if (bb_copyfd_eof(fd_data, fd_local) == -1) {
+			/* error msg is already printed by bb_copyfd_eof */
 			return EXIT_FAILURE;
+		}
 	}
 
 	/* close it all down */
@@ -229,7 +251,7 @@ int ftp_send(ftp_host_info_t *server, FILE *control_stream,
 	int fd_local;
 	int response;
 
-	/*  Connect to the data socket */
+	/* connect to the data socket */
 	if (ftpcmd("PASV", NULL, control_stream, buf) != 227) {
 		ftp_die("PASV", buf);
 	}
@@ -241,6 +263,9 @@ int ftp_send(ftp_host_info_t *server, FILE *control_stream,
 		fd_local = xopen(local_path, O_RDONLY);
 		fstat(fd_local, &sbuf);
 
+// TODO: do we really need to send ALLO? It's ancient...
+// Doesn't it break "ftpput .. .. fifo" too?
+
 		sprintf(buf, "ALLO %"OFF_FMT"u", sbuf.st_size);
 		response = ftpcmd(buf, NULL, control_stream, buf);
 		switch (response) {
@@ -248,7 +273,7 @@ int ftp_send(ftp_host_info_t *server, FILE *control_stream,
 		case 202:
 			break;
 		default:
-			close(fd_local);
+			close(fd_local); // TODO: why bother?
 			ftp_die("ALLO", buf);
 			break;
 		}
@@ -259,13 +284,14 @@ int ftp_send(ftp_host_info_t *server, FILE *control_stream,
 	case 150:
 		break;
 	default:
-		close(fd_local);
+		close(fd_local); // TODO: why bother?
 		ftp_die("STOR", buf);
 	}
 
 	/* transfer the file  */
 	if (bb_copyfd_eof(fd_local, fd_data) == -1) {
-		exit(EXIT_FAILURE);
+		/* error msg is already printed by bb_copyfd_eof */
+		return EXIT_FAILURE;
 	}
 
 	/* close it all down */