[U-Boot] [RFC] Act as a TFTP server
Hi,
I am going to implement in U-Boot the ability to receive a file via TFTP acting as a server, not a client. I've sketched up a way to implement it, and I would appreciate any comments about it.
This is useful to solve the firewall issues that a Management Station can face when sending files to a U-Boot device. These issues are only partially solved using CONFIG_TFTP_PORT for '"punching through" the (Windows XP) firewall' (see the README).
My implementation would be very basic (keep it simple): - receive only (accept WRQ from remote client, not RRQ); - the Filename in the WRQ is ignored: the destination is always a user-provided memory location; - binary transfers only: the Mode in the WRQ is ignored; this is allowed by RFC1350 (section 5); - no TFTP Option Extensions (RFC2347); - no TFTP multicast.
After a preliminary analysis, I think the current TFTP clien implementation can be mostly reused without code duplication, hence the amount of new code would be pretty limited.
Even better, it would not increase binary size when the feature is disabled, as I don't see any need to extend the existing code in a way that cannot be easily put under appropriate #ifdefs.
From the user point of view, I would implement a new command, activated only when CONFIG_CMD_TFTPSRV is defined:
Usage: tftpsrv [<loadaddr>]
This would be used almost like tftpboot, except no file name is specified on the commandline. <loadaddr> would default to the environment variable.
Here's a tentative implementation roadmap. It is presented in a roughly top-down order, from user interface down to low level. Many steps, except for those in net/tftp.c, are pretty trivial.
- add CONFIG_CMD_TFTPSRV config option; - include/net.h: add TFTPSRV to proto_t, *as if it were a different protocol*; this may look dirty but I believe it makes implementation cleaner; - alternative: reuse TFTP protocol, but save in some flag (where?) whether the user wants a client or server transfer; - common/cmd_net.c: add a tftpsrv command and do_tftpsrv() function; just like do_tftpb() simply calls netboot_common(TFTP, ...), do_tftpsrv() would simply call netboot_common(TFTPSRV, ...); - common/cmd_net.c: netboot_common() should not need change, except maybe for the argv parsing: tftpsrv does not take a "hostaddr:filename" argument; to make it simpler, it could accept (but ignore) it if passed; - net/net.c: change net_check_prereq() as needed: it should not check for NetServerIP when protocol==TFTPSRV; - net/net.c: extend NetLoop() to handle the new protocol: call TftpStartServer() (not TftpStart()) when protocol==TFTPSRV; - net/tftp.c: add TftpStartServer() with a role similar to TftpStart(); - alternative: extend TftpStart() to handle both client and server mode; probably not worth doing as the actions to do are mostly different; - net/tftp.c: extend the TFTP state machine (TftpHandler() and TftpStart()) a new STATE_WAITING state (it should be enough); - the state machine would be different in the session setup: in STATE_WAITING handle WRQ packets and select a new UDP port; - after the setup phase, the state machine would converge on the currently-implemented STATE_DATA for normal DATA/ACK management.
Do you think my approach is correct? Would this feature be considered for mainline inclusion?
Thanks in advance, Luca
Dear Luca,
In message 4DA5682A.8040203@comelit.it you wrote:
I am going to implement in U-Boot the ability to receive a file via TFTP acting as a server, not a client. I've sketched up a way to implement it, and I would appreciate any comments about it.
Thanks for the efforts, and especially for discussing the design early. Highly appreciated.
From the user point of view, I would implement a new command, activated only when CONFIG_CMD_TFTPSRV is defined:
Usage: tftpsrv [<loadaddr>]
This would be used almost like tftpboot, except no file name is specified on the commandline. <loadaddr> would default to the environment variable.
I see some user interface issues here:
- When and how do you terminate this command? After a successful download, ok. But how long do we wait for that? Forever? Or do we time out?
- It would be nice if the user could terminate the command using ^C.
Do you think my approach is correct?
Looks ok to me.
Would this feature be considered for mainline inclusion?
Yes, of course.
Best regards,
Wolfgang Denk
Wolfgang, thanks for the feedback.
Wolfgang Denk wrote:
...
From the user point of view, I would implement a new command, activated only when CONFIG_CMD_TFTPSRV is defined:
Usage: tftpsrv [<loadaddr>]This would be used almost like tftpboot, except no file name is specified on the commandline. <loadaddr> would default to the environment variable.
I see some user interface issues here:
- When and how do you terminate this command? After a successful download, ok. But how long do we wait for that? Forever? Or do we time out?
The TFTP client tries sending a RRQ packet TIMEOUT_COUNT times (default 10), spaced TIMEOUT msecs (default 5000) from each other. 50 seconds total by default.
I would keep the same scheme for the server, except it would obviously not send anything after each timeout. This would allow to print something every 5 seconds, for user feedback.
- It would be nice if the user could terminate the command using ^C.
Sure. The main loop in NetLoop() would be unchanged, so I expect ctrlc() to work for free in the same way it does for the TFTP client.
Luca
Dear Luca Ceresoli,
In message 4DA59B5E.5000105@comelit.it you wrote:
Usage: tftpsrv [<loadaddr>]This would be used almost like tftpboot, except no file name is specified on the commandline. <loadaddr> would default to the environment variable.
I see some user interface issues here:
- When and how do you terminate this command? After a successful download, ok. But how long do we wait for that? Forever? Or do we time out?
The TFTP client tries sending a RRQ packet TIMEOUT_COUNT times (default 10), spaced TIMEOUT msecs (default 5000) from each other. 50 seconds total by default.
Well - this requires that there actually _is_ a TFTP client which attempts to connect. But there is no guarantee that any such client is running at all, or that it can connect through firewalls, routers, etc.
So assuming we never see any incoming packets - how long will we wait?
Best regards,
Wolfgang Denk
Il 30/04/2011 21:53, Wolfgang Denk ha scritto:
Dear Luca Ceresoli,
In message4DA59B5E.5000105@comelit.it you wrote:
Usage: tftpsrv [<loadaddr>]This would be used almost like tftpboot, except no file name is specified on the commandline. <loadaddr> would default to the environment variable.
I see some user interface issues here:
- When and how do you terminate this command? After a successful download, ok. But how long do we wait for that? Forever? Or do we time out?
The TFTP client tries sending a RRQ packet TIMEOUT_COUNT times (default 10), spaced TIMEOUT msecs (default 5000) from each other. 50 seconds total by default.
Well - this requires that there actually _is_ a TFTP client which attempts to connect. But there is no guarantee that any such client is running at all, or that it can connect through firewalls, routers, etc.
So assuming we never see any incoming packets - how long will we wait?
Once it has started, the server is stopped like the client is: - on a complete file reception - pressing Ctrl-C - after waiting 5 seconds for 10 times.
Luca
Dear Luca Ceresoli,
In message 4DBFF3AF.4040505@comelit.it you wrote:
So assuming we never see any incoming packets - how long will we wait?
Once it has started, the server is stopped like the client is:
- on a complete file reception
- pressing Ctrl-C
- after waiting 5 seconds for 10 times.
OK, fine. Can you please document thisn, then. Thanks.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Dear Luca Ceresoli,
In message4DBFF3AF.4040505@comelit.it you wrote:
So assuming we never see any incoming packets - how long will we wait?
Once it has started, the server is stopped like the client is:
- on a complete file reception
- pressing Ctrl-C
- after waiting 5 seconds for 10 times.
OK, fine. Can you please document thisn, then. Thanks.
Incorporating also Mike's suggestion for the one-line help, I propose this:
U_BOOT_CMD( tftpsrv, 2, 1, do_tftpsrv, "act as a TFTP server and boot the first received file", "[loadAddress]\n" "Listen for an incoming TFTP transfer, receive a file and boot it.\n" "The transfer is aborted if a transfer has not been started after\n" "about 50 seconds or if Ctrl-C is pressed." );
Comments?
Luca
Luca Ceresoli wrote:
- net/tftp.c: extend the TFTP state machine (TftpHandler() and TftpStart()) a new STATE_WAITING state (it should be enough);
- the state machine would be different in the session setup: in STATE_WAITING handle WRQ packets and select a new UDP port;
- after the setup phase, the state machine would converge on the currently-implemented STATE_DATA for normal DATA/ACK management.
Right now the TFTP server is working here, but before submitting patches I have a question about the TftpHandler() callback function.
TftpHandler() is of type:
include/net.h:80: typedef void rxhand_f(uchar *, unsigned, unsigned, unsigned);
The 4 parameters are: packet pointer, UDP source port, UDP destination port and packet length.
Upon reception of a Write Request packet, the TFTP server needs to know the remote (client) IP address, or it won't be able to send any packet back.
This is different from currently implemented network file transfer modes, which are always initiated by U-Boot in client mode, and thus the remote (server) IP address is known a priori.
I plan to add a new parameter to the rxhand_t, and change accordingly all handlers to have the new (unused) parameter:
- typedef void rxhand_f(uchar *, unsigned, unsigned, unsigned); + typedef void rxhand_f(uchar *pkt, unsigned dest_port, IPaddr_t src_ip, unsigned src_port, unsigned len);
Is this ok, or do you foresee any issues with the function footprint or anything else?
Thanks, Luca
This patch series adds to U-Boot the ability to receive a file via TFTP acting as a server, not a client.
The implementation is kept simple: - receive only (accept WRQ from remote client, not RRQ); - the Filename in the WRQ is ignored: the destination is always a user-provided memory location; - binary transfers only: the Mode in the WRQ is ignored; this is allowed by RFC1350 (section 5); - no TFTP Option Extensions (RFC2347); - no TFTP multicast.
The implementation is discussed here: http://lists.denx.de/pipermail/u-boot/2011-April/090405.html
Once it has started, the server is stopped like the client is: on a complete file reception, Ctrl-C and after waiting 5 seconds for 10 times.
The first four patches are preliminary cleanups and extensions to the current code. Most important, the second patch adds to incoming packet handlers an argument containing the source IP address, and has impact in many places in the networking code.
The fifth patch implements the core TFTP server.
The last patch adds a user command to launch the server.
A note about checkpatch.pl. Some of these patches do have checkpatch errors and/or warnings. These are all issues that were already present in the pre-existing code. An example from patch 2:
static void -PingHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len) +PingHandler (uchar * pkt, unsigned dest, IPaddr_t sip, unsigned src, +unsigned len) {
Raises: * ERROR: "foo * bar" should be "foo *bar" * WARNING: space prohibited between function name and open parenthesis '('
As I didn't touch the pointer parameters nor the stuff before the '(' (but I changed the line somewhere else), should I also fix the checkpatch issues? And in case, should it be a separate patch to make it cleaner?
Also please take a look at the note in patch number 5.
Luca
Luca Ceresoli (6): README: remove spurious line NET: pass source IP address to packet handlers TFTP: rename "server" to "remote" TFTP: rename STATE_RRQ to STATE_SEND_RRQ TFTP: net/tftp.c: add server mode receive TFTP: add tftpsrv command
README | 2 +- common/cmd_net.c | 14 ++++++ drivers/net/netconsole.c | 5 +- include/net.h | 18 +++++--- net/bootp.c | 9 +++- net/dns.c | 2 +- net/net.c | 35 +++++++++------ net/nfs.c | 2 +- net/rarp.c | 3 +- net/sntp.c | 3 +- net/tftp.c | 109 ++++++++++++++++++++++++++++++++++++--------- net/tftp.h | 6 +++ 12 files changed, 157 insertions(+), 51 deletions(-)
Hi Luca,
Le 14/04/2011 17:52, Luca Ceresoli a écrit :
A note about checkpatch.pl.
:)
Some of these patches do have checkpatch errors and/or warnings. These are all issues that were already present in the pre-existing code. An example from patch 2:
static void -PingHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len) +PingHandler (uchar * pkt, unsigned dest, IPaddr_t sip, unsigned src, +unsigned len) {
Raises:
- ERROR: "foo * bar" should be "foo *bar"
- WARNING: space prohibited between function name and open parenthesis '('
As I didn't touch the pointer parameters nor the stuff before the '(' (but I changed the line somewhere else), should I also fix the checkpatch issues? And in case, should it be a separate patch to make it cleaner?
My opinion is that you should make sure that at least the code you touch is checkpatch-clean, so yes, you should fix that; but there is no need to submit 'checkpatch-compliance' patches. Just fix the line here so that checkpatch does not complain.
Amicalement,
Hi, here's the tftpsrv patch series, with checkpatch warnings fixed. Only patches 2 and 3 are actually changed. Patch 1 is unchanged and patches 4-6 have been simply rebased.
There are still a few checkpatch issues (!), though, that I think can be ignored and I'm not going to fix unless there's a specific request.
Here they are:
WARNING: consider using kstrto* in preference to simple_strtol #125: FILE: net/tftp.c:619:
TftpRemotePort = simple_strtol(ep, NULL, 10);
Which is Linux-only
WARNING: do not add new typedefs #61: FILE: include/net.h:367: +typedef enum { BOOTP, RARP, ARP, TFTP, DHCP, PING, DNS, NFS, CDP, NETCONS, SNTP,
Is a typedef *enum* considered bad in U-Boot?
Luca
Luca Ceresoli (6): README: remove spurious line NET: pass source IP address to packet handlers TFTP: rename "server" to "remote" TFTP: rename STATE_RRQ to STATE_SEND_RRQ TFTP: net/tftp.c: add server mode receive TFTP: add tftpsrv command
README | 2 +- common/cmd_net.c | 14 +++++ drivers/net/netconsole.c | 5 +- include/net.h | 18 +++++-- net/bootp.c | 9 ++- net/dns.c | 2 +- net/net.c | 40 +++++++++------ net/nfs.c | 2 +- net/rarp.c | 3 +- net/sntp.c | 3 +- net/tftp.c | 123 +++++++++++++++++++++++++++++++++++----------- net/tftp.h | 6 ++ 12 files changed, 167 insertions(+), 60 deletions(-)
Hi Luca,
Hi, here's the tftpsrv patch series, with checkpatch warnings fixed. Only patches 2 and 3 are actually changed. Patch 1 is unchanged and patches 4-6 have been simply rebased.
There are still a few checkpatch issues (!), though, that I think can be ignored and I'm not going to fix unless there's a specific request.
Here they are:
WARNING: consider using kstrto* in preference to simple_strtol #125: FILE: net/tftp.c:619:
TftpRemotePort = simple_strtol(ep, NULL, 10);Which is Linux-only
WARNING: do not add new typedefs #61: FILE: include/net.h:367: +typedef enum { BOOTP, RARP, ARP, TFTP, DHCP, PING, DNS, NFS, CDP, NETCONS, SNTP,
Is a typedef *enum* considered bad in U-Boot?
For new code yes. We (well at least I) much rather prefer to see what type a variable is without looking up the typedef for it.
Cheers Detlev
This patch series adds to U-Boot the ability to receive a file via TFTP acting as a server, instead of a client.
I rebased v3 on top of the net/tftp.c cleanup work that I submitted recently (http://lists.denx.de/pipermail/u-boot/2011-May/092599.html) and removed all the cleanups from this patchset.
Patches 1 and 2 of v2 have already been committed to master, so the remaining patches shifted back by two.
Finally, I incorporated all changes requested on the ml.
Description:
The implementation is kept simple: - receive only (accept WRQ from remote client, not RRQ); - the Filename in the WRQ is ignored: the destination is always a user-provided memory location; - binary transfers only: the Mode in the WRQ is ignored; this is allowed by RFC1350 (section 5); - no TFTP Option Extensions (RFC2347); - no TFTP multicast.
The implementation is discussed here: http://lists.denx.de/pipermail/u-boot/2011-April/090405.html
Once it has started, the server is stopped like the client is: on a complete file reception, Ctrl-C and after waiting 5 seconds for 10 times.
The first two patches are preliminary cleanups and extensions to the current code.
The third patch implements the core TFTP server.
The fourth patch adds a user command to launch the server.
I also added a trailing patch (#5 in this series), a trivial typo fix requested by Detlev.
Luca
Luca Ceresoli (5): TFTP: replace "server" with "remote" in local variable names TFTP: rename STATE_RRQ to STATE_SEND_RRQ TFTP: net/tftp.c: add server mode receive TFTP: add tftpsrv command net/tftp.c: fix typo
README | 1 + common/cmd_net.c | 17 +++++++++ include/net.h | 3 +- net/net.c | 7 +++- net/tftp.c | 98 +++++++++++++++++++++++++++++++++++++++++------------ net/tftp.h | 6 +++ 6 files changed, 108 insertions(+), 24 deletions(-)
With the upcoming TFTP server implementation, the remote node can be either a client or a server, so avoid ambiguities.
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
--- Changes in v2: - fixed checkpatch issues.
Changes in v3: - rebased on top of the net/tftp.c cleanup; - renamed also local variable ServerNet to RemoteNet in TftpStart(); - clarified commit message.
net/tftp.c | 28 ++++++++++++++-------------- 1 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c index 0f74e6b..b9d0f3b 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -58,9 +58,9 @@ enum { TFTP_ERR_FILE_ALREADY_EXISTS = 6, };
-static IPaddr_t TftpServerIP; +static IPaddr_t TftpRemoteIP; /* The UDP port at their end */ -static int TftpServerPort; +static int TftpRemotePort; /* The UDP port at our end */ static int TftpOurPort; static int TftpTimeoutCount; @@ -289,7 +289,7 @@ TftpSend(void) break; }
- NetSendUDPPacket(NetServerEther, TftpServerIP, TftpServerPort, + NetSendUDPPacket(NetServerEther, TftpRemoteIP, TftpRemotePort, TftpOurPort, len); }
@@ -309,7 +309,7 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, #endif return; } - if (TftpState != STATE_RRQ && src != TftpServerPort) + if (TftpState != STATE_RRQ && src != TftpRemotePort) return;
if (len < 2) @@ -333,7 +333,7 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, pkt, pkt + strlen((char *)pkt) + 1); TftpState = STATE_OACK; - TftpServerPort = src; + TftpRemotePort = src; /* * Check for 'blksize' option. * Careful: "i" is signed, "len" is unsigned, thus @@ -405,7 +405,7 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, if (TftpState == STATE_RRQ || TftpState == STATE_OACK) { /* first block received */ TftpState = STATE_DATA; - TftpServerPort = src; + TftpRemotePort = src; TftpLastBlock = 0; TftpBlockWrap = 0; TftpBlockWrapOffset = 0; @@ -440,7 +440,7 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
/* * Acknoledge the block just received, which will prompt - * the server for the next one. + * the remote for the next one. */ #ifdef CONFIG_MCAST_TFTP /* if I am the MasterClient, actively calculate what my next @@ -569,7 +569,7 @@ TftpStart(void) debug("TFTP blocksize = %i, timeout = %ld ms\n", TftpBlkSizeOption, TftpTimeoutMSecs);
- TftpServerIP = NetServerIP; + TftpRemoteIP = NetServerIP; if (BootFile[0] == '\0') { sprintf(default_filename, "%02lX%02lX%02lX%02lX.img", NetOurIP & 0xFF, @@ -589,7 +589,7 @@ TftpStart(void) strncpy(tftp_filename, BootFile, MAX_LEN); tftp_filename[MAX_LEN-1] = 0; } else { - TftpServerIP = string_to_ip(BootFile); + TftpRemoteIP = string_to_ip(BootFile); strncpy(tftp_filename, p + 1, MAX_LEN); tftp_filename[MAX_LEN-1] = 0; } @@ -599,14 +599,14 @@ TftpStart(void) printf("Using %s device\n", eth_get_name()); #endif printf("TFTP from server %pI4" - "; our IP address is %pI4", &TftpServerIP, &NetOurIP); + "; our IP address is %pI4", &TftpRemoteIP, &NetOurIP);
/* Check if we need to send across this subnet */ if (NetOurGatewayIP && NetOurSubnetMask) { IPaddr_t OurNet = NetOurIP & NetOurSubnetMask; - IPaddr_t ServerNet = TftpServerIP & NetOurSubnetMask; + IPaddr_t RemoteNet = TftpRemoteIP & NetOurSubnetMask;
- if (OurNet != ServerNet) + if (OurNet != RemoteNet) printf("; sending through gateway %pI4", &NetOurGatewayIP); } @@ -630,7 +630,7 @@ TftpStart(void) NetSetTimeout(TftpTimeoutMSecs, TftpTimeout); NetSetHandler(TftpHandler);
- TftpServerPort = WELL_KNOWN_PORT; + TftpRemotePort = WELL_KNOWN_PORT; TftpTimeoutCount = 0; TftpState = STATE_RRQ; /* Use a pseudo-random port unless a specific port is set */ @@ -639,7 +639,7 @@ TftpStart(void) #ifdef CONFIG_TFTP_PORT ep = getenv("tftpdstp"); if (ep != NULL) - TftpServerPort = simple_strtol(ep, NULL, 10); + TftpRemotePort = simple_strtol(ep, NULL, 10); ep = getenv("tftpsrcp"); if (ep != NULL) TftpOurPort = simple_strtol(ep, NULL, 10);
Hi Luca,
With the upcoming TFTP server implementation, the remote node can be either a client or a server, so avoid ambiguities.
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
Acked-by: Detlev Zundel dzu@denx.de
Cheers Detlev
Dear Luca Ceresoli,
In message 1305626621-15008-2-git-send-email-luca.ceresoli@comelit.it you wrote:
With the upcoming TFTP server implementation, the remote node can be either a client or a server, so avoid ambiguities.
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
Changes in v2:
- fixed checkpatch issues.
Changes in v3:
- rebased on top of the net/tftp.c cleanup;
- renamed also local variable ServerNet to RemoteNet in TftpStart();
- clarified commit message.
net/tftp.c | 28 ++++++++++++++-------------- 1 files changed, 14 insertions(+), 14 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk
With the upcoming TFTP server implementation, requests can be either outgoing or incoming, so avoid ambiguities.
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
--- Changes in v2: none.
Changes in v3: - rebased on top of the net/tftp.c cleanup.
net/tftp.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c index b9d0f3b..6386740 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -80,7 +80,7 @@ static int TftpTsize; static short TftpNumchars; #endif
-#define STATE_RRQ 1 +#define STATE_SEND_RRQ 1 #define STATE_DATA 2 #define STATE_TOO_LARGE 3 #define STATE_BAD_MAGIC 4 @@ -215,7 +215,7 @@ TftpSend(void)
switch (TftpState) {
- case STATE_RRQ: + case STATE_SEND_RRQ: xp = pkt; s = (ushort *)pkt; *s++ = htons(TFTP_RRQ); @@ -309,7 +309,7 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, #endif return; } - if (TftpState != STATE_RRQ && src != TftpRemotePort) + if (TftpState != STATE_SEND_RRQ && src != TftpRemotePort) return;
if (len < 2) @@ -399,10 +399,10 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, puts("\n\t "); }
- if (TftpState == STATE_RRQ) + if (TftpState == STATE_SEND_RRQ) debug("Server did not acknowledge timeout option!\n");
- if (TftpState == STATE_RRQ || TftpState == STATE_OACK) { + if (TftpState == STATE_SEND_RRQ || TftpState == STATE_OACK) { /* first block received */ TftpState = STATE_DATA; TftpRemotePort = src; @@ -632,7 +632,7 @@ TftpStart(void)
TftpRemotePort = WELL_KNOWN_PORT; TftpTimeoutCount = 0; - TftpState = STATE_RRQ; + TftpState = STATE_SEND_RRQ; /* Use a pseudo-random port unless a specific port is set */ TftpOurPort = 1024 + (get_timer(0) % 3072);
Luca Ceresoli luca.ceresoli@comelit.it writes:
With the upcoming TFTP server implementation, requests can be either outgoing or incoming, so avoid ambiguities.
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
Acked-by: Detlev Zundel dzu@denx.de
Cheers Detlev
Dear Luca Ceresoli,
In message 1305626621-15008-3-git-send-email-luca.ceresoli@comelit.it you wrote:
With the upcoming TFTP server implementation, requests can be either outgoing or incoming, so avoid ambiguities.
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
Changes in v2: none.
Changes in v3:
- rebased on top of the net/tftp.c cleanup.
net/tftp.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
--- Changes in v2: none.
Changes in v3: - rebased on top of the net/tftp.c cleanup; - removed all #ifdefs that used to remove negligible amounts of compiled code, at the cost of a much less readable source file; after measurements, it turned out this change does not increase code size.
net/tftp.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- net/tftp.h | 6 +++++ 2 files changed, 64 insertions(+), 4 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c index 6386740..6d44298 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@denx.de + * Copyright 2011 Comelit Group SpA, + * Luca Ceresoli luca.ceresoli@comelit.it */
#include <common.h> @@ -85,6 +87,7 @@ static short TftpNumchars; #define STATE_TOO_LARGE 3 #define STATE_BAD_MAGIC 4 #define STATE_OACK 5 +#define STATE_RECV_WRQ 6
/* default TFTP block size */ #define TFTP_BLOCK_SIZE 512 @@ -257,6 +260,8 @@ TftpSend(void) (Mapsize*8), 0); /*..falling..*/ #endif + + case STATE_RECV_WRQ: case STATE_DATA: xp = pkt; s = (ushort *)pkt; @@ -309,7 +314,8 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, #endif return; } - if (TftpState != STATE_SEND_RRQ && src != TftpRemotePort) + if (TftpState != STATE_SEND_RRQ && src != TftpRemotePort && + TftpState != STATE_RECV_WRQ) return;
if (len < 2) @@ -322,12 +328,24 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, switch (ntohs(proto)) {
case TFTP_RRQ: - case TFTP_WRQ: case TFTP_ACK: break; default: break;
+#ifdef CONFIG_CMD_TFTPSRV + case TFTP_WRQ: + debug("Got WRQ\n"); + TftpRemoteIP = sip; + TftpRemotePort = src; + TftpOurPort = 1024 + (get_timer(0) % 3072); + TftpLastBlock = 0; + TftpBlockWrap = 0; + TftpBlockWrapOffset = 0; + TftpSend(); /* Send ACK(0) */ + break; +#endif + case TFTP_OACK: debug("Got OACK: %s %s\n", pkt, @@ -402,7 +420,8 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, if (TftpState == STATE_SEND_RRQ) debug("Server did not acknowledge timeout option!\n");
- if (TftpState == STATE_SEND_RRQ || TftpState == STATE_OACK) { + if (TftpState == STATE_SEND_RRQ || TftpState == STATE_OACK || + TftpState == STATE_RECV_WRQ) { /* first block received */ TftpState = STATE_DATA; TftpRemotePort = src; @@ -537,7 +556,8 @@ TftpTimeout(void) } else { puts("T "); NetSetTimeout(TftpTimeoutMSecs, TftpTimeout); - TftpSend(); + if (TftpState != STATE_RECV_WRQ) + TftpSend(); } }
@@ -661,6 +681,40 @@ TftpStart(void) TftpSend(); }
+#ifdef CONFIG_CMD_TFTPSRV +void +TftpStartServer(void) +{ + tftp_filename[0] = 0; + +#if defined(CONFIG_NET_MULTI) + printf("Using %s device\n", eth_get_name()); +#endif + printf("Listening for TFTP transfer on %pI4\n", &NetOurIP); + printf("Load address: 0x%lx\n", load_addr); + + puts("Loading: *\b"); + + TftpTimeoutCountMax = TIMEOUT_COUNT; + TftpTimeoutCount = 0; + TftpTimeoutMSecs = TIMEOUT; + NetSetTimeout(TftpTimeoutMSecs, TftpTimeout); + + /* Revert TftpBlkSize to dflt */ + TftpBlkSize = TFTP_BLOCK_SIZE; + TftpBlock = 0; + TftpOurPort = WELL_KNOWN_PORT; + +#ifdef CONFIG_TFTP_TSIZE + TftpTsize = 0; + TftpNumchars = 0; +#endif + + TftpState = STATE_RECV_WRQ; + NetSetHandler(TftpHandler); +} +#endif /* CONFIG_CMD_TFTPSRV */ + #ifdef CONFIG_MCAST_TFTP /* Credits: atftp project. */ diff --git a/net/tftp.h b/net/tftp.h index e3dfb26..3abdf7b 100644 --- a/net/tftp.h +++ b/net/tftp.h @@ -2,6 +2,8 @@ * LiMon - BOOTP/TFTP. * * Copyright 1994, 1995, 2000 Neil Russell. + * Copyright 2011 Comelit Group SpA + * Luca Ceresoli luca.ceresoli@comelit.it * (See License) */
@@ -16,6 +18,10 @@ /* tftp.c */ extern void TftpStart (void); /* Begin TFTP get */
+#ifdef CONFIG_CMD_TFTPSRV +extern void TftpStartServer(void); /* Wait for incoming TFTP put */ +#endif + /**********************************************************************/
#endif /* __TFTP_H__ */
Hi Luca,
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
Acked-by: Detlev Zundel dzu@denx.de
Just for the future:
[...]
diff --git a/net/tftp.h b/net/tftp.h index e3dfb26..3abdf7b 100644 --- a/net/tftp.h +++ b/net/tftp.h @@ -2,6 +2,8 @@
- LiMon - BOOTP/TFTP.
- Copyright 1994, 1995, 2000 Neil Russell.
- Copyright 2011 Comelit Group SpA
*/
Luca Ceresoli <luca.ceresoli@comelit.it>- (See License)
@@ -16,6 +18,10 @@ /* tftp.c */ extern void TftpStart (void); /* Begin TFTP get */
+#ifdef CONFIG_CMD_TFTPSRV +extern void TftpStartServer(void); /* Wait for incoming TFTP put */ +#endif
/**********************************************************************/
#endif /* __TFTP_H__ */
I think we don't need the ifdefs in header files, or am I missing something? I _really_ like to avoid ifdefs wherever we can ;)
Cheers Detlev
Dear Luca Ceresoli,
In message 1305626621-15008-4-git-send-email-luca.ceresoli@comelit.it you wrote:
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
Changes in v2: none.
Changes in v3:
- rebased on top of the net/tftp.c cleanup;
- removed all #ifdefs that used to remove negligible amounts of compiled code, at the cost of a much less readable source file; after measurements, it turned out this change does not increase code size.
net/tftp.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- net/tftp.h | 6 +++++ 2 files changed, 64 insertions(+), 4 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
--- Changes in v2: none.
Changes in v3: - rebased on top of the net/tftp.c cleanup; - made do_tftpsrv() static; - improved tftpsrv command help; - removed all #ifdefs that used to remove negligible amounts of compiled code, at the cost of a much less readable source file; after measurements, it turned out this change does not increase code size.
README | 1 + common/cmd_net.c | 17 +++++++++++++++++ include/net.h | 3 ++- net/net.c | 7 ++++++- 4 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/README b/README index 6f3748d..ed73981 100644 --- a/README +++ b/README @@ -709,6 +709,7 @@ The following options need to be configured: (requires CONFIG_CMD_MEMORY) CONFIG_CMD_SOURCE "source" command Support CONFIG_CMD_SPI * SPI serial bus support + CONFIG_CMD_TFTPSRV * TFTP transfer in server mode CONFIG_CMD_USB * USB support CONFIG_CMD_VFD * VFD support (TRAB) CONFIG_CMD_CDP * Cisco Discover Protocol support diff --git a/common/cmd_net.c b/common/cmd_net.c index 8c6f5c8..b2c9355 100644 --- a/common/cmd_net.c +++ b/common/cmd_net.c @@ -52,6 +52,23 @@ U_BOOT_CMD( "[loadAddress] [[hostIPaddr:]bootfilename]" );
+#ifdef CONFIG_CMD_TFTPSRV +static int do_tftpsrv(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) +{ + return netboot_common(TFTPSRV, cmdtp, argc, argv); +} + +U_BOOT_CMD( + tftpsrv, 2, 1, do_tftpsrv, + "act as a TFTP server and boot the first received file", + "[loadAddress]\n" + "Listen for an incoming TFTP transfer, receive a file and boot it.\n" + "The transfer is aborted if a transfer has not been started after\n" + "about 50 seconds or if Ctrl-C is pressed." +); +#endif + + #ifdef CONFIG_CMD_RARP int do_rarpb (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { diff --git a/include/net.h b/include/net.h index 01f7159..018a744 100644 --- a/include/net.h +++ b/include/net.h @@ -364,7 +364,8 @@ extern int NetState; /* Network loop state */ extern int NetRestartWrap; /* Tried all network devices */ #endif
-typedef enum { BOOTP, RARP, ARP, TFTP, DHCP, PING, DNS, NFS, CDP, NETCONS, SNTP } proto_t; +typedef enum { BOOTP, RARP, ARP, TFTP, DHCP, PING, DNS, NFS, CDP, NETCONS, SNTP, + TFTPSRV } proto_t;
/* from net/net.c */ extern char BootFile[128]; /* Boot File name */ diff --git a/net/net.c b/net/net.c index 2abf879..7a93542 100644 --- a/net/net.c +++ b/net/net.c @@ -423,7 +423,11 @@ restart: /* always use ARP to get server ethernet address */ TftpStart(); break; - +#ifdef CONFIG_CMD_TFTPSRV + case TFTPSRV: + TftpStartServer(); + break; +#endif #if defined(CONFIG_CMD_DHCP) case DHCP: BootpTry = 0; @@ -1791,6 +1795,7 @@ common: /* Fall through */
case NETCONS: + case TFTPSRV: if (NetOurIP == 0) { puts("*** ERROR: `ipaddr' not set\n"); return 1;
Luca Ceresoli luca.ceresoli@comelit.it writes:
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
Acked-by: Detlev Zundel dzu@denx.de
Cheers Detlev
Dear Luca Ceresoli,
In message 1305626621-15008-5-git-send-email-luca.ceresoli@comelit.it you wrote:
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
Changes in v2: none.
Changes in v3:
- rebased on top of the net/tftp.c cleanup;
- made do_tftpsrv() static;
- improved tftpsrv command help;
- removed all #ifdefs that used to remove negligible amounts of compiled code, at the cost of a much less readable source file; after measurements, it turned out this change does not increase code size.
README | 1 + common/cmd_net.c | 17 +++++++++++++++++ include/net.h | 3 ++- net/net.c | 7 ++++++- 4 files changed, 26 insertions(+), 2 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
--- Changes in v3: this patch is new in v3.
net/tftp.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c index 6d44298..a893e02 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -458,7 +458,7 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, store_block(TftpBlock - 1, pkt + 2, len);
/* - * Acknoledge the block just received, which will prompt + * Acknowledge the block just received, which will prompt * the remote for the next one. */ #ifdef CONFIG_MCAST_TFTP
Luca Ceresoli luca.ceresoli@comelit.it writes:
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
Acked-by: Detlev Zundel dzu@denx.de
Cheers Detlev
Dear Luca Ceresoli,
In message 1305626621-15008-6-git-send-email-luca.ceresoli@comelit.it you wrote:
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
Changes in v3: this patch is new in v3.
net/tftp.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de --- Changes in v2: none.
README | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/README b/README index 4917e26..b9b0fcb 100644 --- a/README +++ b/README @@ -1299,7 +1299,6 @@ The following options need to be configured: driver in use must provide a function: mcast() to join/leave a multicast group.
- CONFIG_BOOTP_RANDOM_DELAY - BOOTP Recovery Mode: CONFIG_BOOTP_RANDOM_DELAY
Hi Luca,
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
Changes in v2: none.
README | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/README b/README index 4917e26..b9b0fcb 100644 --- a/README +++ b/README @@ -1299,7 +1299,6 @@ The following options need to be configured: driver in use must provide a function: mcast() to join/leave a multicast group.
CONFIG_BOOTP_RANDOM_DELAY- BOOTP Recovery Mode: CONFIG_BOOTP_RANDOM_DELAY
Acked-by: Detlev Zundel dzu@denx.de
This is needed for the upcoming TFTP server implementation.
This also simplifies PingHandler() and fixes rxhand_f documentation.
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de --- Changes in v2: - fixed checkpatch issues.
drivers/net/netconsole.c | 5 +++-- include/net.h | 15 ++++++++++----- net/bootp.c | 9 ++++++--- net/dns.c | 2 +- net/net.c | 30 +++++++++++++++++------------- net/nfs.c | 2 +- net/rarp.c | 3 ++- net/sntp.c | 3 ++- net/tftp.c | 3 ++- 9 files changed, 44 insertions(+), 28 deletions(-)
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index e27bb3e..e40efb8 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -40,13 +40,14 @@ static short nc_port; /* source/target port */ static const char *output_packet; /* used by first send udp */ static int output_packet_len = 0;
-static void nc_wait_arp_handler (uchar * pkt, unsigned dest, unsigned src, +static void nc_wait_arp_handler(uchar *pkt, unsigned dest, + IPaddr_t sip, unsigned src, unsigned len) { NetState = NETLOOP_SUCCESS; /* got arp reply - quit net loop */ }
-static void nc_handler (uchar * pkt, unsigned dest, unsigned src, +static void nc_handler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len) { if (input_size) diff --git a/include/net.h b/include/net.h index 95ef8ab..01f7159 100644 --- a/include/net.h +++ b/include/net.h @@ -72,12 +72,17 @@ typedef ulong IPaddr_t;
-/* - * The current receive packet handler. Called with a pointer to the - * application packet, and a protocol type (PORT_BOOTPC or PORT_TFTP). - * All other packets are dealt with without calling the handler. +/** + * An incoming packet handler. + * @param pkt pointer to the application packet + * @param dport destination UDP port + * @param sip source IP address + * @param sport source UDP port + * @param len packet length */ -typedef void rxhand_f(uchar *, unsigned, unsigned, unsigned); +typedef void rxhand_f(uchar *pkt, unsigned dport, + IPaddr_t sip, unsigned sport, + unsigned len);
/* * A timeout handler. Called after time interval has expired. diff --git a/net/bootp.c b/net/bootp.c index 87b027e..4db63cb 100644 --- a/net/bootp.c +++ b/net/bootp.c @@ -44,7 +44,8 @@ ulong seed1, seed2; dhcp_state_t dhcp_state = INIT; unsigned long dhcp_leasetime = 0; IPaddr_t NetDHCPServerIP = 0; -static void DhcpHandler(uchar * pkt, unsigned dest, unsigned src, unsigned len); +static void DhcpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, + unsigned len);
/* For Debug */ #if 0 @@ -282,7 +283,8 @@ static void BootpVendorProcess (u8 * ext, int size) * Handle a BOOTP received packet. */ static void -BootpHandler(uchar * pkt, unsigned dest, unsigned src, unsigned len) +BootpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, + unsigned len) { Bootp_t *bp; char *s; @@ -858,7 +860,8 @@ static void DhcpSendRequestPkt(Bootp_t *bp_offer) * Handle DHCP received packets. */ static void -DhcpHandler(uchar * pkt, unsigned dest, unsigned src, unsigned len) +DhcpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, + unsigned len) { Bootp_t *bp = (Bootp_t *)pkt;
diff --git a/net/dns.c b/net/dns.c index bb3e3f5..b51d1bd 100644 --- a/net/dns.c +++ b/net/dns.c @@ -101,7 +101,7 @@ DnsTimeout(void) }
static void -DnsHandler(uchar *pkt, unsigned dest, unsigned src, unsigned len) +DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len) { struct header *header; const unsigned char *p, *e, *s; diff --git a/net/net.c b/net/net.c index a609632..132f99b 100644 --- a/net/net.c +++ b/net/net.c @@ -555,7 +555,8 @@ startAgainTimeout(void) }
static void -startAgainHandler(uchar * pkt, unsigned dest, unsigned src, unsigned len) +startAgainHandler(uchar *pkt, unsigned dest, IPaddr_t sip, + unsigned src, unsigned len) { /* Totally ignore the packet */ } @@ -752,13 +753,10 @@ PingTimeout (void) }
static void -PingHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len) +PingHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, + unsigned len) { - IPaddr_t tmp; - volatile IP_t *ip = (volatile IP_t *)pkt; - - tmp = NetReadIP((void *)&ip->ip_src); - if (tmp != NetPingIP) + if (sip != NetPingIP) return;
NetState = NETLOOP_SUCCESS; @@ -990,7 +988,8 @@ CDPTimeout (void) }
static void -CDPDummyHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len) +CDPDummyHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, + unsigned len) { /* nothing */ } @@ -1304,6 +1303,7 @@ NetReceive(volatile uchar * inpkt, int len) IP_t *ip; ARP_t *arp; IPaddr_t tmp; + IPaddr_t src_ip; int x; uchar *pkt; #if defined(CONFIG_CMD_CDP) @@ -1477,7 +1477,7 @@ NetReceive(volatile uchar * inpkt, int len) memcpy(NetArpWaitPacketMAC, &arp->ar_data[0], 6);
#ifdef CONFIG_NETCONSOLE - (*packetHandler)(0,0,0,0); + (*packetHandler)(0, 0, 0, 0, 0); #endif /* modify header, and transmit it */ memcpy(((Ethernet_t *)NetArpWaitTxPacket)->et_dest, NetArpWaitPacketMAC, 6); @@ -1517,7 +1517,7 @@ NetReceive(volatile uchar * inpkt, int len) NetCopyIP(&NetServerIP, &arp->ar_data[ 6]); memcpy (NetServerEther, &arp->ar_data[ 0], 6);
- (*packetHandler)(0,0,0,0); + (*packetHandler)(0, 0, 0, 0, 0); } break; #endif @@ -1557,6 +1557,8 @@ NetReceive(volatile uchar * inpkt, int len) #endif return; } + /* Read source IP address for later use */ + src_ip = NetReadIP(&ip->ip_src); /* * The function returns the unchanged packet if it's not * a fragment, and either the complete packet or NULL if @@ -1596,11 +1598,12 @@ NetReceive(volatile uchar * inpkt, int len) * IP header OK. Pass the packet to the current handler. */ /* XXX point to ip packet */ - (*packetHandler)((uchar *)ip, 0, 0, 0); + (*packetHandler)((uchar *)ip, 0, src_ip, 0, 0); return; case ICMP_ECHO_REQUEST: - debug("Got ICMP ECHO REQUEST, return %d bytes \n", - ETHER_HDR_SIZE + len); + debug("Got ICMP ECHO REQUEST, " + "return %d bytes\n", + ETHER_HDR_SIZE + len);
memcpy (&et->et_dest[0], &et->et_src[0], 6); memcpy (&et->et_src[ 0], NetOurEther, 6); @@ -1678,6 +1681,7 @@ NetReceive(volatile uchar * inpkt, int len) */ (*packetHandler)((uchar *)ip +IP_HDR_SIZE, ntohs(ip->udp_dst), + src_ip, ntohs(ip->udp_src), ntohs(ip->udp_len) - 8); break; diff --git a/net/nfs.c b/net/nfs.c index d11bb4c..f76f60d 100644 --- a/net/nfs.c +++ b/net/nfs.c @@ -580,7 +580,7 @@ NfsTimeout (void) }
static void -NfsHandler (uchar *pkt, unsigned dest, unsigned src, unsigned len) +NfsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len) { int rlen;
diff --git a/net/rarp.c b/net/rarp.c index 9444c03..94c86d3 100644 --- a/net/rarp.c +++ b/net/rarp.c @@ -43,7 +43,8 @@ int RarpTry; * Handle a RARP received packet. */ static void -RarpHandler(uchar * dummi0, unsigned dummi1, unsigned dummi2, unsigned dummi3) +RarpHandler(uchar *dummi0, unsigned dummi1, IPaddr_t sip, unsigned dummi2, + unsigned dummi3) { char *s; debug("Got good RARP\n"); diff --git a/net/sntp.c b/net/sntp.c index 76c10ec..82f2fe6 100644 --- a/net/sntp.c +++ b/net/sntp.c @@ -48,7 +48,8 @@ SntpTimeout (void) }
static void -SntpHandler (uchar *pkt, unsigned dest, unsigned src, unsigned len) +SntpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, + unsigned len) { struct sntp_pkt_t *rpktp = (struct sntp_pkt_t *)pkt; struct rtc_time tm; diff --git a/net/tftp.c b/net/tftp.c index ed559b7..00abec3 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -278,7 +278,8 @@ TftpSend (void)
static void -TftpHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len) +TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, + unsigned len) { ushort proto; ushort *s;
Hi Luca,
This is needed for the upcoming TFTP server implementation.
This also simplifies PingHandler() and fixes rxhand_f documentation.
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
Changes in v2:
- fixed checkpatch issues.
drivers/net/netconsole.c | 5 +++-- include/net.h | 15 ++++++++++----- net/bootp.c | 9 ++++++--- net/dns.c | 2 +- net/net.c | 30 +++++++++++++++++------------- net/nfs.c | 2 +- net/rarp.c | 3 ++- net/sntp.c | 3 ++- net/tftp.c | 3 ++- 9 files changed, 44 insertions(+), 28 deletions(-)
[...]
diff --git a/net/net.c b/net/net.c index a609632..132f99b 100644 --- a/net/net.c +++ b/net/net.c
[...]
@@ -1596,11 +1598,12 @@ NetReceive(volatile uchar * inpkt, int len) * IP header OK. Pass the packet to the current handler. */ /* XXX point to ip packet */
(*packetHandler)((uchar *)ip, 0, 0, 0);
(*packetHandler)((uchar *)ip, 0, src_ip, 0, 0); return; case ICMP_ECHO_REQUEST:
debug("Got ICMP ECHO REQUEST, return %d bytes \n",ETHER_HDR_SIZE + len);
debug("Got ICMP ECHO REQUEST, ""return %d bytes\n",ETHER_HDR_SIZE + len); memcpy (&et->et_dest[0], &et->et_src[0], 6); memcpy (&et->et_src[ 0], NetOurEther, 6);
This second hunk is not related to the patch at hand, so strictly speaking it should not be in here. It's not enough to NAK the patch however, just something to look out for in the future.
Apart from that the changes look good, so
Acked-by: Detlev Zundel dzu@denx.de
Il 19/04/2011 16:15, Detlev Zundel ha scritto:
Hi Luca,
This is needed for the upcoming TFTP server implementation.
This also simplifies PingHandler() and fixes rxhand_f documentation.
Signed-off-by: Luca Ceresoliluca.ceresoli@comelit.it Cc: Wolfgang Denkwd@denx.de
Changes in v2:
- fixed checkpatch issues.
drivers/net/netconsole.c | 5 +++-- include/net.h | 15 ++++++++++----- net/bootp.c | 9 ++++++--- net/dns.c | 2 +- net/net.c | 30 +++++++++++++++++------------- net/nfs.c | 2 +- net/rarp.c | 3 ++- net/sntp.c | 3 ++- net/tftp.c | 3 ++- 9 files changed, 44 insertions(+), 28 deletions(-)
[...]
diff --git a/net/net.c b/net/net.c index a609632..132f99b 100644 --- a/net/net.c +++ b/net/net.c
[...]
@@ -1596,11 +1598,12 @@ NetReceive(volatile uchar * inpkt, int len) * IP header OK. Pass the packet to the current handler. */ /* XXX point to ip packet */
(*packetHandler)((uchar *)ip, 0, 0, 0);
(*packetHandler)((uchar *)ip, 0, src_ip, 0, 0); return; case ICMP_ECHO_REQUEST:
debug("Got ICMP ECHO REQUEST, return %d bytes \n",ETHER_HDR_SIZE + len);
debug("Got ICMP ECHO REQUEST, ""return %d bytes\n",ETHER_HDR_SIZE + len); memcpy (&et->et_dest[0],&et->et_src[0], 6); memcpy (&et->et_src[ 0], NetOurEther, 6);This second hunk is not related to the patch at hand, so strictly speaking it should not be in here. It's not enough to NAK the patch however, just something to look out for in the future.
Yep, but it's needed for checkpatch compliance.
Luca
Dear Luca Ceresoli,
In message 1303143594-5345-3-git-send-email-luca.ceresoli@comelit.it you wrote:
This is needed for the upcoming TFTP server implementation.
This also simplifies PingHandler() and fixes rxhand_f documentation.
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
Changes in v2:
- fixed checkpatch issues.
drivers/net/netconsole.c | 5 +++-- include/net.h | 15 ++++++++++----- net/bootp.c | 9 ++++++--- net/dns.c | 2 +- net/net.c | 30 +++++++++++++++++------------- net/nfs.c | 2 +- net/rarp.c | 3 ++- net/sntp.c | 3 ++- net/tftp.c | 3 ++- 9 files changed, 44 insertions(+), 28 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk
With the upcoming TFTP server implementation, the remote node can be either a client or a server, so avoid ambiguities.
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de --- Changes in v2: - fixed checkpatch issues.
net/tftp.c | 42 +++++++++++++++++++++--------------------- 1 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c index 00abec3..da545c6 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -55,18 +55,18 @@ enum { TFTP_ERR_FILE_ALREADY_EXISTS = 6, };
-static IPaddr_t TftpServerIP; -static int TftpServerPort; /* The UDP port at their end */ -static int TftpOurPort; /* The UDP port at our end */ +static IPaddr_t TftpRemoteIP; +static int TftpRemotePort; /* The UDP port at their end */ +static int TftpOurPort; /* The UDP port at our end */ static int TftpTimeoutCount; -static ulong TftpBlock; /* packet sequence number */ -static ulong TftpLastBlock; /* last packet sequence number received */ -static ulong TftpBlockWrap; /* count of sequence number wraparounds */ -static ulong TftpBlockWrapOffset; /* memory offset due to wrapping */ +static ulong TftpBlock; /* packet sequence number */ +static ulong TftpLastBlock; /* last packet sequence number received */ +static ulong TftpBlockWrap; /* count of sequence number wraparounds */ +static ulong TftpBlockWrapOffset; /* memory offset due to wrapping */ static int TftpState; #ifdef CONFIG_TFTP_TSIZE -static int TftpTsize; /* The file size reported by the server */ -static short TftpNumchars; /* The number of hashes we printed */ +static int TftpTsize; /* The file size reported by the server */ +static short TftpNumchars; /* The number of hashes we printed */ #endif
#define STATE_RRQ 1 @@ -273,7 +273,8 @@ TftpSend (void) break; }
- NetSendUDPPacket(NetServerEther, TftpServerIP, TftpServerPort, TftpOurPort, len); + NetSendUDPPacket(NetServerEther, TftpRemoteIP, TftpRemotePort, + TftpOurPort, len); }
@@ -292,9 +293,8 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, #endif return; } - if (TftpState != STATE_RRQ && src != TftpServerPort) { + if (TftpState != STATE_RRQ && src != TftpRemotePort) return; - }
if (len < 2) { return; @@ -318,7 +318,7 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, pkt, pkt + strlen((char *)pkt) + 1); TftpState = STATE_OACK; - TftpServerPort = src; + TftpRemotePort = src; /* * Check for 'blksize' option. * Careful: "i" is signed, "len" is unsigned, thus @@ -386,7 +386,7 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, if (TftpState == STATE_RRQ || TftpState == STATE_OACK) { /* first block received */ TftpState = STATE_DATA; - TftpServerPort = src; + TftpRemotePort = src; TftpLastBlock = 0; TftpBlockWrap = 0; TftpBlockWrapOffset = 0; @@ -421,7 +421,7 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
/* * Acknoledge the block just received, which will prompt - * the server for the next one. + * the remote for the next one. */ #ifdef CONFIG_MCAST_TFTP /* if I am the MasterClient, actively calculate what my next @@ -548,7 +548,7 @@ TftpStart (void) debug("TFTP blocksize = %i, timeout = %ld ms\n", TftpBlkSizeOption, TftpTimeoutMSecs);
- TftpServerIP = NetServerIP; + TftpRemoteIP = NetServerIP; if (BootFile[0] == '\0') { sprintf(default_filename, "%02lX%02lX%02lX%02lX.img", NetOurIP & 0xFF, @@ -568,7 +568,7 @@ TftpStart (void) strncpy(tftp_filename, BootFile, MAX_LEN); tftp_filename[MAX_LEN-1] = 0; } else { - TftpServerIP = string_to_ip (BootFile); + TftpRemoteIP = string_to_ip(BootFile); strncpy(tftp_filename, p + 1, MAX_LEN); tftp_filename[MAX_LEN-1] = 0; } @@ -578,12 +578,12 @@ TftpStart (void) printf ("Using %s device\n", eth_get_name()); #endif printf("TFTP from server %pI4" - "; our IP address is %pI4", &TftpServerIP, &NetOurIP); + "; our IP address is %pI4", &TftpRemoteIP, &NetOurIP);
/* Check if we need to send across this subnet */ if (NetOurGatewayIP && NetOurSubnetMask) { IPaddr_t OurNet = NetOurIP & NetOurSubnetMask; - IPaddr_t ServerNet = TftpServerIP & NetOurSubnetMask; + IPaddr_t ServerNet = TftpRemoteIP & NetOurSubnetMask;
if (OurNet != ServerNet) printf("; sending through gateway %pI4", &NetOurGatewayIP); @@ -608,7 +608,7 @@ TftpStart (void) NetSetTimeout (TftpTimeoutMSecs, TftpTimeout); NetSetHandler (TftpHandler);
- TftpServerPort = WELL_KNOWN_PORT; + TftpRemotePort = WELL_KNOWN_PORT; TftpTimeoutCount = 0; TftpState = STATE_RRQ; /* Use a pseudo-random port unless a specific port is set */ @@ -616,7 +616,7 @@ TftpStart (void)
#ifdef CONFIG_TFTP_PORT if ((ep = getenv("tftpdstp")) != NULL) { - TftpServerPort = simple_strtol(ep, NULL, 10); + TftpRemotePort = simple_strtol(ep, NULL, 10); } if ((ep = getenv("tftpsrcp")) != NULL) { TftpOurPort= simple_strtol(ep, NULL, 10);
Hi Luca,
With the upcoming TFTP server implementation, the remote node can be either a client or a server, so avoid ambiguities.
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
Changes in v2:
- fixed checkpatch issues.
net/tftp.c | 42 +++++++++++++++++++++--------------------- 1 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c index 00abec3..da545c6 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -55,18 +55,18 @@ enum { TFTP_ERR_FILE_ALREADY_EXISTS = 6, };
-static IPaddr_t TftpServerIP; -static int TftpServerPort; /* The UDP port at their end */ -static int TftpOurPort; /* The UDP port at our end */ +static IPaddr_t TftpRemoteIP; +static int TftpRemotePort; /* The UDP port at their end */ +static int TftpOurPort; /* The UDP port at our end */ static int TftpTimeoutCount; -static ulong TftpBlock; /* packet sequence number */ -static ulong TftpLastBlock; /* last packet sequence number received */ -static ulong TftpBlockWrap; /* count of sequence number wraparounds */ -static ulong TftpBlockWrapOffset; /* memory offset due to wrapping */ +static ulong TftpBlock; /* packet sequence number */ +static ulong TftpLastBlock; /* last packet sequence number received */ +static ulong TftpBlockWrap; /* count of sequence number wraparounds */ +static ulong TftpBlockWrapOffset; /* memory offset due to wrapping */
These changes are indentation only changes, so they should be in a separate patch.
static int TftpState; #ifdef CONFIG_TFTP_TSIZE -static int TftpTsize; /* The file size reported by the server */ -static short TftpNumchars; /* The number of hashes we printed */ +static int TftpTsize; /* The file size reported by the server */ +static short TftpNumchars; /* The number of hashes we printed */
dito.
[...]
@@ -421,7 +421,7 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
/* * Acknoledge the block just received, which will prompt
* the server for the next one.
* the remote for the next one.
Hey, while you're at it, please fix the "Acknoledge" typo ;)
[...]
@@ -568,7 +568,7 @@ TftpStart (void) strncpy(tftp_filename, BootFile, MAX_LEN); tftp_filename[MAX_LEN-1] = 0; } else {
TftpServerIP = string_to_ip (BootFile);
TftpRemoteIP = string_to_ip(BootFile);
Whitespace fix.
Apart from that, patch looks simple enough, so
Acked-by: Detlev Zundel dzu@denx.de
Il 19/04/2011 16:18, Detlev Zundel ha scritto:
Hi Luca,
With the upcoming TFTP server implementation, the remote node can be either a client or a server, so avoid ambiguities.
Signed-off-by: Luca Ceresoliluca.ceresoli@comelit.it Cc: Wolfgang Denkwd@denx.de
Changes in v2:
- fixed checkpatch issues.
net/tftp.c | 42 +++++++++++++++++++++--------------------- 1 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c index 00abec3..da545c6 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -55,18 +55,18 @@ enum { TFTP_ERR_FILE_ALREADY_EXISTS = 6, };
-static IPaddr_t TftpServerIP; -static int TftpServerPort; /* The UDP port at their end */ -static int TftpOurPort; /* The UDP port at our end */ +static IPaddr_t TftpRemoteIP; +static int TftpRemotePort; /* The UDP port at their end */ +static int TftpOurPort; /* The UDP port at our end */ static int TftpTimeoutCount; -static ulong TftpBlock; /* packet sequence number */ -static ulong TftpLastBlock; /* last packet sequence number received */ -static ulong TftpBlockWrap; /* count of sequence number wraparounds */ -static ulong TftpBlockWrapOffset; /* memory offset due to wrapping */ +static ulong TftpBlock; /* packet sequence number */ +static ulong TftpLastBlock; /* last packet sequence number received */ +static ulong TftpBlockWrap; /* count of sequence number wraparounds */ +static ulong TftpBlockWrapOffset; /* memory offset due to wrapping */
These changes are indentation only changes, so they should be in a separate patch.
It's needed for checkpatch compliance.
static int TftpState; #ifdef CONFIG_TFTP_TSIZE -static int TftpTsize; /* The file size reported by the server */ -static short TftpNumchars; /* The number of hashes we printed */ +static int TftpTsize; /* The file size reported by the server */ +static short TftpNumchars; /* The number of hashes we printed */
dito.
[...]
@@ -421,7 +421,7 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
/* * Acknoledge the block just received, which will prompt
* the server for the next one.
* the remote for the next one.Hey, while you're at it, please fix the "Acknoledge" typo ;)
Will do.
[...]
@@ -568,7 +568,7 @@ TftpStart (void) strncpy(tftp_filename, BootFile, MAX_LEN); tftp_filename[MAX_LEN-1] = 0; } else {
TftpServerIP = string_to_ip (BootFile);
TftpRemoteIP = string_to_ip(BootFile);Whitespace fix.
checkpatch again.
Luca
Hi Luca,
[...]
Whitespace fix.
checkpatch again.
Hm, I see. Still, can we have one commit (with "cosmetic" in the changelog) that silences checkpatch but does not have any functional changes? We really try hard to separate cosmetic from functional changes. This makes reviewing (and debugging) so much easier.
This is the second time that I personally encounter that problem and I still don't see an automated way to take care of this. This simply is the result that checkpatch will flag only new errors when there are no _old_ ones. The reality of course looks different:
[dzu@pollux u-boot-testing (master)]$ ../linux/scripts/checkpatch.pl -f net/net.c
[...]
total: 76 errors, 193 warnings, 1934 lines checked
net/net.c has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.
If we strictly do require checkpatch cleanliness, maybe we should start cleaning up the code base as is first with only cosmetic changes. Do I see volunteers? ;)
Cheers Detlev
Hi,
just a few e-mails ago along this thread Albert Aribaud wrote:
My opinion is that you should make sure that at least the code you touch is checkpatch-clean, so yes, you should fix that; but there is no need to submit 'checkpatch-compliance' patches. Just fix the line here so that checkpatch does not complain.
So I proceeded along that way.
Now Detlev Zundel wrote:
... Hm, I see. Still, can we have one commit (with "cosmetic" in the changelog) that silences checkpatch but does not have any functional changes? We really try hard to separate cosmetic from functional changes. This makes reviewing (and debugging) so much easier.
While I appreciate the careful review of my patches, I cannot hide that it is discouraging for new contributors to be requested for contradictory modifications.
There should be one precise policy, and that should be clearly documented.
http://www.denx.de/wiki/U-Boot/CodingStyle is the place where I would expect to find it.
Luca
Hi Luca,
Hi,
just a few e-mails ago along this thread Albert Aribaud wrote:
My opinion is that you should make sure that at least the code you touch is checkpatch-clean, so yes, you should fix that; but there is no need to submit 'checkpatch-compliance' patches. Just fix the line here so that checkpatch does not complain.
So I proceeded along that way.
Now Detlev Zundel wrote:
... Hm, I see. Still, can we have one commit (with "cosmetic" in the changelog) that silences checkpatch but does not have any functional changes? We really try hard to separate cosmetic from functional changes. This makes reviewing (and debugging) so much easier.
While I appreciate the careful review of my patches, I cannot hide that it is discouraging for new contributors to be requested for contradictory modifications.
Sorry for that, but it is only that we start using checkpatch more aggressively, that such problems turn up which we did not yet agree on how to solve.
There should be one precise policy, and that should be clearly documented.
I fully agree.
http://www.denx.de/wiki/U-Boot/CodingStyle is the place where I would expect to find it.
I'll start a new thread to discuss this. Hopefully we then come up with a policy to stick into that wiki page.
Thanks for bearing with me Detlev
Detlev Zundel wrote:
I'll start a new thread to discuss this. Hopefully we then come up with a policy to stick into that wiki page.
Now that the checkpatch policy is much more clear, I started to clean up the networking stuff, on top of which the TFTP server sits.
net/net.c alone counts 76 errors and 197 warnings. In terms of modified lines, it's going to be a big job, so I am doing it in separate patches, one per each category of errors/warnings (whitespace issues, lines >80 chars, parentheses issues etc). Is this choice ok?
Also, it's going to be a bigger job than the TFTP server itself, so I'll send a separate patchset for the cleanup work, and hold the TFTP server until the cleanup is worked out.
Luca
Hi Luca,
Detlev Zundel wrote:
I'll start a new thread to discuss this. Hopefully we then come up with a policy to stick into that wiki page.
Now that the checkpatch policy is much more clear, I started to clean up the networking stuff, on top of which the TFTP server sits.
Thanks a lot for starting this cleanup. Your work is very much appreciated.
net/net.c alone counts 76 errors and 197 warnings. In terms of modified lines, it's going to be a big job, so I am doing it in separate patches, one per each category of errors/warnings (whitespace issues, lines >80 chars, parentheses issues etc). Is this choice ok?
This makes sense to me yes.
Also, it's going to be a bigger job than the TFTP server itself, so I'll send a separate patchset for the cleanup work, and hold the TFTP server until the cleanup is worked out.
If you really start the clenaup, then this order makes absolute sense.
Thanks in advance! Detlev
Hi Luca,
Il 19/04/2011 16:18, Detlev Zundel ha scritto:
Hi Luca,
With the upcoming TFTP server implementation, the remote node can be either a client or a server, so avoid ambiguities.
Signed-off-by: Luca Ceresoliluca.ceresoli@comelit.it Cc: Wolfgang Denkwd@denx.de
Changes in v2:
- fixed checkpatch issues.
net/tftp.c | 42 +++++++++++++++++++++--------------------- 1 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c index 00abec3..da545c6 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -55,18 +55,18 @@ enum { TFTP_ERR_FILE_ALREADY_EXISTS = 6, };
-static IPaddr_t TftpServerIP; -static int TftpServerPort; /* The UDP port at their end */ -static int TftpOurPort; /* The UDP port at our end */ +static IPaddr_t TftpRemoteIP; +static int TftpRemotePort; /* The UDP port at their end */ +static int TftpOurPort; /* The UDP port at our end */ static int TftpTimeoutCount; -static ulong TftpBlock; /* packet sequence number */ -static ulong TftpLastBlock; /* last packet sequence number received */ -static ulong TftpBlockWrap; /* count of sequence number wraparounds */ -static ulong TftpBlockWrapOffset; /* memory offset due to wrapping */ +static ulong TftpBlock; /* packet sequence number */ +static ulong TftpLastBlock; /* last packet sequence number received */ +static ulong TftpBlockWrap; /* count of sequence number wraparounds */ +static ulong TftpBlockWrapOffset; /* memory offset due to wrapping */
These changes are indentation only changes, so they should be in a separate patch.
It's needed for checkpatch compliance.
I'm trying to understand the problems involved, but looking at this again, it is not clear to me what you say here. When I run your version 1 of the patches (where you only do the rename) through checkpatch, I get:
WARNING: line over 80 characters #116: FILE: net/tftp.c:59: +static int TftpRemotePort; /* The UDP port at their end */
WARNING: consider using kstrto* in preference to simple_strtol #215: FILE: net/tftp.c:619: + TftpRemotePort = simple_strtol(ep, NULL, 10);
total: 0 errors, 2 warnings, 99 lines checked
/home/dzu/transfer/p2 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.
So I'm not sure why you say that the other changes are needed for checkpatch. What exactly do you mean by this?
Thanks Detlev
Detlev Zundel wrote:
Hi Luca,
Il 19/04/2011 16:18, Detlev Zundel ha scritto:
Hi Luca,
With the upcoming TFTP server implementation, the remote node can be either a client or a server, so avoid ambiguities.
Signed-off-by: Luca Ceresoliluca.ceresoli@comelit.it Cc: Wolfgang Denkwd@denx.de
Changes in v2:
- fixed checkpatch issues.
net/tftp.c | 42 +++++++++++++++++++++--------------------- 1 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c index 00abec3..da545c6 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -55,18 +55,18 @@ enum { TFTP_ERR_FILE_ALREADY_EXISTS = 6, };
-static IPaddr_t TftpServerIP; -static int TftpServerPort; /* The UDP port at their end */ -static int TftpOurPort; /* The UDP port at our end */ +static IPaddr_t TftpRemoteIP; +static int TftpRemotePort; /* The UDP port at their end */ +static int TftpOurPort; /* The UDP port at our end */ static int TftpTimeoutCount; -static ulong TftpBlock; /* packet sequence number */ -static ulong TftpLastBlock; /* last packet sequence number received */ -static ulong TftpBlockWrap; /* count of sequence number wraparounds */ -static ulong TftpBlockWrapOffset; /* memory offset due to wrapping */ +static ulong TftpBlock; /* packet sequence number */ +static ulong TftpLastBlock; /* last packet sequence number received */ +static ulong TftpBlockWrap; /* count of sequence number wraparounds */ +static ulong TftpBlockWrapOffset; /* memory offset due to wrapping */
These changes are indentation only changes, so they should be in a separate patch.
It's needed for checkpatch compliance.
I'm trying to understand the problems involved, but looking at this again, it is not clear to me what you say here. When I run your version 1 of the patches (where you only do the rename) through checkpatch, I get:
WARNING: line over 80 characters #116: FILE: net/tftp.c:59: +static int TftpRemotePort; /* The UDP port at their end */
WARNING: consider using kstrto* in preference to simple_strtol #215: FILE: net/tftp.c:619:
TftpRemotePort = simple_strtol(ep, NULL, 10);total: 0 errors, 2 warnings, 99 lines checked
/home/dzu/transfer/p2 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.
So I'm not sure why you say that the other changes are needed for checkpatch. What exactly do you mean by this?
All the comments were nicely columned before my patchset. Reducing the length of a line would have broken this.
I chose to change all of them in order to preserve the pre-existing coding style.
Luca
Hi Luca,
It's needed for checkpatch compliance.
I'm trying to understand the problems involved, but looking at this again, it is not clear to me what you say here. When I run your version 1 of the patches (where you only do the rename) through checkpatch, I get:
WARNING: line over 80 characters #116: FILE: net/tftp.c:59: +static int TftpRemotePort; /* The UDP port at their end */
WARNING: consider using kstrto* in preference to simple_strtol #215: FILE: net/tftp.c:619:
TftpRemotePort = simple_strtol(ep, NULL, 10);total: 0 errors, 2 warnings, 99 lines checked
/home/dzu/transfer/p2 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.
So I'm not sure why you say that the other changes are needed for checkpatch. What exactly do you mean by this?
All the comments were nicely columned before my patchset. Reducing the length of a line would have broken this.
I chose to change all of them in order to preserve the pre-existing coding style.
Ok, this makes sense, alas the wording "it's needed for checkpatch compliance" was somewhat misleading.
Ideally only the relevant changes should be in one commit and re-indentation to align everything again should be in a separate commit. As we saw that checkpatch also looks at context lines, this commit usually needs to be logically _before_ your own changes. Probably the easiest way to achieve this is to commit the changes separately and reorder them with git rebase -i.
I amended the wiki page[1] in the hope of getting more light into these things.
Cheers Detlev
[1] http://www.denx.de/wiki/U-Boot/Patches
Luca Ceresoli wrote:
Il 19/04/2011 16:18, Detlev Zundel ha scritto:
Hi Luca,
With the upcoming TFTP server implementation, the remote node can be either a client or a server, so avoid ambiguities.
Signed-off-by: Luca Ceresoliluca.ceresoli@comelit.it Cc: Wolfgang Denkwd@denx.de
Changes in v2:
- fixed checkpatch issues.
net/tftp.c | 42 +++++++++++++++++++++--------------------- 1 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c index 00abec3..da545c6 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -55,18 +55,18 @@ enum { TFTP_ERR_FILE_ALREADY_EXISTS = 6, };
-static IPaddr_t TftpServerIP; -static int TftpServerPort; /* The UDP port at their end */ -static int TftpOurPort; /* The UDP port at our end */ +static IPaddr_t TftpRemoteIP; +static int TftpRemotePort; /* The UDP port at their end */ +static int TftpOurPort; /* The UDP port at our end */ static int TftpTimeoutCount; -static ulong TftpBlock; /* packet sequence number */ -static ulong TftpLastBlock; /* last packet sequence number received */ -static ulong TftpBlockWrap; /* count of sequence number wraparounds */ -static ulong TftpBlockWrapOffset; /* memory offset due to wrapping */ +static ulong TftpBlock; /* packet sequence number */ +static ulong TftpLastBlock; /* last packet sequence number received */ +static ulong TftpBlockWrap; /* count of sequence number wraparounds */ +static ulong TftpBlockWrapOffset; /* memory offset due to wrapping */
These changes are indentation only changes, so they should be in a separate patch.
It's needed for checkpatch compliance.
static int TftpState; #ifdef CONFIG_TFTP_TSIZE -static int TftpTsize; /* The file size reported by the server */ -static short TftpNumchars; /* The number of hashes we printed */ +static int TftpTsize; /* The file size reported by the server */ +static short TftpNumchars; /* The number of hashes we printed */
dito.
[...]
@@ -421,7 +421,7 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
/* * Acknoledge the block just received, which will prompt
* the server for the next one.
* the remote for the next one.Hey, while you're at it, please fix the "Acknoledge" typo ;)
Will do.
Done for v3.
I removed the checkpatch-related changes: they are now on the tftp cleanup patch series that I submitted on saturday, and on top of which v3 of the TFTP server will be based.
Luca
Hi Luca,
[...]
I removed the checkpatch-related changes: they are now on the tftp cleanup patch series that I submitted on saturday, and on top of which v3 of the TFTP server will be based.
Thanks for your persistence - it is very much appreciated! Detlev
On Monday, April 18, 2011 12:19:51 Luca Ceresoli wrote:
With the upcoming TFTP server implementation, the remote node can be either a client or a server, so avoid ambiguities.
the summary made me worried because i thought you were going to rename the env var "server" to "remote". could you tweak the summary to avoid this ambiguity in what you're actually doing ? how about: TFTP: use "remote" in local variable names instead of "server"
IPaddr_t ServerNet = TftpServerIP & NetOurSubnetMask;
IPaddr_t ServerNet = TftpRemoteIP & NetOurSubnetMask;
shouldnt that now be RemoteNet ? -mike
Mike Frysinger wrote:
On Monday, April 18, 2011 12:19:51 Luca Ceresoli wrote:
With the upcoming TFTP server implementation, the remote node can be either a client or a server, so avoid ambiguities.
the summary made me worried because i thought you were going to rename the env var "server" to "remote". could you tweak the summary to avoid this ambiguity in what you're actually doing ? how about: TFTP: use "remote" in local variable names instead of "server"
Improved commit message in v3.
IPaddr_t ServerNet = TftpServerIP& NetOurSubnetMask;
IPaddr_t ServerNet = TftpRemoteIP& NetOurSubnetMask;shouldnt that now be RemoteNet ?
Done for v3.
Luca
Dear Luca Ceresoli,
In message 1303143594-5345-4-git-send-email-luca.ceresoli@comelit.it you wrote:
With the upcoming TFTP server implementation, the remote node can be either a client or a server, so avoid ambiguities.
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
Changes in v2:
- fixed checkpatch issues.
net/tftp.c | 42 +++++++++++++++++++++--------------------- 1 files changed, 21 insertions(+), 21 deletions(-)
How do we proceed now? I've applied paches 1 + 2 of this series, but for patch 3 we had changes requested, and the following patche sdepend on it.
I understand you are now waiting for the "net/net.c: cosmetic:" patches to go in? Normally these would be stuff for the next branch...
I'd even be willing to pull these in now, if you then would re-post the remaining patches of the tftpserver code soon?
Best regards,
Wolfgang Denk
Hi Wolfgang,
Wolfgang Denk wrote:
Dear Luca Ceresoli,
In message1303143594-5345-4-git-send-email-luca.ceresoli@comelit.it you wrote:
With the upcoming TFTP server implementation, the remote node can be either a client or a server, so avoid ambiguities.
Signed-off-by: Luca Ceresoliluca.ceresoli@comelit.it Cc: Wolfgang Denkwd@denx.de
Changes in v2:
- fixed checkpatch issues.
net/tftp.c | 42 +++++++++++++++++++++--------------------- 1 files changed, 21 insertions(+), 21 deletions(-)
How do we proceed now? I've applied paches 1 + 2 of this series, but for patch 3 we had changes requested, and the following patche sdepend on it.
I understand you are now waiting for the "net/net.c: cosmetic:" patches to go in? Normally these would be stuff for the next branch...
I'd even be willing to pull these in now, if you then would re-post the remaining patches of the tftpserver code soon?
I see you've committed the net/net.c cleanup into master. I'll rebase the TFTP server work (only patches 3 and 4) and resend them as soon as possible for inclusion in master, perhaps today or tomorrow.
Luca
Luca Ceresoli wrote:
Hi Wolfgang,
Wolfgang Denk wrote:
Dear Luca Ceresoli,
In message1303143594-5345-4-git-send-email-luca.ceresoli@comelit.it you wrote:
With the upcoming TFTP server implementation, the remote node can be either a client or a server, so avoid ambiguities.
Signed-off-by: Luca Ceresoliluca.ceresoli@comelit.it Cc: Wolfgang Denkwd@denx.de
Changes in v2:
- fixed checkpatch issues.
net/tftp.c | 42 +++++++++++++++++++++--------------------- 1 files changed, 21 insertions(+), 21 deletions(-)
How do we proceed now? I've applied paches 1 + 2 of this series, but for patch 3 we had changes requested, and the following patche sdepend on it.
I understand you are now waiting for the "net/net.c: cosmetic:" patches to go in? Normally these would be stuff for the next branch...
I'd even be willing to pull these in now, if you then would re-post the remaining patches of the tftpserver code soon?
I see you've committed the net/net.c cleanup into master. I'll rebase the TFTP server work (only patches 3 and 4) and resend them as soon as possible for inclusion in master, perhaps today or tomorrow.
Damn. While rebasing, I hit many checkpatch issues. The cleanup soon became larger that the TFTP server work, so I took a breath, and started a new branch out of master to fully cleanup net/tftp.c.
I submitted right now a lengthy patch series just for that cleanup. Since it's very similar to my net/net.c cleanup that has just been merged, I think this will be merged also without a great need of discussion.
I'll take some time ASAP to rebase the TFTP server on top of this cleanup, but alas not today nor tomorrow (I spent all the time with checkpatch...).
Luca
With the upcoming TFTP server implementation, requests can be either outgoing or incoming, so avoid ambiguities.
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de --- Changes in v2: none.
net/tftp.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c index da545c6..e166a63 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -69,7 +69,7 @@ static int TftpTsize; /* The file size reported by the server */ static short TftpNumchars; /* The number of hashes we printed */ #endif
-#define STATE_RRQ 1 +#define STATE_SEND_RRQ 1 #define STATE_DATA 2 #define STATE_TOO_LARGE 3 #define STATE_BAD_MAGIC 4 @@ -200,7 +200,7 @@ TftpSend (void)
switch (TftpState) {
- case STATE_RRQ: + case STATE_SEND_RRQ: xp = pkt; s = (ushort *)pkt; *s++ = htons(TFTP_RRQ); @@ -293,7 +293,7 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, #endif return; } - if (TftpState != STATE_RRQ && src != TftpRemotePort) + if (TftpState != STATE_SEND_RRQ && src != TftpRemotePort) return;
if (len < 2) { @@ -380,10 +380,10 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, } }
- if (TftpState == STATE_RRQ) + if (TftpState == STATE_SEND_RRQ) debug("Server did not acknowledge timeout option!\n");
- if (TftpState == STATE_RRQ || TftpState == STATE_OACK) { + if (TftpState == STATE_SEND_RRQ || TftpState == STATE_OACK) { /* first block received */ TftpState = STATE_DATA; TftpRemotePort = src; @@ -610,7 +610,7 @@ TftpStart (void)
TftpRemotePort = WELL_KNOWN_PORT; TftpTimeoutCount = 0; - TftpState = STATE_RRQ; + TftpState = STATE_SEND_RRQ; /* Use a pseudo-random port unless a specific port is set */ TftpOurPort = 1024 + (get_timer(0) % 3072);
Hi Luca,
With the upcoming TFTP server implementation, requests can be either outgoing or incoming, so avoid ambiguities.
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
Changes in v2: none.
Acked-by: Detlev Zundel dzu@denx.de
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de --- Changes in v2: none.
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@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;
if (len < 2) { @@ -307,12 +318,24 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, switch (ntohs(proto)) {
case TFTP_RRQ: - case TFTP_WRQ: case TFTP_ACK: break; default: break;
+#ifdef CONFIG_CMD_TFTPSRV + case TFTP_WRQ: + debug("Got WRQ\n"); + TftpRemoteIP = sip; + TftpRemotePort = src; + TftpOurPort = 1024 + (get_timer(0) % 3072); + TftpLastBlock = 0; + TftpBlockWrap = 0; + TftpBlockWrapOffset = 0; + TftpSend(); /* Send ACK(0) */ + break; +#endif + case TFTP_OACK: debug("Got OACK: %s %s\n", pkt, @@ -383,7 +406,11 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, if (TftpState == STATE_SEND_RRQ) debug("Server did not acknowledge timeout option!\n");
- if (TftpState == STATE_SEND_RRQ || TftpState == STATE_OACK) { + if (TftpState == STATE_SEND_RRQ || +#ifdef CONFIG_CMD_TFTPSRV + TftpState == STATE_RECV_WRQ || +#endif + TftpState == STATE_OACK) { /* first block received */ TftpState = STATE_DATA; TftpRemotePort = src; @@ -518,7 +545,10 @@ TftpTimeout (void) } else { puts ("T "); NetSetTimeout (TftpTimeoutMSecs, TftpTimeout); - TftpSend (); +#ifdef CONFIG_CMD_TFTPSRV + if (TftpState != STATE_RECV_WRQ) +#endif + TftpSend(); } }
@@ -639,6 +669,40 @@ TftpStart (void) TftpSend (); }
+#ifdef CONFIG_CMD_TFTPSRV +void +TftpStartServer(void) +{ + tftp_filename[0] = 0; + +#if defined(CONFIG_NET_MULTI) + printf("Using %s device\n", eth_get_name()); +#endif + printf("Listening for TFTP transfer on %pI4\n", &NetOurIP); + printf("Load address: 0x%lx\n", load_addr); + + puts("Loading: *\b"); + + TftpTimeoutCountMax = TIMEOUT_COUNT; + TftpTimeoutCount = 0; + TftpTimeoutMSecs = TIMEOUT; + NetSetTimeout(TftpTimeoutMSecs, TftpTimeout); + + /* Revert TftpBlkSize to dflt */ + TftpBlkSize = TFTP_BLOCK_SIZE; + TftpBlock = 0; + TftpOurPort = WELL_KNOWN_PORT; + +#ifdef CONFIG_TFTP_TSIZE + TftpTsize = 0; + TftpNumchars = 0; +#endif + + TftpState = STATE_RECV_WRQ; + NetSetHandler(TftpHandler); +} +#endif /* CONFIG_CMD_TFTPSRV */ + #ifdef CONFIG_MCAST_TFTP /* Credits: atftp project. */ diff --git a/net/tftp.h b/net/tftp.h index e3dfb26..3abdf7b 100644 --- a/net/tftp.h +++ b/net/tftp.h @@ -2,6 +2,8 @@ * LiMon - BOOTP/TFTP. * * Copyright 1994, 1995, 2000 Neil Russell. + * Copyright 2011 Comelit Group SpA + * Luca Ceresoli luca.ceresoli@comelit.it * (See License) */
@@ -16,6 +18,10 @@ /* tftp.c */ extern void TftpStart (void); /* Begin TFTP get */
+#ifdef CONFIG_CMD_TFTPSRV +extern void TftpStartServer(void); /* Wait for incoming TFTP put */ +#endif + /**********************************************************************/
#endif /* __TFTP_H__ */
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@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)
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?
Cheers Detlev
Detlev Zundel wrote:
Hi Luca,
Signed-off-by: Luca Ceresoliluca.ceresoli@comelit.it Cc: Wolfgang Denkwd@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@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)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
Hi Luca,
[...]
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.
Ok, thanks for the number.
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.
:) Another instance of the difficulty to predict modern compilers.
Cheers Detlev
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de --- Changes in v2: none.
README | 1 + common/cmd_net.c | 14 ++++++++++++++ include/net.h | 3 ++- net/net.c | 10 ++++++++-- 4 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/README b/README index b9b0fcb..83dcb2a 100644 --- a/README +++ b/README @@ -685,6 +685,7 @@ The following options need to be configured: (requires CONFIG_CMD_MEMORY) CONFIG_CMD_SOURCE "source" command Support CONFIG_CMD_SPI * SPI serial bus support + CONFIG_CMD_TFTPSRV * TFTP transfer in server mode CONFIG_CMD_USB * USB support CONFIG_CMD_VFD * VFD support (TRAB) CONFIG_CMD_CDP * Cisco Discover Protocol support diff --git a/common/cmd_net.c b/common/cmd_net.c index 8c6f5c8..053162a 100644 --- a/common/cmd_net.c +++ b/common/cmd_net.c @@ -52,6 +52,20 @@ U_BOOT_CMD( "[loadAddress] [[hostIPaddr:]bootfilename]" );
+#ifdef CONFIG_CMD_TFTPSRV +int do_tftpsrv(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{ + return netboot_common(TFTPSRV, cmdtp, argc, argv); +} + +U_BOOT_CMD( + tftpsrv, 2, 1, do_tftpsrv, + "boot image via network acting as a TFTP server", + "[loadAddress]" +); +#endif + + #ifdef CONFIG_CMD_RARP int do_rarpb (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { diff --git a/include/net.h b/include/net.h index 01f7159..018a744 100644 --- a/include/net.h +++ b/include/net.h @@ -364,7 +364,8 @@ extern int NetState; /* Network loop state */ extern int NetRestartWrap; /* Tried all network devices */ #endif
-typedef enum { BOOTP, RARP, ARP, TFTP, DHCP, PING, DNS, NFS, CDP, NETCONS, SNTP } proto_t; +typedef enum { BOOTP, RARP, ARP, TFTP, DHCP, PING, DNS, NFS, CDP, NETCONS, SNTP, + TFTPSRV } proto_t;
/* from net/net.c */ extern char BootFile[128]; /* Boot File name */ diff --git a/net/net.c b/net/net.c index 132f99b..17eb06f 100644 --- a/net/net.c +++ b/net/net.c @@ -388,7 +388,11 @@ restart: /* always use ARP to get server ethernet address */ TftpStart(); break; - +#ifdef CONFIG_CMD_TFTPSRV + case TFTPSRV: + TftpStartServer(); + break; +#endif #if defined(CONFIG_CMD_DHCP) case DHCP: BootpTry = 0; @@ -1731,7 +1735,9 @@ static int net_check_prereq (proto_t protocol) #if defined(CONFIG_CMD_PING) || defined(CONFIG_CMD_SNTP) common: #endif - +#ifdef CONFIG_CMD_TFTPSRV + case TFTPSRV: +#endif if (NetOurIP == 0) { puts ("*** ERROR: `ipaddr' not set\n"); return (1);
Hi Luca,
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
Changes in v2: none.
Acked-by: Detlev Zundel dzu@denx.de
On Monday, April 18, 2011 12:19:54 Luca Ceresoli wrote:
+#ifdef CONFIG_CMD_TFTPSRV +int do_tftpsrv(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
should be static
+U_BOOT_CMD(
- tftpsrv, 2, 1, do_tftpsrv,
- "boot image via network acting as a TFTP server",
- "[loadAddress]"
+);
the desc is a bit confusing. how about: act as a TFTP server and boot the first received file -mike
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de --- README | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/README b/README index 4917e26..b9b0fcb 100644 --- a/README +++ b/README @@ -1299,7 +1299,6 @@ The following options need to be configured: driver in use must provide a function: mcast() to join/leave a multicast group.
- CONFIG_BOOTP_RANDOM_DELAY - BOOTP Recovery Mode: CONFIG_BOOTP_RANDOM_DELAY
Dear Luca Ceresoli,
In message 1302796377-3321-2-git-send-email-luca.ceresoli@comelit.it you wrote:
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
README | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk
This is needed for the upcoming TFTP server implementation.
This also simplifies PingHandler() and fixes rxhand_f documentation.
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de --- drivers/net/netconsole.c | 5 +++-- include/net.h | 15 ++++++++++----- net/bootp.c | 9 ++++++--- net/dns.c | 2 +- net/net.c | 25 ++++++++++++++----------- net/nfs.c | 2 +- net/rarp.c | 3 ++- net/sntp.c | 3 ++- net/tftp.c | 3 ++- 9 files changed, 41 insertions(+), 26 deletions(-)
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index e27bb3e..ed753d1 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -40,13 +40,14 @@ static short nc_port; /* source/target port */ static const char *output_packet; /* used by first send udp */ static int output_packet_len = 0;
-static void nc_wait_arp_handler (uchar * pkt, unsigned dest, unsigned src, +static void nc_wait_arp_handler (uchar * pkt, unsigned dest, + IPaddr_t sip, unsigned src, unsigned len) { NetState = NETLOOP_SUCCESS; /* got arp reply - quit net loop */ }
-static void nc_handler (uchar * pkt, unsigned dest, unsigned src, +static void nc_handler (uchar * pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len) { if (input_size) diff --git a/include/net.h b/include/net.h index 95ef8ab..01f7159 100644 --- a/include/net.h +++ b/include/net.h @@ -72,12 +72,17 @@ typedef ulong IPaddr_t;
-/* - * The current receive packet handler. Called with a pointer to the - * application packet, and a protocol type (PORT_BOOTPC or PORT_TFTP). - * All other packets are dealt with without calling the handler. +/** + * An incoming packet handler. + * @param pkt pointer to the application packet + * @param dport destination UDP port + * @param sip source IP address + * @param sport source UDP port + * @param len packet length */ -typedef void rxhand_f(uchar *, unsigned, unsigned, unsigned); +typedef void rxhand_f(uchar *pkt, unsigned dport, + IPaddr_t sip, unsigned sport, + unsigned len);
/* * A timeout handler. Called after time interval has expired. diff --git a/net/bootp.c b/net/bootp.c index 87b027e..eafaae2 100644 --- a/net/bootp.c +++ b/net/bootp.c @@ -44,7 +44,8 @@ ulong seed1, seed2; dhcp_state_t dhcp_state = INIT; unsigned long dhcp_leasetime = 0; IPaddr_t NetDHCPServerIP = 0; -static void DhcpHandler(uchar * pkt, unsigned dest, unsigned src, unsigned len); +static void DhcpHandler(uchar * pkt, unsigned dest, IPaddr_t sip, unsigned src, + unsigned len);
/* For Debug */ #if 0 @@ -282,7 +283,8 @@ static void BootpVendorProcess (u8 * ext, int size) * Handle a BOOTP received packet. */ static void -BootpHandler(uchar * pkt, unsigned dest, unsigned src, unsigned len) +BootpHandler(uchar * pkt, unsigned dest, IPaddr_t sip, unsigned src, + unsigned len) { Bootp_t *bp; char *s; @@ -858,7 +860,8 @@ static void DhcpSendRequestPkt(Bootp_t *bp_offer) * Handle DHCP received packets. */ static void -DhcpHandler(uchar * pkt, unsigned dest, unsigned src, unsigned len) +DhcpHandler(uchar * pkt, unsigned dest, IPaddr_t sip, unsigned src, + unsigned len) { Bootp_t *bp = (Bootp_t *)pkt;
diff --git a/net/dns.c b/net/dns.c index bb3e3f5..b51d1bd 100644 --- a/net/dns.c +++ b/net/dns.c @@ -101,7 +101,7 @@ DnsTimeout(void) }
static void -DnsHandler(uchar *pkt, unsigned dest, unsigned src, unsigned len) +DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len) { struct header *header; const unsigned char *p, *e, *s; diff --git a/net/net.c b/net/net.c index a609632..79afd8b 100644 --- a/net/net.c +++ b/net/net.c @@ -555,7 +555,8 @@ startAgainTimeout(void) }
static void -startAgainHandler(uchar * pkt, unsigned dest, unsigned src, unsigned len) +startAgainHandler(uchar * pkt, unsigned dest, IPaddr_t sip, + unsigned src, unsigned len) { /* Totally ignore the packet */ } @@ -752,13 +753,10 @@ PingTimeout (void) }
static void -PingHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len) +PingHandler (uchar * pkt, unsigned dest, IPaddr_t sip, unsigned src, + unsigned len) { - IPaddr_t tmp; - volatile IP_t *ip = (volatile IP_t *)pkt; - - tmp = NetReadIP((void *)&ip->ip_src); - if (tmp != NetPingIP) + if (sip != NetPingIP) return;
NetState = NETLOOP_SUCCESS; @@ -990,7 +988,8 @@ CDPTimeout (void) }
static void -CDPDummyHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len) +CDPDummyHandler (uchar * pkt, unsigned dest, IPaddr_t sip, unsigned src, + unsigned len) { /* nothing */ } @@ -1304,6 +1303,7 @@ NetReceive(volatile uchar * inpkt, int len) IP_t *ip; ARP_t *arp; IPaddr_t tmp; + IPaddr_t src_ip; int x; uchar *pkt; #if defined(CONFIG_CMD_CDP) @@ -1477,7 +1477,7 @@ NetReceive(volatile uchar * inpkt, int len) memcpy(NetArpWaitPacketMAC, &arp->ar_data[0], 6);
#ifdef CONFIG_NETCONSOLE - (*packetHandler)(0,0,0,0); + (*packetHandler)(0,0,0,0,0); #endif /* modify header, and transmit it */ memcpy(((Ethernet_t *)NetArpWaitTxPacket)->et_dest, NetArpWaitPacketMAC, 6); @@ -1517,7 +1517,7 @@ NetReceive(volatile uchar * inpkt, int len) NetCopyIP(&NetServerIP, &arp->ar_data[ 6]); memcpy (NetServerEther, &arp->ar_data[ 0], 6);
- (*packetHandler)(0,0,0,0); + (*packetHandler)(0,0,0,0,0); } break; #endif @@ -1557,6 +1557,8 @@ NetReceive(volatile uchar * inpkt, int len) #endif return; } + /* Read source IP address for later use */ + src_ip = NetReadIP(&ip->ip_src); /* * The function returns the unchanged packet if it's not * a fragment, and either the complete packet or NULL if @@ -1596,7 +1598,7 @@ NetReceive(volatile uchar * inpkt, int len) * IP header OK. Pass the packet to the current handler. */ /* XXX point to ip packet */ - (*packetHandler)((uchar *)ip, 0, 0, 0); + (*packetHandler)((uchar *)ip, 0, src_ip, 0, 0); return; case ICMP_ECHO_REQUEST: debug("Got ICMP ECHO REQUEST, return %d bytes \n", @@ -1678,6 +1680,7 @@ NetReceive(volatile uchar * inpkt, int len) */ (*packetHandler)((uchar *)ip +IP_HDR_SIZE, ntohs(ip->udp_dst), + src_ip, ntohs(ip->udp_src), ntohs(ip->udp_len) - 8); break; diff --git a/net/nfs.c b/net/nfs.c index d11bb4c..23e43e1 100644 --- a/net/nfs.c +++ b/net/nfs.c @@ -580,7 +580,7 @@ NfsTimeout (void) }
static void -NfsHandler (uchar *pkt, unsigned dest, unsigned src, unsigned len) +NfsHandler (uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len) { int rlen;
diff --git a/net/rarp.c b/net/rarp.c index 9444c03..d1c83e0 100644 --- a/net/rarp.c +++ b/net/rarp.c @@ -43,7 +43,8 @@ int RarpTry; * Handle a RARP received packet. */ static void -RarpHandler(uchar * dummi0, unsigned dummi1, unsigned dummi2, unsigned dummi3) +RarpHandler(uchar * dummi0, unsigned dummi1, IPaddr_t sip, unsigned dummi2, + unsigned dummi3) { char *s; debug("Got good RARP\n"); diff --git a/net/sntp.c b/net/sntp.c index 76c10ec..7342cfd 100644 --- a/net/sntp.c +++ b/net/sntp.c @@ -48,7 +48,8 @@ SntpTimeout (void) }
static void -SntpHandler (uchar *pkt, unsigned dest, unsigned src, unsigned len) +SntpHandler (uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, + unsigned len) { struct sntp_pkt_t *rpktp = (struct sntp_pkt_t *)pkt; struct rtc_time tm; diff --git a/net/tftp.c b/net/tftp.c index ed559b7..8e6df0a 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -278,7 +278,8 @@ TftpSend (void)
static void -TftpHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len) +TftpHandler (uchar * pkt, unsigned dest, IPaddr_t sip, unsigned src, + unsigned len) { ushort proto; ushort *s;
Dear Luca Ceresoli,
In message 1302796377-3321-3-git-send-email-luca.ceresoli@comelit.it you wrote:
This is needed for the upcoming TFTP server implementation.
This also simplifies PingHandler() and fixes rxhand_f documentation.
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
drivers/net/netconsole.c | 5 +++-- include/net.h | 15 ++++++++++----- net/bootp.c | 9 ++++++--- net/dns.c | 2 +- net/net.c | 25 ++++++++++++++----------- net/nfs.c | 2 +- net/rarp.c | 3 ++- net/sntp.c | 3 ++- net/tftp.c | 3 ++- 9 files changed, 41 insertions(+), 26 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk
Dear Luca Ceresoli,
In message 1302796377-3321-3-git-send-email-luca.ceresoli@comelit.it you wrote:
This is needed for the upcoming TFTP server implementation.
This also simplifies PingHandler() and fixes rxhand_f documentation.
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de
Undone this and previous one as there is a V2 version of this patch.
I wish you would thread yout postings!
Best regards,
Wolfgang Denk
Hi Wolfgang,
Wolfgang Denk wrote:
Dear Luca Ceresoli,
In message1302796377-3321-3-git-send-email-luca.ceresoli@comelit.it you wrote:
This is needed for the upcoming TFTP server implementation.
This also simplifies PingHandler() and fixes rxhand_f documentation.
Signed-off-by: Luca Ceresoliluca.ceresoli@comelit.it Cc: Wolfgang Denkwd@denx.de
Undone this and previous one as there is a V2 version of this patch.
I wish you would thread yout postings!
I'm sorry for your wasted time, but it's not clear (to me at least...) how postings should be threaded in the U-Boot ml.
The Wiki says:
Make sure that your mailer adds or keeps correct |"In-reply-to:"| and|"References:"| headers, so threading of messages is working and everybody can see that the new message refers to some older posting of the same topic.
Uncommented and un-threaded repostings are extremely annoying and time-consuming, as we have to try to remember if anything similar has been posted before, look up the old threads, and then manually compare if anything has been changed, or what.
'correct|"In-reply-to:"| and|"References:"|' can have many interpretations.
On the ML I see varying ones, and I even see submissions with NO THREADING AT ALL: the first submit is in a thread, v2 starts a new thread, v3 yet another one etc.
MY humble interpretation of the Wiki wording is this:
- [PATCH 0/2] | +-> [PATCH 1/2] +-> [PATCH 2/2] | +-> Re: [PATCH 2/2] reviewers comments to patch 2/2 | +-> Re: [PATCH 2/2] more reviewers comments to patch 2/2 | +-> Re: [PATCH 0/2] reviewers comments to the patchset (0/2) | +-> Re: [PATCH 0/2] discussion... | +-> [PATCH v2 0/2] resubmit (In-Reply-To: the first submit) | +-> [PATCH v2 1/2] +-> [PATCH v2 2/2] | ... more discussion - not shown... | +-> [PATCH v3 0/2] resubmit (In-Reply-To: the first submit) | +-> [PATCH v3 1/2] +-> [PATCH v3 2/2]
What's wrong with this? Please state in absolutely unambiguous detail what the correct threading is, and please docuemnt it in the wiki for everybody.
My postings look correctly threaded according to the above interpretation, as you can see for example on the Gmane threaded interface.
Luca
With the upcoming TFTP server implementation, the remote node can be either a client or a server, so avoid ambiguities.
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de --- net/tftp.c | 28 ++++++++++++++-------------- 1 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c index 8e6df0a..42469e7 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -55,8 +55,8 @@ enum { TFTP_ERR_FILE_ALREADY_EXISTS = 6, };
-static IPaddr_t TftpServerIP; -static int TftpServerPort; /* The UDP port at their end */ +static IPaddr_t TftpRemoteIP; +static int TftpRemotePort; /* The UDP port at their end */ static int TftpOurPort; /* The UDP port at our end */ static int TftpTimeoutCount; static ulong TftpBlock; /* packet sequence number */ @@ -273,7 +273,8 @@ TftpSend (void) break; }
- NetSendUDPPacket(NetServerEther, TftpServerIP, TftpServerPort, TftpOurPort, len); + NetSendUDPPacket(NetServerEther, TftpRemoteIP, TftpRemotePort, + TftpOurPort, len); }
@@ -292,9 +293,8 @@ TftpHandler (uchar * pkt, unsigned dest, IPaddr_t sip, unsigned src, #endif return; } - if (TftpState != STATE_RRQ && src != TftpServerPort) { + if (TftpState != STATE_RRQ && src != TftpRemotePort) return; - }
if (len < 2) { return; @@ -318,7 +318,7 @@ TftpHandler (uchar * pkt, unsigned dest, IPaddr_t sip, unsigned src, pkt, pkt + strlen((char *)pkt) + 1); TftpState = STATE_OACK; - TftpServerPort = src; + TftpRemotePort = src; /* * Check for 'blksize' option. * Careful: "i" is signed, "len" is unsigned, thus @@ -386,7 +386,7 @@ TftpHandler (uchar * pkt, unsigned dest, IPaddr_t sip, unsigned src, if (TftpState == STATE_RRQ || TftpState == STATE_OACK) { /* first block received */ TftpState = STATE_DATA; - TftpServerPort = src; + TftpRemotePort = src; TftpLastBlock = 0; TftpBlockWrap = 0; TftpBlockWrapOffset = 0; @@ -421,7 +421,7 @@ TftpHandler (uchar * pkt, unsigned dest, IPaddr_t sip, unsigned src,
/* * Acknoledge the block just received, which will prompt - * the server for the next one. + * the remote for the next one. */ #ifdef CONFIG_MCAST_TFTP /* if I am the MasterClient, actively calculate what my next @@ -548,7 +548,7 @@ TftpStart (void) debug("TFTP blocksize = %i, timeout = %ld ms\n", TftpBlkSizeOption, TftpTimeoutMSecs);
- TftpServerIP = NetServerIP; + TftpRemoteIP = NetServerIP; if (BootFile[0] == '\0') { sprintf(default_filename, "%02lX%02lX%02lX%02lX.img", NetOurIP & 0xFF, @@ -568,7 +568,7 @@ TftpStart (void) strncpy(tftp_filename, BootFile, MAX_LEN); tftp_filename[MAX_LEN-1] = 0; } else { - TftpServerIP = string_to_ip (BootFile); + TftpRemoteIP = string_to_ip(BootFile); strncpy(tftp_filename, p + 1, MAX_LEN); tftp_filename[MAX_LEN-1] = 0; } @@ -578,12 +578,12 @@ TftpStart (void) printf ("Using %s device\n", eth_get_name()); #endif printf("TFTP from server %pI4" - "; our IP address is %pI4", &TftpServerIP, &NetOurIP); + "; our IP address is %pI4", &TftpRemoteIP, &NetOurIP);
/* Check if we need to send across this subnet */ if (NetOurGatewayIP && NetOurSubnetMask) { IPaddr_t OurNet = NetOurIP & NetOurSubnetMask; - IPaddr_t ServerNet = TftpServerIP & NetOurSubnetMask; + IPaddr_t ServerNet = TftpRemoteIP & NetOurSubnetMask;
if (OurNet != ServerNet) printf("; sending through gateway %pI4", &NetOurGatewayIP); @@ -608,7 +608,7 @@ TftpStart (void) NetSetTimeout (TftpTimeoutMSecs, TftpTimeout); NetSetHandler (TftpHandler);
- TftpServerPort = WELL_KNOWN_PORT; + TftpRemotePort = WELL_KNOWN_PORT; TftpTimeoutCount = 0; TftpState = STATE_RRQ; /* Use a pseudo-random port unless a specific port is set */ @@ -616,7 +616,7 @@ TftpStart (void)
#ifdef CONFIG_TFTP_PORT if ((ep = getenv("tftpdstp")) != NULL) { - TftpServerPort = simple_strtol(ep, NULL, 10); + TftpRemotePort = simple_strtol(ep, NULL, 10); } if ((ep = getenv("tftpsrcp")) != NULL) { TftpOurPort= simple_strtol(ep, NULL, 10);
With the upcoming TFTP server implementation, requests can be either outgoing or incoming, so avoid ambiguities.
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de --- net/tftp.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c index 42469e7..2c96358 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -69,7 +69,7 @@ static int TftpTsize; /* The file size reported by the server */ static short TftpNumchars; /* The number of hashes we printed */ #endif
-#define STATE_RRQ 1 +#define STATE_SEND_RRQ 1 #define STATE_DATA 2 #define STATE_TOO_LARGE 3 #define STATE_BAD_MAGIC 4 @@ -200,7 +200,7 @@ TftpSend (void)
switch (TftpState) {
- case STATE_RRQ: + case STATE_SEND_RRQ: xp = pkt; s = (ushort *)pkt; *s++ = htons(TFTP_RRQ); @@ -293,7 +293,7 @@ TftpHandler (uchar * pkt, unsigned dest, IPaddr_t sip, unsigned src, #endif return; } - if (TftpState != STATE_RRQ && src != TftpRemotePort) + if (TftpState != STATE_SEND_RRQ && src != TftpRemotePort) return;
if (len < 2) { @@ -380,10 +380,10 @@ TftpHandler (uchar * pkt, unsigned dest, IPaddr_t sip, unsigned src, } }
- if (TftpState == STATE_RRQ) + if (TftpState == STATE_SEND_RRQ) debug("Server did not acknowledge timeout option!\n");
- if (TftpState == STATE_RRQ || TftpState == STATE_OACK) { + if (TftpState == STATE_SEND_RRQ || TftpState == STATE_OACK) { /* first block received */ TftpState = STATE_DATA; TftpRemotePort = src; @@ -610,7 +610,7 @@ TftpStart (void)
TftpRemotePort = WELL_KNOWN_PORT; TftpTimeoutCount = 0; - TftpState = STATE_RRQ; + TftpState = STATE_SEND_RRQ; /* Use a pseudo-random port unless a specific port is set */ TftpOurPort = 1024 + (get_timer(0) % 3072);
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de ---
A note on the style of this patch. There are many hunks like this:
- if (TftpState != STATE_SEND_RRQ && src != TftpRemotePort)
- if (TftpState != STATE_SEND_RRQ &&
+#ifdef CONFIG_CMD_TFTPSRV
TftpState != STATE_RECV_WRQ &&+#endif
src != TftpRemotePort)
where I put a comparison between TftpState and STATE_RECV_WRQ in #ifdefs.
The #ifdef is not necessary, as both TftpState and STATE_RECV_WRQ are defined even when CONFIG_CMD_TFTPSRV is off. Simply, TftpState can never be equal to STATE_RECV_WRQ.
I've put the #ifdefs with the intention of optimizing the code as much as possible, avoiding a comparison that would be always true (or always false). I understand that this makes the code a lot more difficult to read.
Should I instead remote the #ifdefs and make the code more readable (but less optimized)?
Thanks, Luca
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 2c96358..c586a9f 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@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;
if (len < 2) { @@ -307,12 +318,24 @@ TftpHandler (uchar * pkt, unsigned dest, IPaddr_t sip, unsigned src, switch (ntohs(proto)) {
case TFTP_RRQ: - case TFTP_WRQ: case TFTP_ACK: break; default: break;
+#ifdef CONFIG_CMD_TFTPSRV + case TFTP_WRQ: + debug("Got WRQ\n"); + TftpRemoteIP = sip; + TftpRemotePort = src; + TftpOurPort = 1024 + (get_timer(0) % 3072); + TftpLastBlock = 0; + TftpBlockWrap = 0; + TftpBlockWrapOffset = 0; + TftpSend(); /* Send ACK(0) */ + break; +#endif + case TFTP_OACK: debug("Got OACK: %s %s\n", pkt, @@ -383,7 +406,11 @@ TftpHandler (uchar * pkt, unsigned dest, IPaddr_t sip, unsigned src, if (TftpState == STATE_SEND_RRQ) debug("Server did not acknowledge timeout option!\n");
- if (TftpState == STATE_SEND_RRQ || TftpState == STATE_OACK) { + if (TftpState == STATE_SEND_RRQ || +#ifdef CONFIG_CMD_TFTPSRV + TftpState == STATE_RECV_WRQ || +#endif + TftpState == STATE_OACK) { /* first block received */ TftpState = STATE_DATA; TftpRemotePort = src; @@ -518,7 +545,10 @@ TftpTimeout (void) } else { puts ("T "); NetSetTimeout (TftpTimeoutMSecs, TftpTimeout); - TftpSend (); +#ifdef CONFIG_CMD_TFTPSRV + if (TftpState != STATE_RECV_WRQ) +#endif + TftpSend(); } }
@@ -639,6 +669,40 @@ TftpStart (void) TftpSend (); }
+#ifdef CONFIG_CMD_TFTPSRV +void +TftpStartServer(void) +{ + tftp_filename[0] = 0; + +#if defined(CONFIG_NET_MULTI) + printf("Using %s device\n", eth_get_name()); +#endif + printf("Listening for TFTP transfer on %pI4\n", &NetOurIP); + printf("Load address: 0x%lx\n", load_addr); + + puts("Loading: *\b"); + + TftpTimeoutCountMax = TIMEOUT_COUNT; + TftpTimeoutCount = 0; + TftpTimeoutMSecs = TIMEOUT; + NetSetTimeout(TftpTimeoutMSecs, TftpTimeout); + + /* Revert TftpBlkSize to dflt */ + TftpBlkSize = TFTP_BLOCK_SIZE; + TftpBlock = 0; + TftpOurPort = WELL_KNOWN_PORT; + +#ifdef CONFIG_TFTP_TSIZE + TftpTsize = 0; + TftpNumchars = 0; +#endif + + TftpState = STATE_RECV_WRQ; + NetSetHandler(TftpHandler); +} +#endif /* CONFIG_CMD_TFTPSRV */ + #ifdef CONFIG_MCAST_TFTP /* Credits: atftp project. */ diff --git a/net/tftp.h b/net/tftp.h index e3dfb26..3abdf7b 100644 --- a/net/tftp.h +++ b/net/tftp.h @@ -2,6 +2,8 @@ * LiMon - BOOTP/TFTP. * * Copyright 1994, 1995, 2000 Neil Russell. + * Copyright 2011 Comelit Group SpA + * Luca Ceresoli luca.ceresoli@comelit.it * (See License) */
@@ -16,6 +18,10 @@ /* tftp.c */ extern void TftpStart (void); /* Begin TFTP get */
+#ifdef CONFIG_CMD_TFTPSRV +extern void TftpStartServer(void); /* Wait for incoming TFTP put */ +#endif + /**********************************************************************/
#endif /* __TFTP_H__ */
Signed-off-by: Luca Ceresoli luca.ceresoli@comelit.it Cc: Wolfgang Denk wd@denx.de --- README | 1 + common/cmd_net.c | 14 ++++++++++++++ include/net.h | 3 ++- net/net.c | 10 ++++++++-- 4 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/README b/README index b9b0fcb..83dcb2a 100644 --- a/README +++ b/README @@ -685,6 +685,7 @@ The following options need to be configured: (requires CONFIG_CMD_MEMORY) CONFIG_CMD_SOURCE "source" command Support CONFIG_CMD_SPI * SPI serial bus support + CONFIG_CMD_TFTPSRV * TFTP transfer in server mode CONFIG_CMD_USB * USB support CONFIG_CMD_VFD * VFD support (TRAB) CONFIG_CMD_CDP * Cisco Discover Protocol support diff --git a/common/cmd_net.c b/common/cmd_net.c index 8c6f5c8..053162a 100644 --- a/common/cmd_net.c +++ b/common/cmd_net.c @@ -52,6 +52,20 @@ U_BOOT_CMD( "[loadAddress] [[hostIPaddr:]bootfilename]" );
+#ifdef CONFIG_CMD_TFTPSRV +int do_tftpsrv(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{ + return netboot_common(TFTPSRV, cmdtp, argc, argv); +} + +U_BOOT_CMD( + tftpsrv, 2, 1, do_tftpsrv, + "boot image via network acting as a TFTP server", + "[loadAddress]" +); +#endif + + #ifdef CONFIG_CMD_RARP int do_rarpb (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { diff --git a/include/net.h b/include/net.h index 01f7159..018a744 100644 --- a/include/net.h +++ b/include/net.h @@ -364,7 +364,8 @@ extern int NetState; /* Network loop state */ extern int NetRestartWrap; /* Tried all network devices */ #endif
-typedef enum { BOOTP, RARP, ARP, TFTP, DHCP, PING, DNS, NFS, CDP, NETCONS, SNTP } proto_t; +typedef enum { BOOTP, RARP, ARP, TFTP, DHCP, PING, DNS, NFS, CDP, NETCONS, SNTP, + TFTPSRV } proto_t;
/* from net/net.c */ extern char BootFile[128]; /* Boot File name */ diff --git a/net/net.c b/net/net.c index 79afd8b..a9a2f8b 100644 --- a/net/net.c +++ b/net/net.c @@ -388,7 +388,11 @@ restart: /* always use ARP to get server ethernet address */ TftpStart(); break; - +#ifdef CONFIG_CMD_TFTPSRV + case TFTPSRV: + TftpStartServer(); + break; +#endif #if defined(CONFIG_CMD_DHCP) case DHCP: BootpTry = 0; @@ -1730,7 +1734,9 @@ static int net_check_prereq (proto_t protocol) #if defined(CONFIG_CMD_PING) || defined(CONFIG_CMD_SNTP) common: #endif - +#ifdef CONFIG_CMD_TFTPSRV + case TFTPSRV: +#endif if (NetOurIP == 0) { puts ("*** ERROR: `ipaddr' not set\n"); return (1);
participants (7)
-
Albert ARIBAUD -
Detlev Zundel -
Luca Ceresoli -
Luca Ceresoli -
Luca Ceresoli -
Mike Frysinger -
Wolfgang Denk