[Qemu-devel] [PATCH] slirp: support dynamic block size for TFTP transfers

[Qemu-devel] [PATCH] slirp: support dynamic block size for TFTP transfers

[Hervé Poussineau]

The blocksize option is defined in RFC 1783.
We now support block sizes between 1 and 1432 bytes, instead of 512 only.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 slirp/tftp.c | 26 ++++++++++++++------------
 slirp/tftp.h |  8 +++++---
 2 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/slirp/tftp.c b/slirp/tftp.c
index c185906..d53a657 100644
--- a/slirp/tftp.c
+++ b/slirp/tftp.c
@@ -72,6 +72,7 @@ static int tftp_session_allocate(Slirp *slirp, struct sockaddr_storage *srcsas,
   memset(spt, 0, sizeof(*spt));
   spt->client_addr = *srcsas;
   spt->fd = -1;
+  spt->block_size = 512;
   spt->client_port = tp->udp.uh_sport;
   spt->slirp = slirp;
 
@@ -115,7 +116,7 @@ static int tftp_read_data(struct tftp_session *spt, uint32_t block_nr,
     }
 
     if (len) {
-        lseek(spt->fd, block_nr * 512, SEEK_SET);
+        lseek(spt->fd, block_nr * spt->block_size, SEEK_SET);
 
         bytes_read = read(spt->fd, buf, len);
     }
@@ -189,7 +190,8 @@ static int tftp_send_oack(struct tftp_session *spt,
                       values[i]) + 1;
     }
 
-    m->m_len = sizeof(struct tftp_t) - 514 + n - sizeof(struct udphdr);
+    m->m_len = sizeof(struct tftp_t) - (TFTP_BLOCKSIZE_MAX + 2) + n
+               - sizeof(struct udphdr);
     tftp_udp_output(spt, m, recv_tp);
 
     return 0;
