Message ID | 20200516194950.97880-1-rfried.dev@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | [v2] net: tftp: Add client support for RFC 7440 | expand |
On Sat, May 16, 2020 at 10:49:50PM +0300, Ramon Fried wrote: > [...] > index be9e6391d6..b85b44201f 100644 > --- a/README > +++ b/README > @@ -3522,6 +3522,11 @@ List of environment variables (most likely not complete): > downloads succeed with high packet loss rates, or with > unreliable TFTP servers or client hardware. > > + tftpwindowsize - if this is set, the value is used for TFTP's > + window size as described by RFC 7440. > + This means that count of blocks we can receive before > + sending ack to server. that -> the ? > + > vlan - When set to a value < 4095 the traffic over > Ethernet is encapsulated/received over 802.1q > VLAN tagged frames. > diff --git a/net/tftp.c b/net/tftp.c > index be24e63075..e2c005da6e 100644 > --- a/net/tftp.c > +++ b/net/tftp.c > @@ -5,7 +5,6 @@ > * Copyright 2011 Comelit Group SpA, > * Luca Ceresoli <luca.ceresoli at comelit.it> > */ > - > #include <common.h> > #include <command.h> > #include <efi_loader.h> > @@ -95,6 +94,12 @@ static int tftp_tsize; > /* The number of hashes we printed */ > static short tftp_tsize_num_hash; > #endif > +/* The window size neogiciated */ neogiciated -> negotiated > +static ushort tftp_windowsize; > +/* Next block to send ack to */ > +static ushort tftp_next_ack; > +/* Last nack block we send */ > +static ushort tftp_last_nack; > #ifdef CONFIG_CMD_TFTPPUT > /* 1 if writing, else 0 */ > static int tftp_put_active; > @@ -134,8 +139,19 @@ static char tftp_filename[MAX_LEN]; > * (but those using CONFIG_IP_DEFRAG may want to set a larger block in cfg file) > */ > > +/* When windowsize is defined to 1, > + * tftp behaves the same way as it was > + * never declared > + */ > +#ifdef CONFIG_TFTP_WINDOWSIZE I'm not sure what's the policy but I think that new CONFIG_ options should be added to a Kconfig file. > +#define TFTP_WINDOWSIZE CONFIG_TFTP_WINDOWSIZE > +#else > +#define TFTP_WINDOWSIZE 1 > +#endif > + > static unsigned short tftp_block_size = TFTP_BLOCK_SIZE; > static unsigned short tftp_block_size_option = CONFIG_TFTP_BLOCKSIZE; > +static unsigned short tftp_window_size_option = TFTP_WINDOWSIZE; > > static inline int store_block(int block, uchar *src, unsigned int len) > { > @@ -348,6 +364,14 @@ static void tftp_send(void) > /* try for more effic. blk size */ > pkt += sprintf((char *)pkt, "blksize%c%d%c", > 0, tftp_block_size_option, 0); > + > + /* try for more effic. window size. > + * Implemented only for tftp get. > + * Don't bother sending if it's 1 > + */ > + if (tftp_state == STATE_SEND_RRQ && tftp_window_size_option > 1) > + pkt += sprintf((char *)pkt, "windowsize%c%d%c", > + 0, tftp_window_size_option, 0); > len = pkt - xp; > break; > > @@ -500,6 +524,15 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, > (char *)pkt + i + 6, tftp_tsize); > } > #endif > + if (strcmp((char *)pkt + i, "windowsize") == 0) { > + tftp_windowsize = > + simple_strtoul((char *)pkt + i + 11, > + NULL, 10); > + debug("windowsize = %s, %d\n", > + (char *)pkt + i + 11, tftp_windowsize); > + } > + > + tftp_next_ack = tftp_windowsize; This assignment can be moved outside of the for loop. > } > #ifdef CONFIG_CMD_TFTPPUT > if (tftp_put_active) { > @@ -514,6 +547,26 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, > if (len < 2) > return; > len -= 2; > + > + if (ntohs(*(__be16 *)pkt) != (ushort)(tftp_cur_block + 1)) { > + debug("Received unexpected block: %d, expected: %d\n", > + ntohs(*(__be16 *)pkt), > + (ushort)(tftp_cur_block + 1)); > + /* > + * If one packet is dropped most likely > + * all other buffers in the window > + * that will arrive will cause a sending NACK. > + * This just overwellms the server, let's just send one. > + */ > + if (tftp_last_nack != tftp_cur_block) { > + tftp_send(); > + tftp_last_nack = tftp_cur_block; > + tftp_next_ack = (ushort)(tftp_cur_block + > + tftp_windowsize); Here neither tftp_cur_block nor tftp_windowsize were updated, so it shouldn't be necessary to update tftp_next_ack? If the first data packet is lost and we receive data block > 1, we call tftp_send() still in state STATE_SEND_RRQ, which sends again the read request. I would have expected instead to send an ack for block zero to force the server to retransmit block 1; but I guess both ways are ok. > + } > + break; > + } > + > tftp_cur_block = ntohs(*(__be16 *)pkt); Perhaps, just increase tftp_cur_block by one, since we have already verified that "ntohs(*(__be16 *)pkt) == (ushort)(tftp_cur_block + 1)" before. In tftp_handler() there is this debug message: if (tftp_state == STATE_SEND_RRQ) debug("Server did not acknowledge timeout option!\n"); printed when the server doesn't reply to the options. Since multiple options are now supported (not only 'timeout'), the message should be updated (this was already inaccurate before your patch, but it would be the right moment to fix it). Beniamino
Thanks for the review. Will send v3 shortly. On Mon, May 18, 2020 at 12:42 AM Beniamino Galvani <b.galvani at gmail.com> wrote: > > On Sat, May 16, 2020 at 10:49:50PM +0300, Ramon Fried wrote: > > [...] > > index be9e6391d6..b85b44201f 100644 > > --- a/README > > +++ b/README > > @@ -3522,6 +3522,11 @@ List of environment variables (most likely not complete): > > downloads succeed with high packet loss rates, or with > > unreliable TFTP servers or client hardware. > > > > + tftpwindowsize - if this is set, the value is used for TFTP's > > + window size as described by RFC 7440. > > + This means that count of blocks we can receive before > > + sending ack to server. > > that -> the ? Yes, typo. fixed. > > > + > > vlan - When set to a value < 4095 the traffic over > > Ethernet is encapsulated/received over 802.1q > > VLAN tagged frames. > > diff --git a/net/tftp.c b/net/tftp.c > > index be24e63075..e2c005da6e 100644 > > --- a/net/tftp.c > > +++ b/net/tftp.c > > @@ -5,7 +5,6 @@ > > * Copyright 2011 Comelit Group SpA, > > * Luca Ceresoli <luca.ceresoli at comelit.it> > > */ > > - > > #include <common.h> > > #include <command.h> > > #include <efi_loader.h> > > @@ -95,6 +94,12 @@ static int tftp_tsize; > > /* The number of hashes we printed */ > > static short tftp_tsize_num_hash; > > #endif > > +/* The window size neogiciated */ > > neogiciated -> negotiated Typo. fixed. thanks. > > > +static ushort tftp_windowsize; > > +/* Next block to send ack to */ > > +static ushort tftp_next_ack; > > +/* Last nack block we send */ > > +static ushort tftp_last_nack; > > #ifdef CONFIG_CMD_TFTPPUT > > /* 1 if writing, else 0 */ > > static int tftp_put_active; > > @@ -134,8 +139,19 @@ static char tftp_filename[MAX_LEN]; > > * (but those using CONFIG_IP_DEFRAG may want to set a larger block in cfg file) > > */ > > > > +/* When windowsize is defined to 1, > > + * tftp behaves the same way as it was > > + * never declared > > + */ > > +#ifdef CONFIG_TFTP_WINDOWSIZE > > I'm not sure what's the policy but I think that new CONFIG_ options > should be added to a Kconfig file. Yes, I added a new option in Kconfig. thanks. > > > +#define TFTP_WINDOWSIZE CONFIG_TFTP_WINDOWSIZE > > +#else > > +#define TFTP_WINDOWSIZE 1 > > +#endif > > + > > static unsigned short tftp_block_size = TFTP_BLOCK_SIZE; > > static unsigned short tftp_block_size_option = CONFIG_TFTP_BLOCKSIZE; > > +static unsigned short tftp_window_size_option = TFTP_WINDOWSIZE; > > > > static inline int store_block(int block, uchar *src, unsigned int len) > > { > > @@ -348,6 +364,14 @@ static void tftp_send(void) > > /* try for more effic. blk size */ > > pkt += sprintf((char *)pkt, "blksize%c%d%c", > > 0, tftp_block_size_option, 0); > > + > > + /* try for more effic. window size. > > + * Implemented only for tftp get. > > + * Don't bother sending if it's 1 > > + */ > > + if (tftp_state == STATE_SEND_RRQ && tftp_window_size_option > 1) > > + pkt += sprintf((char *)pkt, "windowsize%c%d%c", > > + 0, tftp_window_size_option, 0); > > len = pkt - xp; > > break; > > > > @@ -500,6 +524,15 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, > > (char *)pkt + i + 6, tftp_tsize); > > } > > #endif > > + if (strcmp((char *)pkt + i, "windowsize") == 0) { > > + tftp_windowsize = > > + simple_strtoul((char *)pkt + i + 11, > > + NULL, 10); > > + debug("windowsize = %s, %d\n", > > + (char *)pkt + i + 11, tftp_windowsize); > > + } > > + > > + tftp_next_ack = tftp_windowsize; > > This assignment can be moved outside of the for loop. Done. > > > } > > #ifdef CONFIG_CMD_TFTPPUT > > if (tftp_put_active) { > > @@ -514,6 +547,26 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, > > if (len < 2) > > return; > > len -= 2; > > + > > + if (ntohs(*(__be16 *)pkt) != (ushort)(tftp_cur_block + 1)) { > > + debug("Received unexpected block: %d, expected: %d\n", > > + ntohs(*(__be16 *)pkt), > > + (ushort)(tftp_cur_block + 1)); > > + /* > > + * If one packet is dropped most likely > > + * all other buffers in the window > > + * that will arrive will cause a sending NACK. > > + * This just overwellms the server, let's just send one. > > + */ > > + if (tftp_last_nack != tftp_cur_block) { > > + tftp_send(); > > + tftp_last_nack = tftp_cur_block; > > + tftp_next_ack = (ushort)(tftp_cur_block + > > + tftp_windowsize); > > Here neither tftp_cur_block nor tftp_windowsize were updated, so it > shouldn't be necessary to update tftp_next_ack? tftp_windowsize can't change during data transfer. If we receive unexpected buffer. We send the last block we got, and set the next ack to the last block + tftp_windowsize. > > If the first data packet is lost and we receive data block > 1, we > call tftp_send() still in state STATE_SEND_RRQ, which sends again the > read request. I would have expected instead to send an ack for block > zero to force the server to retransmit block 1; but I guess both ways > are ok. server implementation actually don't respond to NACK packets, to avoid the sorcerers apprentice bug. The just wait for timeout and re-transmit the last packet they sent. > > > + } > > + break; > > + } > > + > > tftp_cur_block = ntohs(*(__be16 *)pkt); > > Perhaps, just increase tftp_cur_block by one, since we have already > verified that "ntohs(*(__be16 *)pkt) == (ushort)(tftp_cur_block + 1)" > before. > > In tftp_handler() there is this debug message: > > if (tftp_state == STATE_SEND_RRQ) > debug("Server did not acknowledge timeout option!\n"); > > printed when the server doesn't reply to the options. Since multiple > options are now supported (not only 'timeout'), the message should be > updated (this was already inaccurate before your patch, but it would > be the right moment to fix it). > > Beniamino
diff --git a/README b/README index be9e6391d6..b85b44201f 100644 --- a/README +++ b/README @@ -3522,6 +3522,11 @@ List of environment variables (most likely not complete): downloads succeed with high packet loss rates, or with unreliable TFTP servers or client hardware. + tftpwindowsize - if this is set, the value is used for TFTP's + window size as described by RFC 7440. + This means that count of blocks we can receive before + sending ack to server. + vlan - When set to a value < 4095 the traffic over Ethernet is encapsulated/received over 802.1q VLAN tagged frames. diff --git a/net/tftp.c b/net/tftp.c index be24e63075..e2c005da6e 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -5,7 +5,6 @@ * Copyright 2011 Comelit Group SpA, * Luca Ceresoli <luca.ceresoli at comelit.it> */ - #include <common.h> #include <command.h> #include <efi_loader.h> @@ -95,6 +94,12 @@ static int tftp_tsize; /* The number of hashes we printed */ static short tftp_tsize_num_hash; #endif +/* The window size neogiciated */ +static ushort tftp_windowsize; +/* Next block to send ack to */ +static ushort tftp_next_ack; +/* Last nack block we send */ +static ushort tftp_last_nack; #ifdef CONFIG_CMD_TFTPPUT /* 1 if writing, else 0 */ static int tftp_put_active; @@ -134,8 +139,19 @@ static char tftp_filename[MAX_LEN]; * (but those using CONFIG_IP_DEFRAG may want to set a larger block in cfg file) */ +/* When windowsize is defined to 1, + * tftp behaves the same way as it was + * never declared + */ +#ifdef CONFIG_TFTP_WINDOWSIZE +#define TFTP_WINDOWSIZE CONFIG_TFTP_WINDOWSIZE +#else +#define TFTP_WINDOWSIZE 1 +#endif + static unsigned short tftp_block_size = TFTP_BLOCK_SIZE; static unsigned short tftp_block_size_option = CONFIG_TFTP_BLOCKSIZE; +static unsigned short tftp_window_size_option = TFTP_WINDOWSIZE; static inline int store_block(int block, uchar *src, unsigned int len) { @@ -348,6 +364,14 @@ static void tftp_send(void) /* try for more effic. blk size */ pkt += sprintf((char *)pkt, "blksize%c%d%c", 0, tftp_block_size_option, 0); + + /* try for more effic. window size. + * Implemented only for tftp get. + * Don't bother sending if it's 1 + */ + if (tftp_state == STATE_SEND_RRQ && tftp_window_size_option > 1) + pkt += sprintf((char *)pkt, "windowsize%c%d%c", + 0, tftp_window_size_option, 0); len = pkt - xp; break; @@ -500,6 +524,15 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, (char *)pkt + i + 6, tftp_tsize); } #endif + if (strcmp((char *)pkt + i, "windowsize") == 0) { + tftp_windowsize = + simple_strtoul((char *)pkt + i + 11, + NULL, 10); + debug("windowsize = %s, %d\n", + (char *)pkt + i + 11, tftp_windowsize); + } + + tftp_next_ack = tftp_windowsize; } #ifdef CONFIG_CMD_TFTPPUT if (tftp_put_active) { @@ -514,6 +547,26 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, if (len < 2) return; len -= 2; + + if (ntohs(*(__be16 *)pkt) != (ushort)(tftp_cur_block + 1)) { + debug("Received unexpected block: %d, expected: %d\n", + ntohs(*(__be16 *)pkt), + (ushort)(tftp_cur_block + 1)); + /* + * If one packet is dropped most likely + * all other buffers in the window + * that will arrive will cause a sending NACK. + * This just overwellms the server, let's just send one. + */ + if (tftp_last_nack != tftp_cur_block) { + tftp_send(); + tftp_last_nack = tftp_cur_block; + tftp_next_ack = (ushort)(tftp_cur_block + + tftp_windowsize); + } + break; + } + tftp_cur_block = ntohs(*(__be16 *)pkt); update_block_number(); @@ -557,7 +610,10 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, * Acknowledge the block just received, which will prompt * the remote for the next one. */ - tftp_send(); + if (tftp_cur_block == tftp_next_ack) { + tftp_send(); + tftp_next_ack += tftp_windowsize; + } if (len < tftp_block_size) tftp_complete(); @@ -634,6 +690,10 @@ void tftp_start(enum proto_t protocol) if (ep != NULL) tftp_block_size_option = simple_strtol(ep, NULL, 10); + ep = env_get("tftpwindowsize"); + if (ep != NULL) + tftp_window_size_option = simple_strtol(ep, NULL, 10); + ep = env_get("tftptimeout"); if (ep != NULL) timeout_ms = simple_strtol(ep, NULL, 10); @@ -655,8 +715,8 @@ void tftp_start(enum proto_t protocol) } #endif - debug("TFTP blocksize = %i, timeout = %ld ms\n", - tftp_block_size_option, timeout_ms); + debug("TFTP blocksize = %i, TFTP windowsize = %d timeout = %ld ms\n", + tftp_block_size_option, tftp_window_size_option, timeout_ms); tftp_remote_ip = net_server_ip; if (!net_parse_bootfile(&tftp_remote_ip, tftp_filename, MAX_LEN)) { @@ -752,7 +812,8 @@ void tftp_start(enum proto_t protocol) tftp_our_port = simple_strtol(ep, NULL, 10); #endif tftp_cur_block = 0; - + tftp_windowsize = 1; + tftp_last_nack = 0; /* zero out server ether in case the server ip has changed */ memset(net_server_ethaddr, 0, 6); /* Revert tftp_block_size to dflt */