tftp: explain "block# 0" codepath; report our decision to bail out to server

if blocksize option doesn't look good (it was a FIXME. +33 bytes code);
make code more readable.

1 files changed, 70 insertions, 73 deletions

diff --git a/networking/tftp.c b/networking/tftp.c
index 1f1dfff..3fb76ec 100644
--- a/networking/tftp.c
+++ b/networking/tftp.c
@@ -37,15 +37,15 @@
 #define TFTP_OACK  6
 
 #if ENABLE_FEATURE_TFTP_GET && !ENABLE_FEATURE_TFTP_PUT
-#define USE_GETPUT(a)
+#define USE_GETPUT(...)
 #define CMD_GET(cmd) 1
 #define CMD_PUT(cmd) 0
 #elif !ENABLE_FEATURE_TFTP_GET && ENABLE_FEATURE_TFTP_PUT
-#define USE_GETPUT(a)
+#define USE_GETPUT(...)
 #define CMD_GET(cmd) 0
 #define CMD_PUT(cmd) 1
 #else
-#define USE_GETPUT(a) a
+#define USE_GETPUT(...) __VA_ARGS__
 /* masks coming from getpot32 */
 #define CMD_GET(cmd) ((cmd) & 1)
 #define CMD_PUT(cmd) ((cmd) & 2)
@@ -109,10 +109,7 @@ static char *tftp_option_get(char *buf, int len, const char *option)
 
 #endif
 
