[U-Boot] [PATCH] Make TFTP Quiet
From bca49fb5e3045bc175e924999a4015804c39c5c6 Mon Sep 17 00:00:00 2001 From: Robin Getz rgetz@blackfin.uclinux.org
I was using this when I was looking at some other recent tftp performance, and thought I would share. I really don't think it belongs, as it is (a) trivial and (b) mostly debug... (but I'm trying out some more things with git).
This adds the ability to make tftp a little quieter, which saves some time printing hash marks on the console. It also has the ability to print some download stats for debugging network issues.
Signed-off-by: Robin Getz rgetz@blackfin.uclinux.org
--- net/tftp.c | 29 ++++++++++++++++++++++++----- 1 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c index 0fa7033..9544691 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -22,6 +22,16 @@ /* (for checking the image size) */ #define HASHES_PER_LINE 65 /* Number of "loading" hashes per line */
+#ifdef CONFIG_TFTP_QUIET +#define puts_quiet(fmt) +#else +#define puts_quiet(fmt) puts(fmt); +#endif + +#ifdef CONFIG_TFTP_TIME +static ulong start_time, end_time; +#endif + /* * TFTP operations. */ @@ -340,9 +350,9 @@ TftpHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len) printf ("\n\t %lu MB received\n\t ", TftpBlockWrapOffset>>20); } else { if (((TftpBlock - 1) % 10) == 0) { - putc ('#'); + puts_quiet("#"); } else if ((TftpBlock % (10 * HASHES_PER_LINE)) == 0) { - puts ("\n\t "); + puts_quiet("\n\t "); } }
@@ -432,7 +442,12 @@ TftpHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len) * We received the whole thing. Try to * run it. */ - puts ("\ndone\n"); + puts_quiet("\ndone\n"); +#ifdef CONFIG_TFTP_TIME + end_time = get_timer(0); + printf("Time to download == %ld ms\n", end_time - start_time); + printf("Download rate = %lld bytes/second\n", (long long)(NetBootFileXferSize) * 1000 / (end_time - start_time)); +#endif NetState = NETLOOP_SUCCESS; } break; @@ -460,7 +475,7 @@ TftpTimeout (void) #endif NetStartAgain (); } else { - puts ("T "); + puts_quiet("T "); NetSetTimeout (TftpTimeoutMSecs, TftpTimeout); TftpSend (); } @@ -530,7 +545,11 @@ TftpStart (void)
printf ("Load address: 0x%lx\n", load_addr);
- puts ("Loading: *\b"); + puts_quiet("Loading: *\b"); + +#ifdef CONFIG_TFTP_TIME + start_time = get_timer(0); +#endif
TftpTimeoutMSecs = TftpRRQTimeoutMSecs; TftpTimeoutCountMax = TftpRRQTimeoutCountMax;
On Mon, Aug 10, 2009 at 3:26 PM, Robin Getzrgetz@blackfin.uclinux.org wrote:
From bca49fb5e3045bc175e924999a4015804c39c5c6 Mon Sep 17 00:00:00 2001 From: Robin Getz rgetz@blackfin.uclinux.org
I was using this when I was looking at some other recent tftp performance, and thought I would share. I really don't think it belongs, as it is (a) trivial and (b) mostly debug... (but I'm trying out some more things with git).
This adds the ability to make tftp a little quieter, which saves some time printing hash marks on the console. It also has the ability to print some download stats for debugging network issues.
Signed-off-by: Robin Getz rgetz@blackfin.uclinux.org
net/tftp.c | 29 ++++++++++++++++++++++++----- 1 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c index 0fa7033..9544691 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -22,6 +22,16 @@ /* (for checking the image size) */ #define HASHES_PER_LINE 65 /* Number of "loading" hashes per line */
+#ifdef CONFIG_TFTP_QUIET +#define puts_quiet(fmt) +#else +#define puts_quiet(fmt) puts(fmt); +#endif
This looks backwards to me. I would do this:
#ifdef CONFIG_TFTP_QUIET #define puts(x) puts_quiet(x) #endif
That way, you don't need to change all of the puts calls to puts_quiet. Plus, having the normal calls be "puts_quiet" that changes to puts when QUIET is *not* enabled just feels wrong.
On Mon, Aug 10, 2009 at 03:55:20PM -0500, Timur Tabi wrote:
+#ifdef CONFIG_TFTP_QUIET +#define puts_quiet(fmt) +#else +#define puts_quiet(fmt) puts(fmt); +#endif
This looks backwards to me. I would do this:
#ifdef CONFIG_TFTP_QUIET #define puts(x) puts_quiet(x) #endif
That way, you don't need to change all of the puts calls to puts_quiet. Plus, having the normal calls be "puts_quiet" that changes to puts when QUIET is *not* enabled just feels wrong.
I think the point was to establish two different functions a caller may use -- puts() which always prints, and puts_quiet() which only prints when the user hasn't asked for silence.
-Scott
On Mon 10 Aug 2009 16:55, Timur Tabi pondered:
On Mon, Aug 10, 2009 at 3:26 PM, Robin Getzrgetz@blackfin.uclinux.org wrote:
--- a/net/tftp.c +++ b/net/tftp.c @@ -22,6 +22,16 @@ /* (for checking the image size) */ #define HASHES_PER_LINE 65 /* Number of "loading" hashes per line */
+#ifdef CONFIG_TFTP_QUIET +#define puts_quiet(fmt) +#else +#define puts_quiet(fmt) puts(fmt); +#endif
This looks backwards to me. I would do this:
#ifdef CONFIG_TFTP_QUIET #define puts(x) puts_quiet(x) #endif
That way, you don't need to change all of the puts calls to puts_quiet. Plus, having the normal calls be "puts_quiet" that changes to puts when QUIET is *not* enabled just feels wrong.
There are other puts that you don't want quiet...
puts ("Starting again\n\n"); puts ("\nRetry count exceeded; starting again\n");
Otherwise - if you have a bad network - it will never output anything...
Hi Timur,
+#ifdef CONFIG_TFTP_QUIET +#define puts_quiet(fmt) +#else +#define puts_quiet(fmt) puts(fmt); +#endif
This looks backwards to me. I would do this:
#ifdef CONFIG_TFTP_QUIET #define puts(x) puts_quiet(x) #endif
That way, you don't need to change all of the puts calls to puts_quiet. Plus, having the normal calls be "puts_quiet" that changes to puts when QUIET is *not* enabled just feels wrong.
Just as a general remark - I consider it a bad idea to "overload" well known functions with non-standard behaviour. This breaks the "principle of least surprise" which turns out to be very valuable.
Cheers Detlev
On Wed, Aug 12, 2009 at 12:02:33PM +0200, Detlev Zundel wrote:
Hi Timur,
+#ifdef CONFIG_TFTP_QUIET +#define puts_quiet(fmt) +#else +#define puts_quiet(fmt) puts(fmt); +#endif
This looks backwards to me. I would do this:
#ifdef CONFIG_TFTP_QUIET #define puts(x) puts_quiet(x) #endif
That way, you don't need to change all of the puts calls to puts_quiet. Plus, having the normal calls be "puts_quiet" that changes to puts when QUIET is *not* enabled just feels wrong.
Just as a general remark - I consider it a bad idea to "overload" well known functions with non-standard behaviour. This breaks the "principle of least surprise" which turns out to be very valuable.
Technically, U-Boot's puts() is already non-standard (no automatic newline)...
But there's no redefinition in the original patch. It's just introducing a new puts_quiet().
-Scott
On Wed 12 Aug 2009 15:48, Scott Wood pondered:
On Wed, Aug 12, 2009 at 12:02:33PM +0200, Detlev Zundel wrote:
Hi Timur,
+#ifdef CONFIG_TFTP_QUIET +#define puts_quiet(fmt) +#else +#define puts_quiet(fmt) puts(fmt); +#endif
This looks backwards to me. I would do this:
#ifdef CONFIG_TFTP_QUIET #define puts(x) puts_quiet(x) #endif
That way, you don't need to change all of the puts calls to puts_quiet. Plus, having the normal calls be "puts_quiet" that changes to puts when QUIET is *not* enabled just feels wrong.
Just as a general remark - I consider it a bad idea to "overload" well known functions with non-standard behaviour. This breaks the "principle of least surprise" which turns out to be very valuable.
Technically, U-Boot's puts() is already non-standard (no automatic newline)...
But there's no redefinition in the original patch. It's just introducing a new puts_quiet().
Yeah, I think Detlev was just commenting on Timur's suggestion to the patch, not on the original patch...
I would be happy to change things to a debugX() - but this changes everyone's default behaviour (it becomes opt in, not opt-out), and most likely would cause some puzzlement when normally things don't print like they use to...
Hi,
On Wed 12 Aug 2009 15:48, Scott Wood pondered:
On Wed, Aug 12, 2009 at 12:02:33PM +0200, Detlev Zundel wrote:
Hi Timur,
+#ifdef CONFIG_TFTP_QUIET +#define puts_quiet(fmt) +#else +#define puts_quiet(fmt) puts(fmt); +#endif
This looks backwards to me. I would do this:
#ifdef CONFIG_TFTP_QUIET #define puts(x) puts_quiet(x) #endif
That way, you don't need to change all of the puts calls to puts_quiet. Plus, having the normal calls be "puts_quiet" that changes to puts when QUIET is *not* enabled just feels wrong.
Just as a general remark - I consider it a bad idea to "overload" well known functions with non-standard behaviour. This breaks the "principle of least surprise" which turns out to be very valuable.
Technically, U-Boot's puts() is already non-standard (no automatic newline)...
But there's no redefinition in the original patch. It's just introducing a new puts_quiet().
Yeah, I think Detlev was just commenting on Timur's suggestion to the patch, not on the original patch...
Correct.
Cheers Detlev
On Monday 10 August 2009 16:26:20 Robin Getz wrote:
From bca49fb5e3045bc175e924999a4015804c39c5c6 Mon Sep 17 00:00:00 2001 From: Robin Getz rgetz@blackfin.uclinux.org
I was using this when I was looking at some other recent tftp performance, and thought I would share. I really don't think it belongs, as it is (a) trivial and (b) mostly debug... (but I'm trying out some more things with git).
This adds the ability to make tftp a little quieter, which saves some time printing hash marks on the console. It also has the ability to print some download stats for debugging network issues.
Signed-off-by: Robin Getz rgetz@blackfin.uclinux.org
net/tftp.c | 29 ++++++++++++++++++++++++----- 1 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c index 0fa7033..9544691 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -22,6 +22,16 @@ /* (for checking the image size) */ #define HASHES_PER_LINE 65 /* Number of "loading" hashes per line */
+#ifdef CONFIG_TFTP_QUIET +#define puts_quiet(fmt) +#else +#define puts_quiet(fmt) puts(fmt);
macros shouldnt insert semi-colons for you
putc ('#');
puts_quiet("#");
you might as well fix the style while you're here (no spaces before the open paren)
printf("Time to download == %ld ms\n", end_time - start_time);printf("Download rate = %lld bytes/second\n", (longlong)(NetBootFileXferSize) * 1000 / (end_time - start_time));
shouldnt they both be = or == ? do you really need 64bit math ? -mike
participants (5)
-
Detlev Zundel -
Mike Frysinger -
Robin Getz -
Scott Wood -
Timur Tabi