Skip to contentSkip to conversation

slirp: tftp server improvements

PATCH Superseded Hervé Poussineau 2 participants 7 messages Sep 10, 2012

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

v2 cover slirp: tftp server improvements

These patches have already been sent in April 2011, and contain some fixes for the internal TFTP server. With these patches, MS Windows PE can be booted via PXE, and 32MB file limitation has been removed.

This has been tested with MS Windows 2003 PXE boot client, PXELINUX and gPXE.

Indentation seems a little bit off in patch 3/3, because surrounding code indentation is one tab followed by spaces. I've indented new lines by replacing the tab by 8 spaces. Other patches also have indentation errors reported by checkpatch.pl, but I kept indentation consistent.

Changes v1 -> v2
- rebased
- updated with Aurélien Jarno remarks
- improved commit messages

Hervé Poussineau (3):
slirp: improve TFTP performance
slirp: Handle more than 65535 blocks in TFTP transfers
slirp: Implement TFTP Blocksize option

 slirp/tftp.c |   81 +++++++++++++++++++++++++++++++++++++++-------------------
 slirp/tftp.h |    2 ++
 2 files changed, 57 insertions(+), 26 deletions(-)
Signature · 1 line
1.7.10.4
#1 lore raw

PATCH v2 1/3 slirp: improve TFTP performance

When transfering a file, keep it open during the whole transfer, instead of opening/closing it for each block.

---
 slirp/tftp.c |   32 ++++++++++++++++++--------------
 slirp/tftp.h |    1 +
 2 files changed, 19 insertions(+), 14 deletions(-)
slirp/tftp.c +18 -14
index b78765f..520dbd6 100644
@@ -37,6 +37,10 @@ static inline void tftp_session_update(struct tftp_session *spt)
 
 static void tftp_session_terminate(struct tftp_session *spt)
 {
+    if (spt->fd >= 0) {
+        close(spt->fd);
+        spt->fd = -1;
+    }
     g_free(spt->filename);
     spt->slirp = NULL;
 }
@@ -54,7 +58,7 @@ static int tftp_session_allocate(Slirp *slirp, struct tftp_t *tp)
 
     /* sessions time out after 5 inactive seconds */
     if ((int)(curtime - spt->timestamp) > 5000) {
-        g_free(spt->filename);
+        tftp_session_terminate(spt);
         goto found;
     }
   }
@@ -64,6 +68,7 @@ static int tftp_session_allocate(Slirp *slirp, struct tftp_t *tp)
  found:
   memset(spt, 0, sizeof(*spt));
   memcpy(&spt->client_ip, &tp->ip.ip_src, sizeof(spt->client_ip));
+  spt->fd = -1;
   spt->client_port = tp->udp.uh_sport;
   spt->slirp = slirp;
 
