| Message ID | 6de92bad17c86e300e3ae4d1b8b67569b350410c.1728482473.git.jerome.forissier@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce the lwIP network stack | expand |
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 > + > + 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-C8v8uBNng@mail.gmail.com/ Thanks /Ilias
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-C8v8uBNng@mail.gmail.com/ > > Thanks > /Ilias Thanks,
On Wed, 9 Oct 2024 at 19:30, Jerome Forissier <jerome.forissier@linaro.org> wrote: > > > > 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 >, There's a cnt++ in that code after the loop, so it's either with > and ++ or >= > 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++; > + } Yea that looks ok Thanks /Ilias > > > > >> + > >> + 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-C8v8uBNng@mail.gmail.com/ > > > > Thanks > > /Ilias > > Thanks, > -- > Jerome
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--; + } + + p = init_packet(opcode, 0, fname_length + mode_length + blksize_length - 2); if (p == NULL) { return ERR_MEM; } @@ -148,7 +165,10 @@ send_request(const ip_addr_t *addr, u16_t port, u16_t opcode, const char* fname, payload = (char*) p->payload; MEMCPY(payload+2, fname, fname_length); MEMCPY(payload+2+fname_length, mode, mode_length); + if (tftp_state.blksize) + sprintf(payload+2+fname_length+mode_length, "blksize%c%d%c", 0, tftp_state.blksize, 0); + tftp_state.wait_oack = true; ret = udp_sendto(tftp_state.upcb, p, addr, port); pbuf_free(p); return ret; @@ -221,14 +241,14 @@ send_data(const ip_addr_t *addr, u16_t port) pbuf_free(tftp_state.last_data); } - tftp_state.last_data = init_packet(TFTP_DATA, tftp_state.blknum, TFTP_MAX_PAYLOAD_SIZE); + tftp_state.last_data = init_packet(TFTP_DATA, tftp_state.blknum, TFTP_DEFAULT_BLOCK_SIZE); if (tftp_state.last_data == NULL) { return; } payload = (u16_t *) tftp_state.last_data->payload; - ret = tftp_state.ctx->read(tftp_state.handle, &payload[2], TFTP_MAX_PAYLOAD_SIZE); + ret = tftp_state.ctx->read(tftp_state.handle, &payload[2], TFTP_DEFAULT_BLOCK_SIZE); if (ret < 0) { send_error(addr, port, TFTP_ERROR_ACCESS_VIOLATION, "Error occurred while reading the file."); close_handle(); @@ -239,6 +259,28 @@ send_data(const ip_addr_t *addr, u16_t port) resend_data(addr, port); } +static u16_t payload_size(void) +{ + if (tftp_state.blksize) + return tftp_state.blksize; + return TFTP_DEFAULT_BLOCK_SIZE; +} + +static const char * +find_option(struct pbuf *p, const char *option) +{ + int i; + u16_t optlen = strlen(option); + const char *b = p->payload; + + for (i = 0; i + optlen + 1 < p->len; i++) { + if (lwip_strnstr(b + i, option, optlen)) + return b + i + optlen + 2; + } + + return NULL; +} + static void tftp_recv(void *arg, struct udp_pcb *upcb, struct pbuf *p, const ip_addr_t *addr, u16_t port) { @@ -338,6 +380,15 @@ tftp_recv(void *arg, struct udp_pcb *upcb, struct pbuf *p, const ip_addr_t *addr } blknum = lwip_ntohs(sbuf[1]); + if (tftp_state.blksize && tftp_state.wait_oack) { + /* + * Data received while we are expecting an OACK for our blksize option. + * This means the server doesn't support it, let's switch back to the + * default block size. + */ + tftp_state.blksize = 0; + tftp_state.wait_oack = false; + } if (blknum == tftp_state.blknum) { pbuf_remove_header(p, TFTP_HEADER_LENGTH); @@ -349,7 +400,7 @@ tftp_recv(void *arg, struct udp_pcb *upcb, struct pbuf *p, const ip_addr_t *addr send_ack(addr, port, blknum); } - if (p->tot_len < TFTP_MAX_PAYLOAD_SIZE) { + if (p->tot_len < payload_size()) { close_handle(); } else { tftp_state.blknum++; @@ -386,7 +437,7 @@ tftp_recv(void *arg, struct udp_pcb *upcb, struct pbuf *p, const ip_addr_t *addr lastpkt = 0; if (tftp_state.last_data != NULL) { - lastpkt = tftp_state.last_data->tot_len != (TFTP_MAX_PAYLOAD_SIZE + TFTP_HEADER_LENGTH); + lastpkt = tftp_state.last_data->tot_len != (TFTP_DEFAULT_BLOCK_SIZE + TFTP_HEADER_LENGTH); } if (!lastpkt) { @@ -405,6 +456,25 @@ tftp_recv(void *arg, struct udp_pcb *upcb, struct pbuf *p, const ip_addr_t *addr close_handle(); } break; + case PP_HTONS(TFTP_OACK): { + const char *optval = find_option(p, "blksize"); + u16_t srv_blksize = 0; + tftp_state.wait_oack = false; + if (optval) { + if (!tftp_state.blksize) { + /* We did not request this option */ + send_error(addr, port, TFTP_ERROR_ILLEGAL_OPERATION, "blksize unexpected"); + } + srv_blksize = atoi(optval); + if (srv_blksize <= 0 || srv_blksize > tftp_state.blksize) { + send_error(addr, port, TFTP_ERROR_ILLEGAL_OPERATION, "Invalid blksize"); + } + LWIP_DEBUGF(TFTP_DEBUG | LWIP_DBG_STATE, ("tftp: accepting blksize=%d\n", srv_blksize)); + tftp_state.blksize = srv_blksize; + } + send_ack(addr, port, 0); + break; + } default: send_error(addr, port, TFTP_ERROR_ILLEGAL_OPERATION, "Unknown operation"); break; @@ -495,6 +565,18 @@ tftp_init_client(const struct tftp_context *ctx) return tftp_init_common(LWIP_TFTP_MODE_CLIENT, ctx); } +/** @ingroup tftp + * Set the block size to be used by the TFTP client. The server may choose to + * accept a lower value. + * @param blksize Block size in bytes + */ +void +tftp_client_set_blksize(u16_t blksize) +{ + if (blksize != TFTP_DEFAULT_BLOCK_SIZE) + tftp_state.blksize = blksize; +} + /** @ingroup tftp * Deinitialize ("turn off") TFTP client/server. */ diff --git a/lib/lwip/lwip/src/include/lwip/apps/tftp_client.h b/lib/lwip/lwip/src/include/lwip/apps/tftp_client.h index 24dbda6a8c9..e1e21d06b67 100644 --- a/lib/lwip/lwip/src/include/lwip/apps/tftp_client.h +++ b/lib/lwip/lwip/src/include/lwip/apps/tftp_client.h @@ -44,6 +44,7 @@ enum tftp_transfer_mode { }; err_t tftp_init_client(const struct tftp_context* ctx); +void tftp_client_set_blksize(u16_t blksize); err_t tftp_get(void* handle, const ip_addr_t *addr, u16_t port, const char* fname, enum tftp_transfer_mode mode); err_t tftp_put(void* handle, const ip_addr_t *addr, u16_t port, const char* fname, enum tftp_transfer_mode mode);