[PATCH v12 18/21] net: lwip: tftp: add support of blksize option to client

[Ilias Apalodimas]

Hi Jerome,

On Wed, 9 Oct 2024 at 17:50, 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.
>
> Submitted upstream: https://savannah.nongnu.org/patch/index.php?10462
>
> Signed-off-by: Jerome Forissier <jerome.forissier at linaro.org>
> Acked-by: Ilias Apalodimas <ilias.apalodimas 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'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