[app][inetsrv] add tftp server.

[app][inetsrv] add tftp server.

Only allows writting files. Needs a per-file handler to be wired and
provisionally has a test handler wired to "tftp_test.txt" which prints
the crc32 of the complete file pushed via
   tftp -i 192.169.0.xx tftp_test.txt

Some error conditions are yet to be fully handled.

BUG=none

[cpu_(ooo_6.6-7.5), 2015-09-16 18:04:11]

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c
File app/inetsrv/tftp.c (right):

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode37
app/inetsrv/tftp.c:37: #define TFTP_BUFSIZE 512
remove

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode63
app/inetsrv/tftp.c:63: // Represents tftp jobs in progress or possible.
elaborate, confusing

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode96
app/inetsrv/tftp.c:96: {
move send error next to send ack

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode101
app/inetsrv/tftp.c:101: st = udp_send(err, sizeof(err), socket);
xplain magic numbers

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode110
app/inetsrv/tftp.c:110: char* data_c = data;
in comment do the layout of the packet

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode128
app/inetsrv/tftp.c:128: end_transfer(job);
not sure spec requires to abort job.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode141
app/inetsrv/tftp.c:141: if (job->callback(&data_c[4], len, job->arg) < 0) {
bug: len - 4

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode188
app/inetsrv/tftp.c:188: 
put in the comment the packet layout.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode213
app/inetsrv/tftp.c:213: // next_port <----> srcport via |udp_wrq_callback|.
remove || and put ()

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode268
app/inetsrv/tftp.c:268: tftp_set_write_client("tftp_test.txt",
&test_tftp_client, NULL);
268 is test code, it can be removed.

[cja, 2015-09-16 21:19:41]

cja@google.com changed reviewers:
+ cja@google.com

[cja, 2015-09-16 21:19:42]

Mostly nits around TRACEF and checking argument parameters.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c
File app/inetsrv/tftp.c (right):

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode85
app/inetsrv/tftp.c:85: TRACEF("send_ack failed: %d\n", st);
nit: inconsistent use of single line conditionals with and without braces.
Perhaps run the file through the astyle script?

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode85
app/inetsrv/tftp.c:85: TRACEF("send_ack failed: %d\n", st);
LTRACEF for code you're checking in.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode103
app/inetsrv/tftp.c:103: TRACEF("send_err failed: %d\n", st);
LTRACEF for code you're checking in.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode109
app/inetsrv/tftp.c:109: {
What if data is NULL? It's dereferenced below with no check.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode111
app/inetsrv/tftp.c:111: tftp_job_t* job = arg;
arg also needs a NULL check.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode126
app/inetsrv/tftp.c:126: TRACEF("invalid source\n");
LTRACEF for code you're checking in.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode133
app/inetsrv/tftp.c:133: TRACEF("invalid opcode\n");
LTRACEF for code you're checking in.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode147
app/inetsrv/tftp.c:147: if (len != 516) {
What does 516 mean?

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode153
app/inetsrv/tftp.c:153: {
Need NULL check for file_name.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode174
app/inetsrv/tftp.c:174: TRACEF("error opening send socket %d\n", st);
LTRACEF for code you're checking in.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode183
app/inetsrv/tftp.c:183: TRACEF("op not supported, opcode: %d\n", opcode);
LTRACEF for code you're checking in.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode204
app/inetsrv/tftp.c:204: // last packet was processed.
is this todo a "nice to have" or required for operation?

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode205
app/inetsrv/tftp.c:205: TRACEF("existing job in progress\n");
LTRACEF for code you're checking in.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode211
app/inetsrv/tftp.c:211: TRACEF("write op accepted, port %d\n", srcport);
LTRACEF for code you're checking in.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode222
app/inetsrv/tftp.c:222: TRACEF("error listening on port\n");
LTRACEF for code you're checking in.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode231
app/inetsrv/tftp.c:231: {
Arg check.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode236
app/inetsrv/tftp.c:236: // TODO: un-registration.
Important?

[cpu_(ooo_6.6-7.5), 2015-09-17 01:22:31]

changes made.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c
File app/inetsrv/tftp.c (right):

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode85
app/inetsrv/tftp.c:85: TRACEF("send_ack failed: %d\n", st);
On 2015/09/16 21:19:41, cja wrote:
> LTRACEF for code you're checking in.

Acknowledged.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode85
app/inetsrv/tftp.c:85: TRACEF("send_ack failed: %d\n", st);
On 2015/09/16 21:19:41, cja wrote:
> LTRACEF for code you're checking in.

Acknowledged.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode103
app/inetsrv/tftp.c:103: TRACEF("send_err failed: %d\n", st);
On 2015/09/16 21:19:41, cja wrote:
> LTRACEF for code you're checking in.

Acknowledged.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode109
app/inetsrv/tftp.c:109: {
On 2015/09/16 21:19:41, cja wrote:
> What if data is NULL? It's dereferenced below with no check.

it will come with len == 0 ?

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode126
app/inetsrv/tftp.c:126: TRACEF("invalid source\n");
On 2015/09/16 21:19:41, cja wrote:
> LTRACEF for code you're checking in.

Acknowledged.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode133
app/inetsrv/tftp.c:133: TRACEF("invalid opcode\n");
On 2015/09/16 21:19:41, cja wrote:
> LTRACEF for code you're checking in.

Acknowledged.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode147
app/inetsrv/tftp.c:147: if (len != 516) {
On 2015/09/16 21:19:41, cja wrote:
> What does 516 mean?

good point, added a comment.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode153
app/inetsrv/tftp.c:153: {
On 2015/09/16 21:19:41, cja wrote:
> Need NULL check for file_name.

Acknowledged.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode174
app/inetsrv/tftp.c:174: TRACEF("error opening send socket %d\n", st);
On 2015/09/16 21:19:41, cja wrote:
> LTRACEF for code you're checking in.

Acknowledged.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode183
app/inetsrv/tftp.c:183: TRACEF("op not supported, opcode: %d\n", opcode);
On 2015/09/16 21:19:41, cja wrote:
> LTRACEF for code you're checking in.

Acknowledged.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode204
app/inetsrv/tftp.c:204: // last packet was processed.
On 2015/09/16 21:19:41, cja wrote:
> is this todo a "nice to have" or required for operation?

It is is a very nice to have, but it works as-is today. But I will implement
this at some point.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode205
app/inetsrv/tftp.c:205: TRACEF("existing job in progress\n");
On 2015/09/16 21:19:41, cja wrote:
> LTRACEF for code you're checking in.

Acknowledged.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode231
app/inetsrv/tftp.c:231: {
On 2015/09/16 21:19:41, cja wrote:
> Arg check.

Acknowledged.

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode236
app/inetsrv/tftp.c:236: // TODO: un-registration.
On 2015/09/16 21:19:41, cja wrote:
> Important?

In the next CL. I need to make sure I write the other side first.

[cja, 2015-09-17 18:11:56]

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c
File app/inetsrv/tftp.c (right):

https://codereview.chromium.org/1346853002/diff/1/app/inetsrv/tftp.c#newcode109
app/inetsrv/tftp.c:109: {
On 2015/09/17 01:22:31, cpu wrote:
> On 2015/09/16 21:19:41, cja wrote:
> > What if data is NULL? It's dereferenced below with no check.
> 
> it will come with len == 0 ?

Perhaps it's a personal preference, but I'd still check that the pointer is
valid when passed in.