@@ -95,24 +100,23 @@ static int tftp_session_find(Slirp *slirp, struct tftp_t *tp)
 static int tftp_read_data(struct tftp_session *spt, uint16_t block_nr,
                           uint8_t *buf, int len)
 {
-  int fd;
-  int bytes_read = 0;
-
-  fd = open(spt->filename, O_RDONLY | O_BINARY);
+    int bytes_read = 0;
 
-  if (fd < 0) {
-    return -1;
-  }
+    if (spt->fd < 0) {
+        spt->fd = open(spt->filename, O_RDONLY | O_BINARY);
+    }
 
-  if (len) {
-    lseek(fd, block_nr * 512, SEEK_SET);
+    if (spt->fd < 0) {
+        return -1;
+    }
 
-    bytes_read = read(fd, buf, len);
-  }
+    if (len) {
+        lseek(spt->fd, block_nr * 512, SEEK_SET);
 
-  close(fd);
+        bytes_read = read(spt->fd, buf, len);
+    }
 
-  return bytes_read;
+    return bytes_read;
 }
 
 static int tftp_send_oack(struct tftp_session *spt,
slirp/tftp.h +1 -0
index 72e5e91..9c364ea 100644
@@ -33,6 +33,7 @@ struct tftp_t {
 struct tftp_session {
     Slirp *slirp;
     char *filename;
+    int fd;
 
     struct in_addr client_ip;
     uint16_t client_port;
Signature · 1 line
1.7.10.4
#2 lore raw

Re: [Qemu-devel] [PATCH v2 1/3] slirp: improve TFTP performance

Quoted 85 lines · Hervé Poussineau

When transfering a file, keep it open during the whole transfer, instead of opening/closing it for each block.

---
 slirp/tftp.c |   32 ++++++++++++++++++--------------
 slirp/tftp.h |    1 +
 2 files changed, 19 insertions(+), 14 deletions(-)
slirp/tftp.c +18 -14
index b78765f..520dbd6 100644
@@ -37,6 +37,10 @@ static inline void tftp_session_update(struct tftp_session *spt)
 
 static void tftp_session_terminate(struct tftp_session *spt)
 {
+    if (spt->fd >= 0) {
+        close(spt->fd);
+        spt->fd = -1;
+    }
     g_free(spt->filename);
     spt->slirp = NULL;
 }
@@ -54,7 +58,7 @@ static int tftp_session_allocate(Slirp *slirp, struct tftp_t *tp)
 
     /* sessions time out after 5 inactive seconds */
     if ((int)(curtime - spt->timestamp) > 5000) {
-        g_free(spt->filename);
+        tftp_session_terminate(spt);
         goto found;
     }
   }
@@ -64,6 +68,7 @@ static int tftp_session_allocate(Slirp *slirp, struct tftp_t *tp)
  found:
   memset(spt, 0, sizeof(*spt));
   memcpy(&spt->client_ip, &tp->ip.ip_src, sizeof(spt->client_ip));
+  spt->fd = -1;
   spt->client_port = tp->udp.uh_sport;
   spt->slirp = slirp;
 
@@ -95,24 +100,23 @@ static int tftp_session_find(Slirp *slirp, struct tftp_t *tp)
 static int tftp_read_data(struct tftp_session *spt, uint16_t block_nr,
                           uint8_t *buf, int len)
 {
-  int fd;
-  int bytes_read = 0;
-
-  fd = open(spt->filename, O_RDONLY | O_BINARY);
+    int bytes_read = 0;
 
-  if (fd < 0) {
-    return -1;
-  }
+    if (spt->fd < 0) {
+        spt->fd = open(spt->filename, O_RDONLY | O_BINARY);
+    }
 
-  if (len) {
-    lseek(fd, block_nr * 512, SEEK_SET);
+    if (spt->fd < 0) {
+        return -1;
+    }
 
-    bytes_read = read(fd, buf, len);
-  }
+    if (len) {
+        lseek(spt->fd, block_nr * 512, SEEK_SET);
 
-  close(fd);
+        bytes_read = read(spt->fd, buf, len);
+    }
 
-  return bytes_read;
+    return bytes_read;
 }
 
 static int tftp_send_oack(struct tftp_session *spt,
slirp/tftp.h +0 -0
index 72e5e91..9c364ea 100644

slirp/tftp.h struct tftp_t { · line 39

Quoted 7 lines
slirp/tftp.h +1 -0
@@ -33,6 +33,7 @@ struct tftp_t {
 struct tftp_session {
     Slirp *slirp;
     char *filename;
+    int fd;
 
     struct in_addr client_ip;
     uint16_t client_port;

Thanks, added to slirp queue.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
#3 lore raw

PATCH v2 2/3 slirp: Handle more than 65535 blocks in TFTP transfers

In reply to Hervé Poussineau

RFC 1350 does not mention block count roll-over. However, a lot of TFTP servers implement it to be able to transmit big files, so do it also.

Current block size is 512 bytes, so TFTP files were limited to 32 MB.

---
 slirp/tftp.c |    9 +++++----
 slirp/tftp.h |    1 +
 2 files changed, 6 insertions(+), 4 deletions(-)
slirp/tftp.c +5 -4
index 520dbd6..75c9030 100644
@@ -97,7 +97,7 @@ static int tftp_session_find(Slirp *slirp, struct tftp_t *tp)
   return -1;
 }
 
-static int tftp_read_data(struct tftp_session *spt, uint16_t block_nr,
+static int tftp_read_data(struct tftp_session *spt, uint32_t block_nr,
                           uint8_t *buf, int len)
 {
     int bytes_read = 0;
@@ -198,7 +198,7 @@ out:
 }
 
 static int tftp_send_data(struct tftp_session *spt,
-                          uint16_t block_nr,
+                          uint32_t block_nr,
 			  struct tftp_t *recv_tp)
 {
   struct sockaddr_in saddr, daddr;
@@ -223,7 +223,7 @@ static int tftp_send_data(struct tftp_session *spt,
   m->m_data += sizeof(struct udpiphdr);
 
   tp->tp_op = htons(TFTP_DATA);
-  tp->x.tp_data.tp_block_nr = htons(block_nr);
+  tp->x.tp_data.tp_block_nr = htons(block_nr & 0xffff);
 
   saddr.sin_addr = recv_tp->ip.ip_dst;
   saddr.sin_port = recv_tp->udp.uh_dport;
@@ -255,6 +255,7 @@ static int tftp_send_data(struct tftp_session *spt,
     tftp_session_terminate(spt);
   }
 
+  spt->block_nr = block_nr;
   return 0;
 }
 
@@ -387,7 +388,7 @@ static void tftp_handle_ack(Slirp *slirp, struct tftp_t *tp, int pktlen)
   }
 
   if (tftp_send_data(&slirp->tftp_sessions[s],
-		     ntohs(tp->x.tp_data.tp_block_nr) + 1,
+		     slirp->tftp_sessions[s].block_nr + 1,
 		     tp) < 0) {
     return;
   }
slirp/tftp.h +1 -0
index 9c364ea..51704e4 100644
@@ -37,6 +37,7 @@ struct tftp_session {
 
     struct in_addr client_ip;
     uint16_t client_port;
+    uint32_t block_nr;
 
     int timestamp;
 };
Signature · 1 line
1.7.10.4
#4 lore raw

Re: [Qemu-devel] [PATCH v2 2/3] slirp: Handle more than 65535 blocks in TFTP transfers

Quoted 44 lines · Hervé Poussineau

RFC 1350 does not mention block count roll-over. However, a lot of TFTP servers implement it to be able to transmit big files, so do it also.

Current block size is 512 bytes, so TFTP files were limited to 32 MB.

---
 slirp/tftp.c |    9 +++++----
 slirp/tftp.h |    1 +
 2 files changed, 6 insertions(+), 4 deletions(-)
slirp/tftp.c +3 -3
index 520dbd6..75c9030 100644
@@ -97,7 +97,7 @@ static int tftp_session_find(Slirp *slirp, struct tftp_t *tp)
   return -1;
 }
 
-static int tftp_read_data(struct tftp_session *spt, uint16_t block_nr,
+static int tftp_read_data(struct tftp_session *spt, uint32_t block_nr,
                           uint8_t *buf, int len)
 {
     int bytes_read = 0;
@@ -198,7 +198,7 @@ out:
 }
 
 static int tftp_send_data(struct tftp_session *spt,
-                          uint16_t block_nr,
+                          uint32_t block_nr,
 			  struct tftp_t *recv_tp)
 {
   struct sockaddr_in saddr, daddr;
@@ -223,7 +223,7 @@ static int tftp_send_data(struct tftp_session *spt,
   m->m_data += sizeof(struct udpiphdr);
 
   tp->tp_op = htons(TFTP_DATA);
-  tp->x.tp_data.tp_block_nr = htons(block_nr);
+  tp->x.tp_data.tp_block_nr = htons(block_nr & 0xffff);
 
   saddr.sin_addr = recv_tp->ip.ip_dst;
   saddr.sin_port = recv_tp->udp.uh_dport;

slirp/tftp.c static int tftp_send_data(struct tftp_session *spt, · line 258

Quoted 4 lines
slirp/tftp.c +1 -0
@@ -255,6 +255,7 @@ static int tftp_send_data(struct tftp_session *spt,
     tftp_session_terminate(spt);
   }
 
+  spt->block_nr = block_nr;

That's not really a nice interface: You pass in tftp_session::block_nr as argument but you also manipulate it here. I would vote for some pure
variant: either implement a

void tftp_send_next_block(struct tftp_session *spt,
struct tftp_t *recv_tp)

, initializing tftp_session::block_nr to 0 before first call and incrementing it inside on success. Or do the maintenance of block_nr completely outside. I leaning a bit to variant 1.

Quoted 18 lines
slirp/tftp.c +1 -1
   return 0;
 }
 
@@ -387,7 +388,7 @@ static void tftp_handle_ack(Slirp *slirp, struct tftp_t *tp, int pktlen)
   }
 
   if (tftp_send_data(&slirp->tftp_sessions[s],
-		     ntohs(tp->x.tp_data.tp_block_nr) + 1,
+		     slirp->tftp_sessions[s].block_nr + 1,
 		     tp) < 0) {
     return;
   }
slirp/tftp.h +0 -0
index 9c364ea..51704e4 100644

slirp/tftp.h struct tftp_session { · line 43

Quoted 7 lines
slirp/tftp.h +1 -0
@@ -37,6 +37,7 @@ struct tftp_session {
 
     struct in_addr client_ip;
     uint16_t client_port;
+    uint32_t block_nr;
 
     int timestamp;
 };

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
#5 lore raw

PATCH v2 3/3 slirp: Implement TFTP Blocksize option

In reply to Hervé Poussineau

This option is described in RFC 1783. As this is only an optional field, we may ignore it in some situations and handle it in some others.

However, MS Windows 2003 PXE boot client requests a block size of the MTU (most of the times 1472 bytes), and doesn't work if the option is not acknowledged (with whatever value).

According to the RFC 1783, we cannot acknowledge the option with a bigger value than the requested one.

As current implementation is using 512 bytes by block, accept the option with a value of 512 if the option was specified, and don't acknowledge it if it is not present or less than 512 bytes.

---
 slirp/tftp.c |   40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)
slirp/tftp.c +32 -8
index 75c9030..e02a29b 100644
@@ -120,13 +120,13 @@ static int tftp_read_data(struct tftp_session *spt, uint32_t block_nr,
 }
 
 static int tftp_send_oack(struct tftp_session *spt,
-                          const char *key, uint32_t value,
+                          const char *keys[], uint32_t values[], int nb,
                           struct tftp_t *recv_tp)
 {
     struct sockaddr_in saddr, daddr;
     struct mbuf *m;
     struct tftp_t *tp;
-    int n = 0;
+    int i, n = 0;
 
     m = m_get(spt->slirp);
 
@@ -140,10 +140,12 @@ static int tftp_send_oack(struct tftp_session *spt,
     m->m_data += sizeof(struct udpiphdr);
 
     tp->tp_op = htons(TFTP_OACK);
-    n += snprintf(tp->x.tp_buf + n, sizeof(tp->x.tp_buf) - n, "%s",
-                  key) + 1;
-    n += snprintf(tp->x.tp_buf + n, sizeof(tp->x.tp_buf) - n, "%u",
-                  value) + 1;
+    for (i = 0; i < nb; i++) {
+        n += snprintf(tp->x.tp_buf + n, sizeof(tp->x.tp_buf) - n, "%s",
+                      keys[i]) + 1;
+        n += snprintf(tp->x.tp_buf + n, sizeof(tp->x.tp_buf) - n, "%u",
+                      values[i]) + 1;
+    }
 
     saddr.sin_addr = recv_tp->ip.ip_dst;
     saddr.sin_port = recv_tp->udp.uh_dport;
@@ -265,6 +267,9 @@ static void tftp_handle_rrq(Slirp *slirp, struct tftp_t *tp, int pktlen)
   int s, k;
   size_t prefix_len;
   char *req_fname;
+  const char *option_name[2];
+  uint32_t option_value[2];
+  int nb_options = 0;
 
   /* check if a session already exists and if so terminate it */
   s = tftp_session_find(slirp, tp);
@@ -369,11 +374,30 @@ static void tftp_handle_rrq(Slirp *slirp, struct tftp_t *tp, int pktlen)
 	      }
 	  }
 
-	  tftp_send_oack(spt, "tsize", tsize, tp);
-	  return;
+          option_name[nb_options] = "tsize";
+          option_value[nb_options] = tsize;
+          nb_options++;
+      } else if (strcasecmp(key, "blksize") == 0) {
+          int blksize = atoi(value);
+
+          /* If blksize option is bigger than what we will
+           * emit, accept the option with our packet size.
+           * Otherwise, simply do as we didn't see the option.
+           */
+          if (blksize >= 512) {
+              option_name[nb_options] = "blksize";
+              option_value[nb_options] = 512;
+              nb_options++;
+          }
       }
   }
 
+  if (nb_options > 0) {
+    assert(nb_options <= sizeof(option_name) / sizeof(option_name[0]));
+    tftp_send_oack(spt, option_name, option_value, nb_options, tp);
+    return;
+  }
+
   tftp_send_data(spt, 1, tp);
 }
 
Signature · 1 line
1.7.10.4
#6 lore raw

Re: [Qemu-devel] [PATCH v2 3/3] slirp: Implement TFTP Blocksize option

Quoted 82 lines · Hervé Poussineau

This option is described in RFC 1783. As this is only an optional field, we may ignore it in some situations and handle it in some others.

However, MS Windows 2003 PXE boot client requests a block size of the MTU (most of the times 1472 bytes), and doesn't work if the option is not acknowledged (with whatever value).

According to the RFC 1783, we cannot acknowledge the option with a bigger value than the requested one.

As current implementation is using 512 bytes by block, accept the option with a value of 512 if the option was specified, and don't acknowledge it if it is not present or less than 512 bytes.

---
 slirp/tftp.c |   40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)
slirp/tftp.c +21 -8
index 75c9030..e02a29b 100644
@@ -120,13 +120,13 @@ static int tftp_read_data(struct tftp_session *spt, uint32_t block_nr,
 }
 
 static int tftp_send_oack(struct tftp_session *spt,
-                          const char *key, uint32_t value,
+                          const char *keys[], uint32_t values[], int nb,
                           struct tftp_t *recv_tp)
 {
     struct sockaddr_in saddr, daddr;
     struct mbuf *m;
     struct tftp_t *tp;
-    int n = 0;
+    int i, n = 0;
 
     m = m_get(spt->slirp);
 
@@ -140,10 +140,12 @@ static int tftp_send_oack(struct tftp_session *spt,
     m->m_data += sizeof(struct udpiphdr);
 
     tp->tp_op = htons(TFTP_OACK);
-    n += snprintf(tp->x.tp_buf + n, sizeof(tp->x.tp_buf) - n, "%s",
-                  key) + 1;
-    n += snprintf(tp->x.tp_buf + n, sizeof(tp->x.tp_buf) - n, "%u",
-                  value) + 1;
+    for (i = 0; i < nb; i++) {
+        n += snprintf(tp->x.tp_buf + n, sizeof(tp->x.tp_buf) - n, "%s",
+                      keys[i]) + 1;
+        n += snprintf(tp->x.tp_buf + n, sizeof(tp->x.tp_buf) - n, "%u",
+                      values[i]) + 1;
+    }
 
     saddr.sin_addr = recv_tp->ip.ip_dst;
     saddr.sin_port = recv_tp->udp.uh_dport;
@@ -265,6 +267,9 @@ static void tftp_handle_rrq(Slirp *slirp, struct tftp_t *tp, int pktlen)
   int s, k;
   size_t prefix_len;
   char *req_fname;
+  const char *option_name[2];
+  uint32_t option_value[2];
+  int nb_options = 0;
 
   /* check if a session already exists and if so terminate it */
   s = tftp_session_find(slirp, tp);
@@ -369,11 +374,30 @@ static void tftp_handle_rrq(Slirp *slirp, struct tftp_t *tp, int pktlen)
 	      }
 	  }
 
-	  tftp_send_oack(spt, "tsize", tsize, tp);
-	  return;
+          option_name[nb_options] = "tsize";
+          option_value[nb_options] = tsize;
+          nb_options++;
+      } else if (strcasecmp(key, "blksize") == 0) {
+          int blksize = atoi(value);
+
+          /* If blksize option is bigger than what we will
+           * emit, accept the option with our packet size.
+           * Otherwise, simply do as we didn't see the option.
+           */

slirp/tftp.c static void tftp_handle_rrq(Slirp *slirp, struct tftp_t *tp, int pktlen) · line 396

Quoted 10 lines
slirp/tftp.c +7 -0
+          if (blksize >= 512) {
+              option_name[nb_options] = "blksize";
+              option_value[nb_options] = 512;
+              nb_options++;
+          }
       }
   }
 
+  if (nb_options > 0) {
+    assert(nb_options <= sizeof(option_name) / sizeof(option_name[0]));

What prevents that an evil guest triggers this? BTW, we have ARRAY_SIZE.

slirp/tftp.c static void tftp_handle_rrq(Slirp *slirp, struct tftp_t *tp, int pktlen) · line 398

Quoted 2 lines
slirp/tftp.c +2 -0
+    tftp_send_oack(spt, option_name, option_value, nb_options, tp);
+    return;

tftp_handle_rrq isn't consistent, but let's indent by 4 relative to the condition.

slirp/tftp.c static void tftp_handle_rrq(Slirp *slirp, struct tftp_t *tp, int pktlen) · line 403

Quoted 6 lines
slirp/tftp.c +2 -0
+  }
+
   tftp_send_data(spt, 1, tp);
 }
 

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux