[U-Boot] [PATCH] Make TFTP Quiet

[U-Boot] [PATCH] Make TFTP Quiet

[Robin Getz]

[U-Boot] [PATCH] Make TFTP Quiet

[Timur Tabi]

On Mon, Aug 10, 2009 at 3:26 PM, Robin Getz<rgetz@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.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] Make TFTP Quiet

[Scott Wood]

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

[U-Boot] [PATCH] Make TFTP Quiet

[Robin Getz]

On Mon 10 Aug 2009 16:55, Timur Tabi pondered:
> On Mon, Aug 10, 2009 at 3:26 PM, Robin Getz<rgetz@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...

[U-Boot] [PATCH] Make TFTP Quiet

[Detlev Zundel]

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

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

[U-Boot] [PATCH] Make TFTP Quiet

[Scott Wood]

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

[U-Boot] [PATCH] Make TFTP Quiet

[Robin Getz]

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

[U-Boot] [PATCH] Make TFTP Quiet

[Mike Frysinger]

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", (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 

[U-Boot] [PATCH] Make TFTP Quiet

[Detlev Zundel]

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

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