| Message ID | 1303143594-5345-4-git-send-email-luca.ceresoli@comelit.it |
|---|---|
| State | Superseded |
| Headers | show |
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 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. 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
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 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. > > 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 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. >> 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
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
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
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 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? 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 >> 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? > > 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
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
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 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. > > 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
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);
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(-)