[PATCH v11 23/29] lwip: tftp: add support of blksize option to client
Ilias Apalodimas
ilias.apalodimas at linaro.org
Fri Oct 4 08:41:02 CEST 2024
Hi Jerome,
On Thu, 3 Oct 2024 at 18:47, Jerome Forissier
<jerome.forissier at 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.
>
> Signed-off-by: Jerome Forissier <jerome.forissier at 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 don't think we have log10 anywhere in u-boot but I think writing this as
blksize_length = strlen("blksize") + 1;
int cnt = 0;
int blksize = tftp_state.blksize;
while (blksize > 10) {
blksize /= 10;
cnt++;
}
cnt++;
is a bit easier to read
> +
> + 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);
>
> --
> 2.40.1
>
I am fine with it as is since this has to be merged in LWIP anyway.
The refactoring above can go in a new patch if we decide to merge this
now.
Acked-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
More information about the U-Boot
mailing list