-static int tftp(
-#if ENABLE_FEATURE_TFTP_GET && ENABLE_FEATURE_TFTP_PUT
-		const int cmd,
-#endif
+static int tftp( USE_GETPUT(const int cmd,)
 		len_and_sockaddr *peer_lsa,
 		const char *remotefile, const int localfd,
 		unsigned port, int tftp_bufsize)
@@ -181,6 +178,9 @@ static int tftp(
 	/* First packet is built, so skip packet generation */
 	goto send_pkt;
 
+	/* Using mostly goto's - continue/break will be less clear
+	 * in where we actually jump to */
+
 	while (1) {
 		/* Build ACK or DATA */
 		cp = xbuf + 2;
@@ -203,67 +203,64 @@ static int tftp(
  send_pkt:
 		/* Send packet */
 		*((uint16_t*)xbuf) = htons(opcode); /* fill in opcode part */
-		timeout = TFTP_NUM_RETRIES;	/* re-initialize */
-		while (1) {
-			send_len = cp - xbuf;
-			/* nb: need to preserve send_len value in code below
-			 * for potential resend! */
+		send_len = cp - xbuf;
+		/* NB: send_len value is preserved in code below
+		 * for potential resend */
  send_again:
 #if ENABLE_DEBUG_TFTP
-			fprintf(stderr, "sending %u bytes\n", send_len);
-			for (cp = xbuf; cp < &xbuf[send_len]; cp++)
-				fprintf(stderr, "%02x ", (unsigned char) *cp);
-			fprintf(stderr, "\n");
+		fprintf(stderr, "sending %u bytes\n", send_len);
+		for (cp = xbuf; cp < &xbuf[send_len]; cp++)
+			fprintf(stderr, "%02x ", (unsigned char) *cp);
+		fprintf(stderr, "\n");
 #endif
-			xsendto(socketfd, xbuf, send_len, &peer_lsa->sa, peer_lsa->len);
-			/* Was it final ACK? then exit */
-			if (finished && (opcode == TFTP_ACK))
-				goto ret;
+		xsendto(socketfd, xbuf, send_len, &peer_lsa->sa, peer_lsa->len);
+		/* Was it final ACK? then exit */
+		if (finished && (opcode == TFTP_ACK))
+			goto ret;
 
-			/* Receive packet */
+		timeout = TFTP_NUM_RETRIES;	/* re-initialize */
  recv_again:
-			tv.tv_sec = TFTP_TIMEOUT;
-			tv.tv_usec = 0;
-			FD_ZERO(&rfds);
-			FD_SET(socketfd, &rfds);
-			switch (select(socketfd + 1, &rfds, NULL, NULL, &tv)) {
-				unsigned from_port;
-			case 1:
-				from->len = peer_lsa->len;
-				memset(&from->sa, 0, peer_lsa->len);
-				len = recvfrom(socketfd, rbuf, tftp_bufsize, 0,
-							&from->sa, &from->len);
-				if (len < 0) {
-					bb_perror_msg("recvfrom");
-					goto ret;
-				}
-				from_port = get_nport(&from->sa);
-				if (port == org_port) {
-					/* Our first query went to port 69
-					 * but reply will come from different one.
-					 * Remember and use this new port */
-					port = from_port;
-					set_nport(peer_lsa, from_port);
-				}
-				if (port != from_port)
-					goto recv_again;
-				goto recvd_good;
-			case 0:
-				timeout--;
-				if (timeout == 0) {
-					bb_error_msg("last timeout");
-					goto ret;
-				}
-				bb_error_msg("last timeout" + 5);
-				goto send_again; /* resend last sent pkt */
-			default:
-				bb_perror_msg("select");
+		/* Receive packet */
+		tv.tv_sec = TFTP_TIMEOUT;
+		tv.tv_usec = 0;
+		FD_ZERO(&rfds);
+		FD_SET(socketfd, &rfds);
+		switch (select(socketfd + 1, &rfds, NULL, NULL, &tv)) {
+			unsigned from_port;
+		case 1:
+			from->len = peer_lsa->len;
+			memset(&from->sa, 0, peer_lsa->len);
+			len = recvfrom(socketfd, rbuf, tftp_bufsize, 0,
+						&from->sa, &from->len);
+			if (len < 0) {
+				bb_perror_msg("recvfrom");
 				goto ret;
 			}
-		} /* while we don't see recv packet with correct port# */
-
+			from_port = get_nport(&from->sa);
+			if (port == org_port) {
+				/* Our first query went to port 69
+				 * but reply will come from different one.
+				 * Remember and use this new port */
+				port = from_port;
+				set_nport(peer_lsa, from_port);
+			}
+			if (port != from_port)
+				goto recv_again;
+			goto process_pkt;
+		case 0:
+			timeout--;
+			if (timeout == 0) {
+				bb_error_msg("last timeout");
+				goto ret;
+			}
+			bb_error_msg("last timeout" + 5);
+			goto send_again; /* resend last sent pkt */
+		default:
+			bb_perror_msg("select");
+			goto ret;
+		}
+ process_pkt:
 		/* Process recv'ed packet */
- recvd_good:
 		opcode = ntohs( ((uint16_t*)rbuf)[0] );
 		recv_blk = ntohs( ((uint16_t*)rbuf)[1] );
 
@@ -281,9 +278,9 @@ static int tftp(
 				"unknown transfer id",
 				"file already exists",
 				"no such user",
+				"bad option",
 			};
 			enum { NUM_ERRCODE = sizeof(errcode_str) / sizeof(errcode_str[0]) };
-
 			const char *msg = "";
 
 			if (rbuf[4] != '\0') {
@@ -308,8 +305,13 @@ static int tftp(
 				if (res) {
 					int blksize = xatoi_u(res);
 					if (!tftp_blocksize_check(blksize, tftp_bufsize - 4)) {
+						/* send ERROR 8 to server... */
+						/* htons can be impossible to use in const initializer: */
+						/*static const uint16_t error_8[2] = { htons(TFTP_ERROR), htons(8) };*/
+						/* thus we open-code big-endian layout */
+						static const char error_8[4] = { 0,TFTP_ERROR, 0,8 };
+						xsendto(socketfd, error_8, 4, &peer_lsa->sa, peer_lsa->len);
 						bb_error_msg("server proposes bad blksize %d, exiting", blksize);
-						// FIXME: must also send ERROR 8 to server...
 						goto ret;
 					}
 #if ENABLE_DEBUG_TFTP
@@ -317,7 +319,8 @@ static int tftp(
 							blksize);
 #endif
 					tftp_bufsize = blksize + 4;
-					block_nr = 0; // TODO: explain why???
+					/* Send ACK for OACK ("block" no: 0) */
+					block_nr = 0;
 					continue;
 				}
 				/* rfc2347:
@@ -430,9 +433,8 @@ int tftp_main(int argc, char **argv)
 	if (!remotefile || !argv[0])
 		bb_show_usage();
 
-	if (LONE_DASH(localfile)) {
-		fd = CMD_GET(cmd) ? STDOUT_FILENO : STDIN_FILENO;
-	} else {
+	fd = CMD_GET(cmd) ? STDOUT_FILENO : STDIN_FILENO;
+	if (!LONE_DASH(localfile)) {
 		fd = xopen(localfile, flags);
 	}
 
@@ -440,17 +442,12 @@ int tftp_main(int argc, char **argv)
 	peer_lsa = xhost2sockaddr(argv[0], port);
 
 #if ENABLE_DEBUG_TFTP
-	fprintf(stderr, "using server \"%s\", "
-			"remotefile \"%s\", localfile \"%s\"\n",
+	fprintf(stderr, "using server '%s', remotefile '%s', localfile '%s'\n",
 			xmalloc_sockaddr2dotted(&peer_lsa->sa, peer_lsa->len),
 			remotefile, localfile);
 #endif
 
-	result = tftp(
-#if ENABLE_FEATURE_TFTP_GET && ENABLE_FEATURE_TFTP_PUT
-		cmd,
-#endif
-		peer_lsa, remotefile, fd, port, blocksize);
+	result = tftp( USE_GETPUT(cmd,) peer_lsa, remotefile, fd, port, blocksize);
 
 	if (ENABLE_FEATURE_CLEAN_UP)
 		close(fd);