Re: [Qemu-devel] [PATCH v2 2/3] slirp: Handle more than 65535 blocks in TFTP transfers

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2012-09-10 20:05, Hervé Poussineau wrote:
> RFC 1350 does not mention block count roll-over. However, a lot of TFTP servers
> implement it to be able to transmit big files, so do it also.
> 
> Current block size is 512 bytes, so TFTP files were limited to 32 MB.
> 
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  slirp/tftp.c |    9 +++++----
>  slirp/tftp.h |    1 +
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/slirp/tftp.c b/slirp/tftp.c
> index 520dbd6..75c9030 100644
> --- a/slirp/tftp.c
> +++ b/slirp/tftp.c
> @@ -97,7 +97,7 @@ static int tftp_session_find(Slirp *slirp, struct tftp_t *tp)
>    return -1;
>  }
>  
> -static int tftp_read_data(struct tftp_session *spt, uint16_t block_nr,
> +static int tftp_read_data(struct tftp_session *spt, uint32_t block_nr,
>                            uint8_t *buf, int len)
>  {
>      int bytes_read = 0;
> @@ -198,7 +198,7 @@ out:
>  }
>  
>  static int tftp_send_data(struct tftp_session *spt,
> -                          uint16_t block_nr,
> +                          uint32_t block_nr,
>  			  struct tftp_t *recv_tp)
>  {
>    struct sockaddr_in saddr, daddr;
> @@ -223,7 +223,7 @@ static int tftp_send_data(struct tftp_session *spt,
>    m->m_data += sizeof(struct udpiphdr);
>  
>    tp->tp_op = htons(TFTP_DATA);
> -  tp->x.tp_data.tp_block_nr = htons(block_nr);
> +  tp->x.tp_data.tp_block_nr = htons(block_nr & 0xffff);
>  
>    saddr.sin_addr = recv_tp->ip.ip_dst;
>    saddr.sin_port = recv_tp->udp.uh_dport;
> @@ -255,6 +255,7 @@ static int tftp_send_data(struct tftp_session *spt,
>      tftp_session_terminate(spt);
>    }
>  
> +  spt->block_nr = block_nr;

That's not really a nice interface: You pass in tftp_session::block_nr
as argument but you also manipulate it here. I would vote for some pure
variant: either implement a

void tftp_send_next_block(struct tftp_session *spt,
                          struct tftp_t *recv_tp)

, initializing tftp_session::block_nr to 0 before first call and
incrementing it inside on success. Or do the maintenance of block_nr
completely outside. I leaning a bit to variant 1.

>    return 0;
>  }
>  
> @@ -387,7 +388,7 @@ static void tftp_handle_ack(Slirp *slirp, struct tftp_t *tp, int pktlen)
>    }
>  
>    if (tftp_send_data(&slirp->tftp_sessions[s],
> -		     ntohs(tp->x.tp_data.tp_block_nr) + 1,
> +		     slirp->tftp_sessions[s].block_nr + 1,
>  		     tp) < 0) {
>      return;
>    }
> diff --git a/slirp/tftp.h b/slirp/tftp.h
> index 9c364ea..51704e4 100644
> --- a/slirp/tftp.h
> +++ b/slirp/tftp.h
> @@ -37,6 +37,7 @@ struct tftp_session {
>  
>      struct in_addr client_ip;
>      uint16_t client_port;
> +    uint32_t block_nr;
>  
>      int timestamp;
>  };
> 

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux