[PATCH v2] net: tftp: Add client support for RFC 7440
Ramon Fried
rfried.dev at gmail.com
Mon May 18 09:53:40 CEST 2020
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
More information about the U-Boot
mailing list