From d0d5f33bfc1fae38ca76f02ac249da6779c6282a Mon Sep 17 00:00:00 2001 From: Carlos Pizano Date: Wed, 16 Sep 2015 18:21:57 -0700 Subject: [PATCH] [lib][tftp] Move tftp from inetsrv and apply review changes. From https://codereview.chromium.org/1346853002 --- app/inetsrv/inetsrv.c | 2 +- app/inetsrv/rules.mk | 3 +- {app/inetsrv => lib/tftp/include/lib}/tftp.h | 0 lib/tftp/rules.mk | 14 ++++ {app/inetsrv => lib/tftp}/tftp.c | 69 ++++++++++++-------- 5 files changed, 59 insertions(+), 29 deletions(-) rename {app/inetsrv => lib/tftp/include/lib}/tftp.h (100%) create mode 100644 lib/tftp/rules.mk rename {app/inetsrv => lib/tftp}/tftp.c (83%) diff --git a/app/inetsrv/inetsrv.c b/app/inetsrv/inetsrv.c index cb147cfe..11d412b5 100644 --- a/app/inetsrv/inetsrv.c +++ b/app/inetsrv/inetsrv.c @@ -29,11 +29,11 @@ #include #include #include +#include #include #include #include "inetsrv.h" -#include "tftp.h" static int chargen_worker(void *socket) { diff --git a/app/inetsrv/rules.mk b/app/inetsrv/rules.mk index b340ee35..05bd83ba 100644 --- a/app/inetsrv/rules.mk +++ b/app/inetsrv/rules.mk @@ -4,10 +4,11 @@ MODULE := $(LOCAL_DIR) MODULE_SRCS += \ $(LOCAL_DIR)/inetsrv.c \ - $(LOCAL_DIR)/tftp.c \ + MODULE_DEPS := \ lib/cksum \ lib/minip \ + lib/tftp \ include make/module.mk diff --git a/app/inetsrv/tftp.h b/lib/tftp/include/lib/tftp.h similarity index 100% rename from app/inetsrv/tftp.h rename to lib/tftp/include/lib/tftp.h diff --git a/lib/tftp/rules.mk b/lib/tftp/rules.mk new file mode 100644 index 00000000..8098ded6 --- /dev/null +++ b/lib/tftp/rules.mk @@ -0,0 +1,14 @@ +LOCAL_DIR := $(GET_LOCAL_DIR) + +MODULE := $(LOCAL_DIR) + +MODULE_DEPS := \ + lib/minip \ + lib/cksum + +GLOBAL_INCLUDES += $(LOCAL_DIR)/include + +MODULE_SRCS += \ + $(LOCAL_DIR)/tftp.c \ + +include make/module.mk diff --git a/app/inetsrv/tftp.c b/lib/tftp/tftp.c similarity index 83% rename from app/inetsrv/tftp.c rename to lib/tftp/tftp.c index a64c0f2e..694e39f7 100644 --- a/app/inetsrv/tftp.c +++ b/lib/tftp/tftp.c @@ -23,6 +23,7 @@ #include #include +#include #include #include #include @@ -32,9 +33,9 @@ #include #include -#include "tftp.h" +#include -#define TFTP_BUFSIZE 512 +#define LOCAL_TRACE 0 // TFTP Opcodes: #define TFTP_OPCODE_RRQ 1UL @@ -60,7 +61,8 @@ static struct list_node tftp_list = LIST_INITIAL_VALUE(tftp_list); -// Represents tftp jobs in progress or possible. +// Represents tftp jobs and clients of them. If |socket| is not null the +// job is in progress and members below it are valid. typedef struct { struct list_node list; // Registration info. @@ -78,35 +80,42 @@ uint16_t next_port = 2224; static void send_ack(udp_socket_t* socket, uint16_t count) { + // Packet is [4][count]. status_t st; uint16_t ack[] = {htons(TFTP_OPCODE_ACK), htons(count)}; st = udp_send(ack, sizeof(ack), socket); - if (st < 0) - TRACEF("send_ack failed: %d\n", st); + if (st < 0) { + LTRACEF("send_ack failed: %d\n", st); + } +} + +static void send_error(udp_socket_t* socket, uint16_t code) +{ + // Packet is [5][error code][error in ascii-string][0]. + status_t st; + uint16_t ncode = htons(code); + uint16_t err[] = {htons(TFTP_OPCODE_ERROR), ncode, + 0x7245, 0x2072, 0x3030 + ncode, 0 }; + st = udp_send(err, sizeof(err), socket); + if (st < 0) { + LTRACEF("send_err failed: %d\n", st); + } } static void end_transfer(tftp_job_t* job) { udp_close(job->socket); job->socket = NULL; + job->src_addr = 0UL; job->callback(NULL, 0UL, job->arg); } -static void send_error(udp_socket_t* socket, uint16_t code) -{ - status_t st; - uint16_t ncode = htons(code); - uint16_t err[] = {htons(TFTP_OPCODE_ERROR), ncode, - 0x7245, 0x2072, 0x3030 + ncode, 0 }; - st = udp_send(err, sizeof(err), socket); - if (st < 0) - TRACEF("send_err failed: %d\n", st); -} - static void udp_wrq_callback(void *data, size_t len, uint32_t srcaddr, uint16_t srcport, void *arg) { + // Packet is [3][count][data]. All packets but the last have 512 + // bytes of data, including zero data. char* data_c = data; tftp_job_t* job = arg; job->pkt_count++; @@ -123,14 +132,14 @@ static void udp_wrq_callback(void *data, size_t len, } if ((srcaddr != job->src_addr) || (srcport != job->src_port)) { - TRACEF("invalid source\n"); + LTRACEF("invalid source\n"); send_error(job->socket, TFTP_ERROR_UNKNOWN_XFER); end_transfer(job); return; } if (RD_U16(data_c) != htons(TFTP_OPCODE_DATA)) { - TRACEF("invalid opcode\n"); + LTRACEF("invalid opcode\n"); send_error(job->socket, TFTP_ERROR_ILLEGAL_OP); end_transfer(job); return; @@ -138,12 +147,14 @@ static void udp_wrq_callback(void *data, size_t len, send_ack(job->socket, job->pkt_count); - if (job->callback(&data_c[4], len, job->arg) < 0) { + if (job->callback(&data_c[4], len - 4, job->arg) < 0) { // The client wants to abort. send_error(job->socket, TFTP_ERROR_FULL); end_transfer(job); } + // 512 bytes payload plus 4 of fixed header. The last packet. + // has always less than 512 bytes of payload. if (len != 516) { end_transfer(job); } @@ -151,6 +162,7 @@ static void udp_wrq_callback(void *data, size_t len, static tftp_job_t* get_job_by_name(const char* file_name) { + DEBUG_ASSERT(file_name); tftp_job_t *entry; list_for_every_entry(&tftp_list, entry, tftp_job_t, list) { if (strcmp(entry->file_name, file_name) == 0) { @@ -171,7 +183,7 @@ static void udp_svc_callback(void *data, size_t len, st = udp_open(srcaddr, next_port, srcport, &socket); if (st < 0) { - TRACEF("error opening send socket %d\n", st); + LTRACEF("error opening send socket %d\n", st); return; } @@ -180,7 +192,7 @@ static void udp_svc_callback(void *data, size_t len, if (opcode != TFTP_OPCODE_WRQ) { // Operation not suported. - TRACEF("op not supported, opcode: %d\n", opcode); + LTRACEF("op not supported, opcode: %d\n", opcode); send_error(socket, TFTP_ERROR_ACCESS); udp_close(socket); return; @@ -192,7 +204,7 @@ static void udp_svc_callback(void *data, size_t len, if (!job) { // Nobody claims to handle that file. - TRACEF("no client registered for file\n"); + LTRACEF("no client registered for file\n"); send_error(socket, TFTP_ERROR_UNKNOWN_XFER); udp_close(socket); return; @@ -202,15 +214,15 @@ static void udp_svc_callback(void *data, size_t len, // There is already an ongoing job. // TODO: garbage collect the existing one if too long since the // last packet was processed. - TRACEF("existing job in progress\n"); + LTRACEF("existing job in progress\n"); send_error(socket, TFTP_ERROR_EXISTS); udp_close(socket); return; } - TRACEF("write op accepted, port %d\n", srcport); + LTRACEF("write op accepted, port %d\n", srcport); // Request accepted. The rest of the transfer happens between - // next_port <----> srcport via |udp_wrq_callback|. + // next_port <----> srcport via udp_wrq_callback(). job->socket = socket; job->src_addr = srcaddr; @@ -219,7 +231,7 @@ static void udp_svc_callback(void *data, size_t len, st = udp_listen(next_port, &udp_wrq_callback, job); if (st < 0) { - TRACEF("error listening on port\n"); + LTRACEF("error listening on port\n"); return; } @@ -229,6 +241,9 @@ static void udp_svc_callback(void *data, size_t len, int tftp_set_write_client(const char* file_name, tftp_callback_t cb, void* arg) { + DEBUG_ASSERT(file_name); + DEBUG_ASSERT(cb); + tftp_job_t *job; list_for_every_entry(&tftp_list, job, tftp_job_t, list) { @@ -255,7 +270,7 @@ static unsigned long test_crc = 0UL; int test_tftp_client(void* data, size_t len, void* arg) { if (!data) { - TRACEF("--test transfer done-- crc32 = %lu\n", test_crc); + printf("--test transfer done-- crc32 = %lu\n", test_crc); test_crc = 0UL; }