[U-Boot] [PATCH v2 5/6] TFTP: net/tftp.c: add server mode receive

Luca Ceresoli
2011-05-17T08:06:31Z

Detlev Zundel wrote:

> Hi Luca,
>
>> Signed-off-by: Luca Ceresoli<luca.ceresoli@comelit.it>
>> Cc: Wolfgang Denk<wd@denx.de>
>> ---
>> Changes in v2: none.
> Only one comment below
>
>> net/tftp.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>> net/tftp.h | 6 +++++
>> 2 files changed, 74 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/tftp.c b/net/tftp.c
>> index e166a63..87eb0b8 100644
>> --- a/net/tftp.c
>> +++ b/net/tftp.c
>> @@ -2,6 +2,8 @@
>> * Copyright 1994, 1995, 2000 Neil Russell.
>> * (See License)
>> * Copyright 2000, 2001 DENX Software Engineering, Wolfgang Denk, wd at denx.de
>> + * Copyright 2011 Comelit Group SpA,
>> + * Luca Ceresoli<luca.ceresoli@comelit.it>
>> */
>>
>> #include<common.h>
>> @@ -74,6 +76,7 @@ static short TftpNumchars; /* The number of hashes we printed */
>> #define STATE_TOO_LARGE 3
>> #define STATE_BAD_MAGIC 4
>> #define STATE_OACK 5
>> +#define STATE_RECV_WRQ 6
>>
>> #define TFTP_BLOCK_SIZE 512 /* default TFTP block size */
>> #define TFTP_SEQUENCE_SIZE ((ulong)(1<<16)) /* sequence number is 16 bit */
>> @@ -241,6 +244,10 @@ TftpSend (void)
>> TftpBlock=ext2_find_next_zero_bit(Bitmap,(Mapsize*8),0);
>> /*..falling..*/
>> #endif
>> +
>> +#ifdef CONFIG_CMD_TFTPSRV
>> + case STATE_RECV_WRQ:
>> +#endif
>> case STATE_DATA:
>> xp = pkt;
>> s = (ushort *)pkt;
>> @@ -293,7 +300,11 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
>> #endif
>> return;
>> }
>> - if (TftpState != STATE_SEND_RRQ&& src != TftpRemotePort)
>> + if (TftpState != STATE_SEND_RRQ&&
>> +#ifdef CONFIG_CMD_TFTPSRV
>> + TftpState != STATE_RECV_WRQ&&
>> +#endif
>> + src != TftpRemotePort)
>> return;
> Hm, I have to admit that I do not really like adding so many ifdefs into
> the tftp code and even into the state machine code. Can you give me a
> number on how much larger u-boot gets on your platform with and without
> the server support? If this is not too much, maybe we should include
> support always. If it is too much, maybe we can at least keep the state
> machine without ifdefs - if I see it correctly, this will only add at
> maximum a few bytes and STATE_RECW_WRQ will simply never be entered,
> correct?
Here are my measurements for the dig297 board (ARMV7 OMAP3):

text data bss dec hex
357085 8684 214264 580033 8d9c1 TFTPSRV on
356327 8660 214264 579251 8d6b3 TFTPSRV off, without #ifs
356355 8660 214268 579283 8d6d3 TFTPSRV off, with #ifs (as in PATCH v2)

The TFTP server adds 730 bytes to the text, so I guess it's worth to keep it
optional.

To my big surprise, removing most bad-looking #ifs (all of those that save just
one line) I got a *smaller* U-Boot! I repeated the test three times to be extra
sure, the figures are correct: without the #ifs we save a few bytes.

Obvious enough, I'm going to remove the #ifs.

Luca