On 10/9/24 17:26, Ilias Apalodimas wrote:
Hi Jerome,
On Wed, 9 Oct 2024 at 17:50, Jerome Forissier jerome.forissier@linaro.org wrote:
The TFTP protocol uses a default block size of 512 bytes. This value is sub-optimal for ethernet devices, which have a MTU (Maximum Transmission Unit) of 1500 bytes. When taking into acount the overhead of the IP and UDP layers, this leaves 1468 bytes for the TFTP payload.
This patch introduces a new function: tftp_client_set_blksize() which may be used to change the block size from the default. It has to be called after tftp_client_init() and before tftp_get(). If the server does not support the option, the client will still accept to receive 512-byte blocks.
Submitted upstream: https://savannah.nongnu.org/patch/index.php?10462
Signed-off-by: Jerome Forissier jerome.forissier@linaro.org Acked-by: Ilias Apalodimas ilias.apalodimas@linaro.org
lib/lwip/lwip/src/apps/tftp/tftp.c | 94 +++++++++++++++++-- .../lwip/src/include/lwip/apps/tftp_client.h | 1 + 2 files changed, 89 insertions(+), 6 deletions(-)
diff --git a/lib/lwip/lwip/src/apps/tftp/tftp.c b/lib/lwip/lwip/src/apps/tftp/tftp.c index 74fc1fbe586..8e0c071772a 100644 --- a/lib/lwip/lwip/src/apps/tftp/tftp.c +++ b/lib/lwip/lwip/src/apps/tftp/tftp.c @@ -57,7 +57,7 @@ #include "lwip/timeouts.h" #include "lwip/debug.h"
-#define TFTP_MAX_PAYLOAD_SIZE 512 +#define TFTP_DEFAULT_BLOCK_SIZE 512 #define TFTP_HEADER_LENGTH 4
#define TFTP_RRQ 1 @@ -65,6 +65,7 @@ #define TFTP_DATA 3 #define TFTP_ACK 4 #define TFTP_ERROR 5 +#define TFTP_OACK 6
enum tftp_error { TFTP_ERROR_FILE_NOT_FOUND = 1, @@ -88,9 +89,11 @@ struct tftp_state { int timer; int last_pkt; u16_t blknum;
- u16_t blksize; u8_t retries; u8_t mode_write; u8_t tftp_mode;
- bool wait_oack;
};
static struct tftp_state tftp_state; @@ -137,10 +140,24 @@ send_request(const ip_addr_t *addr, u16_t port, u16_t opcode, const char* fname, { size_t fname_length = strlen(fname)+1; size_t mode_length = strlen(mode)+1;
- struct pbuf* p = init_packet(opcode, 0, fname_length + mode_length - 2);
size_t blksize_length = 0;
struct pbuf* p; char* payload; err_t ret;
if (tftp_state.blksize) {
blksize_length = 14; /* maximum (blksize is a u16_t): 'blksize\0XXXXX\0' */
if (tftp_state.blksize < 10000)
blksize_length--;if (tftp_state.blksize < 1000)
blksize_length--;if (tftp_state.blksize < 100)
blksize_length--;if (tftp_state.blksize < 10)
blksize_length--;}
I'd really prefer the alternative for this [0]. Since you havent changed it, make sure you queue it up if v12 gets merged
Sorry about that, I forgot. The code you propose in [0] needs a few adjustments: >= not >, initialize cnt to 1 to account for one digit at least, and do nothing if tftp_state.blksize == 0. We can get rid of cnt and it still looks good and even better IMO. So how about this?
diff --git a/lib/lwip/lwip/src/apps/tftp/tftp.c b/lib/lwip/lwip/src/apps/tftp/tftp.c index 8e0c071772a..7012aad9d91 100644 --- a/lib/lwip/lwip/src/apps/tftp/tftp.c +++ b/lib/lwip/lwip/src/apps/tftp/tftp.c @@ -141,20 +141,18 @@ send_request(const ip_addr_t *addr, u16_t port, u16_t opcode, const char* fname, size_t fname_length = strlen(fname)+1; size_t mode_length = strlen(mode)+1; size_t blksize_length = 0; + int blksize = tftp_state.blksize; struct pbuf* p; char* payload; err_t ret;
- if (tftp_state.blksize) { - blksize_length = 14; /* maximum (blksize is a u16_t): 'blksize\0XXXXX\0' */ - if (tftp_state.blksize < 10000) - blksize_length--; - if (tftp_state.blksize < 1000) - blksize_length--; - if (tftp_state.blksize < 100) - blksize_length--; - if (tftp_state.blksize < 10) - blksize_length--; + if (blksize) { + /* "blksize\0.\0" with . = 1 digit */ + blksize_length = strlen("blksize") + 1 + 1 + 1; + while (blksize >= 10) { + blksize /= 10; + blksize_length++; + }
- p = init_packet(opcode, 0, fname_length + mode_length + blksize_length - 2); if (p == NULL) { return ERR_MEM;
[...]
-- 2.40.1
[0] https://lore.kernel.org/u-boot/CAC_iWjJjsOZYZHB+019G8xs33+Bj2FeV8MB7FfhJ-C8v...
Thanks /Ilias
Thanks,