| Message ID | 1479757549-18672-1-git-send-email-hpoussin@reactos.org |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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;
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(-)