RedBoot patches regarding redboot_getc_terminate

RedBoot patches regarding redboot_getc_terminate

[Doyle, Patrick]

Sometime since September of 2005, we updated our local eCos repository,
which includes Andrew Dyer's RedBoot patches to call
'redboot_getc_terminate()' in various error scenarios.  Since that time, we
have observed that anytime we use YMODEM to download code to our boards, it
terminates with an error message.

I hate training myself to ignore error messages... it always comes back to
haunt me.

So, I looked into this, and I see in 'load_elf_image()', near line 455 of
"load.c", there is a call to 'redboot_getc_terminate()' with an an argument
of 'true', indicating that the data transfer should be aborted.  I changed
the abort flag back to 'false', and things have returned to normal for us.

However, before I submit a formal patch undoing the work that Andrew did
(which had to do with terminating TFTP transfers gracefully), I thought I
would check in with folks, Andrew especially.

What do folks think?  Should I generate the one line patch (along with the 5
line ChangeLog entry)?

--wpd


Patrick Doyle
Manager, Digital Systems Group
DTC Communications, Inc.
Phone: (603) 546-2179
Fax: (603) 880-6965
Email: wpd@dtccom.com

 

This communication is from DTC Communications, Inc. and is intended to be
confidential and solely for the use of the persons or entities addressed
above.  If you are not an intended recipient, be aware that the information
contained herein may be protected from unauthorized use by privilege or law,
and any copying, distribution, disclosure, or other use of this information
is prohibited.  If you have received this communication in error, please
contact the sender by return e-mail or telephone the above number
immediately and delete or destroy all copies.  Thank you for your
cooperation.

Re: RedBoot patches regarding redboot_getc_terminate

[Andrew Lunn]

On Thu, May 18, 2006 at 09:17:15AM -0400, Doyle, Patrick wrote:
> Sometime since September of 2005, we updated our local eCos repository,
> which includes Andrew Dyer's RedBoot patches to call
> 'redboot_getc_terminate()' in various error scenarios.  Since that time, we
> have observed that anytime we use YMODEM to download code to our boards, it
> terminates with an error message.
> 
> I hate training myself to ignore error messages... it always comes back to
> haunt me.
> 
> So, I looked into this, and I see in 'load_elf_image()', near line 455 of
> "load.c", there is a call to 'redboot_getc_terminate()' with an an argument
> of 'true', indicating that the data transfer should be aborted.  I changed
> the abort flag back to 'false', and things have returned to normal for us.
> 
> However, before I submit a formal patch undoing the work that Andrew did
> (which had to do with terminating TFTP transfers gracefully), I thought I
> would check in with folks, Andrew especially.
> 
> What do folks think?  Should I generate the one line patch (along with the 5
> line ChangeLog entry)?

What happens to TFTP transfers with your change? Are they terminated
gracefully? Or do they hang around until the server times out and
kills them?

      Andrew

RE: RedBoot patches regarding redboot_getc_terminate

[Andrew Dyer]

> Perhaps the 'tftp_stream_terminate()' function should be modified to issue
> the "Remaining file contents not required" message in the case of a normal
> termination.

That sounds fine to me, but I can't test as I would have to pull
the old target board out of mothballs (ie actually FIND it) and
resurrect a cygwin server to do any testing.  We haven't done
much redboot/ecos stuff for a long time.

RE: RedBoot patches regarding redboot_getc_terminate

[Doyle, Patrick]

