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

[Jerome Forissier]

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 >, 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,
-- 
Jerome