Skip to contentSkip to conversation

Make TFTP Quiet

PATCH New Robin Getz 5 participants 9 messages Aug 10, 2009 – Aug 13, 2009

lore
Share & tools
lore
#1 lore raw

PATCH 1/1 Make TFTP Quiet

Quoted 26 lines · Robin Getz

From bca49fb5e3045bc175e924999a4015804c39c5c6 Mon Sep 17 00:00:00 2001
From: Robin Getz <[email protected]>

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 <[email protected]>

---
?net/tftp.c | ? 29 ++++++++++++++++++++++++----- ?1 files changed, 24 insertions(+), 5 deletions(-)

net/tftp.c +0 -0
index 0fa7033..9544691 100644
@@ -22,6 +22,16 @@

? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?/* (for checking the image size) ? ? ? ?*/ ?#define HASHES_PER_LINE ? ? ? ?65 ? ? ? ? ? ? ?/* Number of "loading" hashes per line ?*/

net/tftp.c

Quoted 5 lines
net/tftp.c +5 -0
+#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.

Signature · 2 lines
Timur Tabi
Linux kernel developer at Freescale
#2 lore raw

PATCH 1/1 Make TFTP Quiet

Quoted 8 lines · Timur Tabi

Review

Quoted 5 lines
(quoted diff) +5 -0
+#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:

Quoted 7 lines

#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

author
#3 lore raw

PATCH 1/1 Make TFTP Quiet

In reply to Timur Tabi

On Mon 10 Aug 2009 16:55, Timur Tabi pondered:

Quoted 15 lines · Robin Getz

net/tftp.c

Quoted 11 lines
net/tftp.c +0 -0
@@ -22,6 +22,16 @@

? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?/* (for checking the image size) ? ? ? ?*/ ?#define HASHES_PER_LINE ? ? ? ?65 ? ? ? ? ? ? ?/* Number of "loading" hashes per line ?*/

net/tftp.c +5 -0
+#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:

Quoted 7 lines

#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...

#4 lore raw

PATCH 1/1 Make TFTP Quiet

In reply to Timur Tabi

Hi Timur,

Quoted 8 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+#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:

Quoted 7 lines

#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

Signature · 8 lines
It was actually a very beautiful thing to see a sunrise, cause' that's
such a calm time of day. It's a wonderful time of day  to get ready to
go to bed.
                                            -- Richard M. Stallman
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de
#5 lore raw

PATCH 1/1 Make TFTP Quiet

Quoted 14 lines · Detlev Zundel

Hi Timur,

Quoted 12 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+#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

Quoted 7 lines
Quoted 3 lines

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

author
#6 lore raw

PATCH 1/1 Make TFTP Quiet

On Wed 12 Aug 2009 15:48, Scott Wood pondered:

Quoted 23 lines · Detlev Zundel
Quoted 14 lines

Hi Timur,

Quoted 12 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+#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

Quoted 7 lines
Quoted 3 lines

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.

Quoted 5 lines

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...

#7 lore raw

PATCH 1/1 Make TFTP Quiet

Hi,

Quoted 24 lines

On Wed 12 Aug 2009 15:48, Scott Wood pondered:

Quoted 23 lines · Detlev Zundel
Quoted 21 lines

Hi Timur,

Quoted 8 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+#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:

Quoted 7 lines

#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.

Quoted 8 lines
Quoted 5 lines

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

Signature · 6 lines
To you I'm an atheist; to God, I'm the Loyal Opposition.
                                        -- Woody Allen
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de
#8 lore raw

PATCH 1/1 Make TFTP Quiet

In reply to Robin Getz
Quoted 23 lines · On Monday 10 August 2009 16:26:20 Robin Getz wrote:

From bca49fb5e3045bc175e924999a4015804c39c5c6 Mon Sep 17 00:00:00 2001
From: Robin Getz <[email protected]>

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 <[email protected]>
---
 net/tftp.c |   29 ++++++++++++++++++++++++-----
 1 files changed, 24 insertions(+), 5 deletions(-)
net/tftp.c +0 -0
index 0fa7033..9544691 100644

net/tftp.c line 28

Quoted 7 lines
net/tftp.c +4 -0
@@ -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

net/tftp.c line 29

Quoted 2 lines
net/tftp.c +1 -1
-				putc ('#');
+				puts_quiet("#");

you might as well fix the style while you're here (no spaces before the open paren)

net/tftp.c line 32

Quoted 3 lines
net/tftp.c +2 -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));

shouldnt they both be = or == ? do you really need 64bit math ?
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090813/1d4969b4/attachment.pgp