> -----Original Message-----
> From: David Vrabel [mailto:dvrabel@arcom.com] 
> Sent: Thursday, May 18, 2006 11:55 AM
> To: Andrew Lunn
> Cc: Doyle, Patrick; 'ecos-devel@sources.redhat.com'; 'Andrew Dyer'
> Subject: Re: RedBoot patches regarding redboot_getc_terminate
> 
> 
> Andrew Lunn wrote:
> >> I'm not entirely happy with the fix. It terminates the 
> download when all
> >> the relevant bits of the ELF have been transferred. The causes the
> >> sender to think that the file transfer has failed which causes some
> >> customer confusion.
> > 
> > It is possible to include a text string in the error message. 
> > 
> > http://www.faqs.org/rfcs/rfc1350.html, figure 5-4. 
> > 
> > So maybe a message like "Remaining file contents not required" would
> > prevent confusion. This assumes the lame tftp servers actually display
> > the message to the user.
> 
> This would take care of the TFTP transfers.  But what about 
> the Y-modem transfers?

Well, 'retboot_getc_terminate(false)' has always worked for me in the past,
and continues to work for me now that I've put it back in.

Perhaps the 'tftp_stream_terminate()' function should be modified to issue
the "Remaining file contents not required" message in the case of a normal
termination.

> 
> >> I think a better solution would be for the downloader to continue to
> >> transfer the remaining portions of the ELF image and just throw them
away."
> > 
> > This can be a lot of data, eg the complete debug symbol table if the
> > image has been compiled -g. I would not really be in favor of that.
> 
> Agreed.  Although I think this is what RedBoot used to do for Y-modem
> transfers.

Actually, as confusing as this is to me, this is what it _still_ does!  I'm
pretty sure I remember the behavior you describe, but I'm darn'd if I can
see it in the code (nor in practice).

So if I submit a modified patch that restores the call to
'redboot_getc_terminate()' to its original (pre Andrew Dyer) state and
modify 'tftp_stream_terminate()' to issue different strings based on normal
vs. abnormal termination conditions, is there anybody who can test the TFTP
changes?

--wpd

Re: RedBoot patches regarding redboot_getc_terminate

[David Vrabel]

Andrew Lunn wrote:
>> I'm not entirely happy with the fix. It terminates the download when all
>> the relevant bits of the ELF have been transferred. The causes the
>> sender to think that the file transfer has failed which causes some
>> customer confusion.
> 
> It is possible to include a text string in the error message. 
> 
> http://www.faqs.org/rfcs/rfc1350.html, figure 5-4. 
> 
> So maybe a message like "Remaining file contents not required" would
> prevent confusion. This assumes the lame tftp servers actually display
> the message to the user.

This would take care of the TFTP transfers.  But what about the Y-modem
transfers?

>> I think a better solution would be for the downloader to continue to
>> transfer the remaining portions of the ELF image and just throw them away."
> 
> This can be a lot of data, eg the complete debug symbol table if the
> image has been compiled -g. I would not really be in favor of that.

Agreed.  Although I think this is what RedBoot used to do for Y-modem
transfers.

David Vrabel
-- 
David Vrabel, Design Engineer

Arcom, Clifton Road           Tel: +44 (0)1223 411200 ext. 3233
Cambridge CB1 7EA, UK         Web: http://www.arcom.com/

Re: RedBoot patches regarding redboot_getc_terminate

[Andrew Lunn]

> I'm not entirely happy with the fix. It terminates the download when all
> the relevant bits of the ELF have been transferred. The causes the
> sender to think that the file transfer has failed which causes some
> customer confusion.

It is possible to include a text string in the error message. 

http://www.faqs.org/rfcs/rfc1350.html, figure 5-4. 

So maybe a message like "Remaining file contents not required" would
prevent confusion. This assumes the lame tftp servers actually display
the message to the user.

> I think a better solution would be for the downloader to continue to
> transfer the remaining portions of the ELF image and just throw them away."

This can be a lot of data, eg the complete debug symbol table if the
image has been compiled -g. I would not really be in favor of that.

      Andrew

Re: RedBoot patches regarding redboot_getc_terminate

[David Vrabel]