@@ -214,7 +216,7 @@ static void tftp_send_error(struct tftp_session *spt,
   tp->x.tp_error.tp_error_code = htons(errorcode);
   pstrcpy((char *)tp->x.tp_error.tp_msg, sizeof(tp->x.tp_error.tp_msg), msg);
 
-  m->m_len = sizeof(struct tftp_t) - 514 + 3 + strlen(msg)
+  m->m_len = sizeof(struct tftp_t) - (TFTP_BLOCKSIZE_MAX + 2) + 3 + strlen(msg)
              - sizeof(struct udphdr);
   tftp_udp_output(spt, m, recv_tp);
 
@@ -240,7 +242,8 @@ static void tftp_send_next_block(struct tftp_session *spt,
   tp->tp_op = htons(TFTP_DATA);
   tp->x.tp_data.tp_block_nr = htons((spt->block_nr + 1) & 0xffff);
 
-  nobytes = tftp_read_data(spt, spt->block_nr, tp->x.tp_data.tp_buf, 512);
+  nobytes = tftp_read_data(spt, spt->block_nr, tp->x.tp_data.tp_buf,
+                           spt->block_size);
 
   if (nobytes < 0) {
     m_free(m);
@@ -252,10 +255,11 @@ static void tftp_send_next_block(struct tftp_session *spt,
     return;
   }
 
-  m->m_len = sizeof(struct tftp_t) - (512 - nobytes) - sizeof(struct udphdr);
+  m->m_len = sizeof(struct tftp_t) - (TFTP_BLOCKSIZE_MAX - nobytes)
+             - sizeof(struct udphdr);
   tftp_udp_output(spt, m, recv_tp);
 
-  if (nobytes == 512) {
+  if (nobytes == spt->block_size) {
     tftp_session_update(spt);
   }
   else {
@@ -385,13 +389,11 @@ static void tftp_handle_rrq(Slirp *slirp, struct sockaddr_storage *srcsas,
       } 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) {
+          /* Accept blksize up to our maximum size */
+          if (blksize > 0) {
+              spt->block_size = min(blksize, TFTP_BLOCKSIZE_MAX);
               option_name[nb_options] = "blksize";
-              option_value[nb_options] = 512;
+              option_value[nb_options] = spt->block_size;
               nb_options++;
           }
       }
diff --git a/slirp/tftp.h b/slirp/tftp.h
index 2cd276d..9446dbc 100644
--- a/slirp/tftp.h
+++ b/slirp/tftp.h
@@ -15,6 +15,7 @@
 #define TFTP_OACK   6
 
 #define TFTP_FILENAME_MAX 512
+#define TFTP_BLOCKSIZE_MAX 1432
 
 struct tftp_t {
   struct udphdr udp;
@@ -22,13 +23,13 @@ struct tftp_t {
   union {
     struct {
       uint16_t tp_block_nr;
-      uint8_t tp_buf[512];
+      uint8_t tp_buf[TFTP_BLOCKSIZE_MAX];
     } tp_data;
     struct {
       uint16_t tp_error_code;
-      uint8_t tp_msg[512];
+      uint8_t tp_msg[TFTP_BLOCKSIZE_MAX];
     } tp_error;
-    char tp_buf[512 + 2];
+    char tp_buf[TFTP_BLOCKSIZE_MAX + 2];
   } x;
 } __attribute__((packed));
 
@@ -36,6 +37,7 @@ struct tftp_session {
     Slirp *slirp;
     char *filename;
     int fd;
+    uint16_t block_size;
 
     struct sockaddr_storage client_addr;
     uint16_t client_port;
-- 
2.1.4

Re: [Qemu-devel] [PATCH] slirp: support dynamic block size for TFTP transfers

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH] slirp: support dynamic block size for TFTP transfers
Type: series
Message-id: 1479757549-18672-1-git-send-email-hpoussin@reactos.org

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/1479749488-31808-1-git-send-email-kwolf@redhat.com -> patchew/1479749488-31808-1-git-send-email-kwolf@redhat.com
 * [new tag]         patchew/1479757549-18672-1-git-send-email-hpoussin@reactos.org -> patchew/1479757549-18672-1-git-send-email-hpoussin@reactos.org
Switched to a new branch 'test'
f12a58c slirp: support dynamic block size for TFTP transfers

=== OUTPUT BEGIN ===
Checking PATCH 1/1: slirp: support dynamic block size for TFTP transfers...
ERROR: suspect code indent for conditional statements (10, 14)
#89: FILE: slirp/tftp.c:393:
+          if (blksize > 0) {
+              spt->block_size = min(blksize, TFTP_BLOCKSIZE_MAX);

total: 1 errors, 0 warnings, 101 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH] slirp: support dynamic block size for TFTP transfers

[H. Peter Anvin]

On 11/21/16 11:45, Hervé Poussineau wrote:
> The blocksize option is defined in RFC 1783.
> We now support block sizes between 1 and 1432 bytes, instead of 512 only.

It ought to be 1476: Ethernet MTU = 1500, minus a minimum of 20 bytes
for an IPv4 header and 4 for a TFTP header.

Some bootloaders including Syslinux sandbag this value to avoid creating
fragmented packets in case there is a VPN or something similar in the
network path.

	-hpa

Re: [Qemu-devel] [PATCH] slirp: support dynamic block size for TFTP transfers

[Thomas Huth]

On 21.11.2016 20:51, H. Peter Anvin wrote:
> On 11/21/16 11:45, Hervé Poussineau wrote:
>> The blocksize option is defined in RFC 1783.
>> We now support block sizes between 1 and 1432 bytes, instead of 512 only.
> 
> It ought to be 1476: Ethernet MTU = 1500, minus a minimum of 20 bytes
> for an IPv4 header and 4 for a TFTP header.

Don't forget the size of the UDP header - so the maximum value is 1468.
However, if you want to play safe, you should also consider that
somebody is putting additional options into the IPv4 header, so the IPv4
header can be up to 60 bytes instead of only 20 bytes. So a real safe
value for the maximum TFTP block size is:

 1500 - 60 - 8 - 4 = 1428

This is also what is mentioned in RFC2348 which obsoletes RFC1783 (that
mentions 1432).

 Thomas

Re: [Qemu-devel] [PATCH] slirp: support dynamic block size for TFTP transfers

[Samuel Thibault]

Hervé Poussineau, on Mon 21 Nov 2016 20:45:49 +0100, wrote:
> The blocksize option is defined in RFC 1783.
> We now support block sizes between 1 and 1432 bytes, instead of 512 only.
> 
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>

I fixed a couple of trivial things and commited to my tree, thanks!

Samuel