In version 7.21.5 (sorry for not relating to the latest version for some reasons, but as far as I know it was not corrected recently) I've discovered that when during download TFTP client receives a block that it did not expect, it does not acknowledge it and increase number of tries, what later can cause termination of transfer because of "TFTP illegal operation". In my opinion it is incorrect because:
- if we wait for block X and we receive block (X-1), it means that ACK for block (X-1) was lost and it must be retransmitted. Why would we wait until retransmission timeout (which is in my opinion another problematic issue) with sending ACK and not retransmit it right away?
- this implementation is not transparrent for servers that use windowing extension

I suggest following solution. If we are waiting for block X and receive block Y:
- Y < X we should send ACK and not increase retry counter
- Y > X we should not increase retry counter
- Y == X we should send ACK and increase X (which is already implemented)

Moreover I don't think timeout for ACK retransmission should be dependent on timeout for whole operation. The bigger timeout, the slower transmission on networks with significant packet losses (when using windowing on server side).

Here are my changes to version 7.21.5 in tftp.c:

259,260c259
< /* Set per-block timeout to 10% of total */
< timeout = maxtime/10 ;

---
>     timeout = maxtime;
262,263c261,262
<     /* Average reposting an ACK after 15 seconds */
<     state->retry_max = (int)timeout/15;
---
>     /* Average reposting an ACK after 5 seconds */
>     state->retry_max = (int)timeout/5;
269,271d267
<   if(state->retry_max>50)
<     state->retry_max=50;
< 
601,615c597,603
<     if(NEXT_BLOCKNUM(state->block) != rblock) {
<       /* No, log it, up the retry count and fail if over the limit */
<       infof(data,
<             "Received unexpected DATA packet block %d\n", rblock);
<       state->retries++;
<       if(state->retries > state->retry_max) {
<         failf(data, "tftp_rx: giving up waiting for block %d",
<               NEXT_BLOCKNUM(state->block));
<         return CURLE_TFTP_ILLEGAL;
<       }
<       break;
<     }
<     /* This is the expected block.  Reset counters and ACK it. */
<     state->block = (unsigned short)rblock;
<     state->retries = 0;
---
>     int exp_block = NEXT_BLOCKNUM(state->block);
>     
>     if (exp_block < rblock)
>         break;
> 
>     /* We need to acknowledge blocks that we are waiting for and also the ones that we had already received 
>      * to allow TFTP server use windowing */
617c605
<     setpacketblock(&state->spacket, state->block);
---
>     setpacketblock(&state->spacket, (unsigned short)rblock);
626,629c614,628
< 
<     /* Check if completed (That is, a less than full packet is received) */
<     if(state->rbytes < (ssize_t)state->blksize+4){
<       state->state = TFTP_STATE_FIN;
---
>     
>     if (exp_block == rblock) {
>         /* This is the expected block.  Reset counters */
>         state->block = (unsigned short)rblock;
>         state->retries = 0;
> 
>         /* Check if completed (That is, a less than full packet is received) */
>         if(state->rbytes < (ssize_t)state->blksize+4){
>           state->state = TFTP_STATE_FIN;
>         }
>         else {
>           state->state = TFTP_STATE_RX;
>         }
>         
>         time(&state->rx_time);
632c631,635
<       state->state = TFTP_STATE_RX;
---
>         infof(data,
>             "Received unexpected DATA packet block %d, while waiting for block %d. Block has been acknowledged\n",
>             rblock, exp_block);
>         /* We don't update state->rx_time, because we don't want to delay retransmission of ACK 
>          * for last successfull reception of expected block (its number is (exp_block - 1)) */
634c637
<     time(&state->rx_time);
---
> 
Regards
Marcin Adamski
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette:  http://curl.haxx.se/mail/etiquette.html