Doyle, Patrick wrote:
>> What happens to TFTP transfers with your change? Are they terminated
>> gracefully? Or do they hang around until the server times out and
>> kills them?
>>
>>       Andrew
>>
> Unfortunately, I don't have any means to check that.  Which is why I brought
> it up as a topic for discussion.  Then I realized that it would be easier to
> discuss if somebody who _did_ have a means to check that checked that, which
> led to me posting the patch :-)
> 
> IIRC, a TFTP server will keep spewing out packets until it has sent the
> whole file, and will retry and retransmit if the client stops responding.
> So, it guess it depends on what the TFTP transport stream (implemented in
> code somewhere in RedBoot) does when it gets a "terminate" call that isn't
> an abort...
> 
> Hmmm... looking at the code, I see something that looks like:
> 
> if (abort)
>   tftp_error_ack(...)
> else
>   tftp_ack(...)
> 
> which looks to me like TFTP folks would be getting some sort of similar
> error message for TFTP transfers.
> 
> I wonder why I'm the only one whose noticed this?  (he asks in a plaintive,
> "why do these things always happen to me" voice)

No.  I noticed (or rather a customer did) but didn't have time to really
look at it or implement a solution.  I guess I should have a least
mentioned it though. Sorry.

I don't think your patch does the correct thing wrt TFTP transfers since
we're back to the original situation of TFTP connections remaining open
for ages.

Here's a snippet from our internal bug tracker wrt lingering TFTP
connection issue:

"When loading ELF images using TFTP RedBoot fails to ACK the last
received data block. This causes the server to timeout and retransmit
the last data block several times. See attached packet captures for details.

This can cause problems with lame TFTP servers (like tftpd32 for
Windows) that can only handle one transfer at a time.

----

The ELF image used in testing had a trailing .comment section which
RedBoot isn't interested in so it stopped reading data from the TFTP
stream and hence didn't ack subsequent data blocks (I don't think it
even reads them from the network).

I'd suggest stripping unneeded sections from eCos/RedBoot ELF images. e.g.,
$ arm-elf-string --remove-section=.comment foo.elf

----

I'm not entirely happy with the fix. It terminates the download when all
the relevant bits of the ELF have been transferred. The causes the
sender to think that the file transfer has failed which causes some
customer confusion.

I think a better solution would be for the downloader to continue to
transfer the remaining portions of the ELF image and just throw them away."

Perhaps it's best to revert the fix for the lingering TFTP connections
and note instead that ELFs really need all unneccesary sections stripped
before transferring to the target?  At least until a complete solution
(e.g., my suggested solution in the paragraph above) has been implemented.

David Vrabel

ps. If possible, attach patches as text/plain so people can easily read
them in their mail readers.  Thanks.  Sticking a .txt extension at the
end of the file name may do the trick if you're using a lame mail client
that doesn't recognize patches as text/plain.
-- 
David Vrabel, Design Engineer

Arcom, Clifton Road           Tel: +44 (0)1223 411200 ext. 3233
Cambridge CB1 7EA, UK         Web: http://www.arcom.com/

Re: RedBoot patches regarding redboot_getc_terminate

[Andrew Lunn]

On Thu, May 18, 2006 at 09:39:12AM -0400, Doyle, Patrick wrote:
> > 
> > What happens to TFTP transfers with your change? Are they terminated
> > gracefully? Or do they hang around until the server times out and
> > kills them?
> > 
> >       Andrew
> > 
> Unfortunately, I don't have any means to check that.  Which is why I brought
> it up as a topic for discussion.  Then I realized that it would be easier to
> discuss if somebody who _did_ have a means to check that checked that, which
> led to me posting the patch :-)
> 
> IIRC, a TFTP server will keep spewing out packets until it has sent the
> whole file, and will retry and retransmit if the client stops responding.
> So, it guess it depends on what the TFTP transport stream (implemented in
> code somewhere in RedBoot) does when it gets a "terminate" call that isn't
> an abort...
> 
> Hmmm... looking at the code, I see something that looks like:
> 
> if (abort)
>   tftp_error_ack(...)
> else
>   tftp_ack(...)
> 
> which looks to me like TFTP folks would be getting some sort of similar
> error message for TFTP transfers.

