[PATCH] tftp(d): fix wrong logic in get/put feature preprocessor work + some error handling enhancements

[Denys Vlasenko]

On Monday 15 February 2010 05:23, Benjamin Cama wrote:
> Hi,
> 
> I tried to build tftpd applet with only "get" opcode support, but it
> didn't work (no data is transfered). I also got some accidental file
> overwriting problem which seems to be due to this problem.

bbox version and .config?

I just tested current git and it works. I.e. it _still_ works,
because I was also testing it every time tftpd code was touched.

> Basically, the logic behind the get/put opcode feature macros is
> brain-dead.

Not a bad start of the conversation with the author of the said code :D

Did you study how to communicate with people or is it your natural talent?

> Furthermore, there is a confusion in the code between the 
> actual get/put opcode and the meaning CMD_GET() and CMD_PUT() are
> assigned in tftp_protocol(), which is, get is to receive data (so it's
> get opcode for tftp, but put for tftpd) and put is to send it (same for
> tftp, but means get for tftpd). I think it would be better to name them
> XXX_RECV/XXX_SEND, but I leave that to someone else. It would make the
> code far clearer.

Perhaps.

> So, I removed almost all the macro conditionnal stuff at the top, and
> also fixed another bug in the middle (the test for the opcode against
> TFTP_RRQ/WRQ was inverted). The previous logic also assumed every opcode
> is what was enabled if there's only one opcode selected (so, a get
> becomes a put if get support has not been enabled ...). I didn't like it
> and find the "general case everywhere" approach better. (yes, you loose
> some optimizations) This "general case" is needed anyway either when you
> select at least the two applets (tftp and tftpd) or when you choose the
> two opcodes.
> 
> I also separated one error case (which opcode is supported) and added
> another error message saying we don't handle anything else than octet
> mode (yes, I got caught by that for some time).
> 
> Tested working with different features enabled on 1.15.3, patch against
> git master following.

Well, even discounting the size of new error messages, the code got bigger
(allyesconfig build):

# make bloatcheck
function                                             old     new   delta
tftpd_main                                           503     539     +36
------------------------------------------------------------------------------
(add/remove: 0/0 grow/shrink: 1/0 up/down: 36/0)               Total: 36 bytes
   text    data     bss     dec     hex filename
 839065     453    6828  846346   cea0a busybox_old
 839149     453    6828  846430   cea5e busybox_unstripped


Let me reproduce the bug first. I need bbox version and .config for this.
--
vda