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

[Ilias Apalodimas]

On Wed, 9 Oct 2024 at 19:30, Jerome Forissier
<jerome.forissier at 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 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
>
> 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