Well, it would depend on the exact error returned to the TFTP server. 

[Goes and looks at the code][Goes and looks at the RFC]

7. Premature Termination

   If a request can not be granted, or some error occurs during the
   transfer, then an ERROR packet (opcode 5) is sent.  This is only a
   courtesy since it will not be retransmitted or acknowledged, so it
   may never be received.  Timeouts must also be used to detect errors.

It looks like reporting an error is the only way to abort the rest of
the transfer. Depending on the TFTP server it might put a comment in
/var/log/syslog etc, but most users don't read that unless something
serious has gone wrong.

I suggest for [X-Z]modem you read the RFC or whatever and find out if
there is a graceful way of closing the connection, as opposed to a
none graceful way which it currently seems to be doing according to
your report.

     Andrew

RE: RedBoot patches regarding redboot_getc_terminate

[Doyle, Patrick]

> 
> What happens to TFTP transfers with your change? Are they terminated
> gracefully? Or do they hang around until the server times out and
> kills them?
> 
>       Andrew
> 
Unfortunately, I don't have any means to check that.  Which is why I brought
it up as a topic for discussion.  Then I realized that it would be easier to
discuss if somebody who _did_ have a means to check that checked that, which
led to me posting the patch :-)

IIRC, a TFTP server will keep spewing out packets until it has sent the
whole file, and will retry and retransmit if the client stops responding.
So, it guess it depends on what the TFTP transport stream (implemented in
code somewhere in RedBoot) does when it gets a "terminate" call that isn't
an abort...

Hmmm... looking at the code, I see something that looks like:

if (abort)
  tftp_error_ack(...)
else
  tftp_ack(...)

which looks to me like TFTP folks would be getting some sort of similar
error message for TFTP transfers.

I wonder why I'm the only one whose noticed this?  (he asks in a plaintive,
"why do these things always happen to me" voice)

--wpd

RE: RedBoot patches regarding redboot_getc_terminate

[Doyle, Patrick]

[-- Attachment #1: Type: text/plain, Size: 106 bytes --]

Ahh what the heck... here's the patch anyway, just to add a little more meat
to the discussion...

--wpd


[-- Attachment #2: redboot-load.patch --]
[-- Type: application/octet-stream, Size: 1578 bytes --]

Index: ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/redboot/current/ChangeLog,v
retrieving revision 1.237
diff -u -r1.237 ChangeLog
--- ChangeLog	30 Jan 2006 21:04:04 -0000	1.237
+++ ChangeLog	18 May 2006 13:25:01 -0000
@@ -1,3 +1,8 @@
+2006-05-18  Patrick Doyle  <wpd@dtccom.com>
+
+	* src/load.c (load_elf_image): Terminate ELF transfers gracefully
+	after reading all of the sections required by RedBoot.
+
 2005-11-23  Peter Korsgaard  <peter.korsgaard@barco.com>
 
 	* src/gunzip.c (do_gunzip): Fixed diag_printf format string warnings.
Index: src/load.c
===================================================================
RCS file: /cvs/ecos/ecos/packages/redboot/current/src/load.c,v
retrieving revision 1.45
diff -u -r1.45 load.c
--- src/load.c	9 Sep 2005 13:26:03 -0000	1.45
+++ src/load.c	18 May 2006 13:25:01 -0000
@@ -430,10 +430,9 @@
         entry_address = ehdr.e_entry;
     }
 
-    // nak everything to stop the transfer, since redboot
-    // usually doesn't read all the way to the end of the
-    // elf files.
-    redboot_getc_terminate(true);
+    // Terminate the transfer gracefully at this point, since redboot
+    // usually doesn't read all the way to the end of the elf files.
+    redboot_getc_terminate(false);
     if (addr_offset) diag_printf("Address offset = %p\n", (void *)addr_offset);
     diag_printf("Entry point: %p, address range: %p-%p\n", 
                 (void*)entry_address, (void *)load_address, (void *)load_address_end);