[U-Boot-Users] [PATCH] USB Storage, add meaningful return value

[Aras Vaichas]

This patch changes the "usb storage" command to return success iff it
finds a USB storage device, otherwise it returns error.

This enables you to check for a USB storage device before trying to
access it:

if usb storage; then fatload usb 0 $loadaddr $filename; fi

______________________________________________________________________
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email
______________________________________________________________________

[Wolfgang Denk]

Dear Aras,

in message <47E836F8...@magtech.com.au> you wrote:
>
> This patch changes the "usb storage" command to return success iff it
> finds a USB storage device, otherwise it returns error.

Thanks. I appreciate your contribution, but please fix the coding
style:

> @@ -196,9 +196,13 @@
> for (i = 0; i < usb_max_devs; i++) {

There is probably a { missing before this line (as this is not a
single-line statement).

> printf (" Device %d: ", i);
> dev_print(&usb_dev_desc[i]);
> + return 0;
> }
> else
> + {

...and then this should read

} else {

Thanks.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
The explanation requiring the fewest assumptions is the most likely
to be correct. -- William of Occam

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
U-Boot-Users mailing list
U-Boot...@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/u-boot-users

[Jerry Van Baren]

Wolfgang Denk wrote:
> Dear Aras,
>
> in message <47E836F8...@magtech.com.au> you wrote:
>> This patch changes the "usb storage" command to return success iff it
>> finds a USB storage device, otherwise it returns error.
>
> Thanks. I appreciate your contribution, but please fix the coding
> style:
>
>> @@ -196,9 +196,13 @@
>> for (i = 0; i < usb_max_devs; i++) {
>
> There is probably a { missing before this line (as this is not a
> single-line statement).
>
>> printf (" Device %d: ", i);
>> dev_print(&usb_dev_desc[i]);
>> + return 0;
>> }
>> else
>> + {
>
> ...and then this should read
>
> } else {
>
> Thanks.
>
> Best regards,
>
> Wolfgang Denk

Just in case Wolfgang was too terse, I'll add to his critique... the
original code is the origin of the coding violations:

void usb_stor_info(void)
{
int i;

if (usb_max_devs > 0)

for (i = 0; i < usb_max_devs; i++) {

printf (" Device %d: ", i);
dev_print(&usb_dev_desc[i]);
}

else
printf("No storage devices, perhaps not 'usb
start'ed..?\n");
}

It doesn't have braces on the "if (usb_max_devs > 0)" which is
syntactically OK but the source of the coding violation. Please add
braces after the "if" and "} else {" per Wolfgang's comments.

Thanks for making the code a little better and a little prettier, ;-)
gvb

[Aras Vaichas]

Jerry Van Baren wrote:
> Wolfgang Denk wrote:
>> Dear Aras,
>>
>> in message <47E836F8...@magtech.com.au> you wrote:
>>> This patch changes the "usb storage" command to return success iff it
>>> finds a USB storage device, otherwise it returns error.
>>
>> Thanks. I appreciate your contribution, but please fix the coding
>> style:

Done.

[Markus Klotzbücher]

Aras Vaichas <ar...@magtech.com.au> writes:

> Jerry Van Baren wrote:
>> Wolfgang Denk wrote:
>>> Dear Aras,
>>>
>>> in message <47E836F8...@magtech.com.au> you wrote:
>>>> This patch changes the "usb storage" command to return success iff it
>>>> finds a USB storage device, otherwise it returns error.
>>>
>>> Thanks. I appreciate your contribution, but please fix the coding
>>> style:
> Done.

Thanks, i'll pick this one up. Please remember to add a proper
Signed-off-by: line next time.

Best regards

Markus Klotzbuecher

--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de

[Jon Loeliger]

Jerry Van Baren wrote:

> Just in case Wolfgang was too terse, I'll add to his critique... the
> original code is the origin of the coding violations:
>
> void usb_stor_info(void)
> {
> int i;
>
> if (usb_max_devs > 0)
> for (i = 0; i < usb_max_devs; i++) {
> printf (" Device %d: ", i);
> dev_print(&usb_dev_desc[i]);
> }
> else
> printf("No storage devices, perhaps not 'usb
> start'ed..?\n");
> }
>
> It doesn't have braces on the "if (usb_max_devs > 0)" which is
> syntactically OK but the source of the coding violation. Please add
> braces after the "if" and "} else {" per Wolfgang's comments.
>
> Thanks for making the code a little better and a little prettier, ;-)
> gvb
>

Hey Stefan,

What do you think? Wouldn't a policy of _always_ using
braces even for single sub-statements have just made this
a _correct_ no-brainer from the onset? :-)

jdl

PS -- No, no point. Why do you ask?

[Scott Wood]

On Tue, Mar 25, 2008 at 09:23:43AM -0500, Jon Loeliger wrote:
> What do you think? Wouldn't a policy of _always_ using
> braces even for single sub-statements have just made this
> a _correct_ no-brainer from the onset? :-)

Yeah, but it'd be ugly. :-)

-Scott

[Jon Loeliger]

Scott Wood wrote:
> On Tue, Mar 25, 2008 at 09:23:43AM -0500, Jon Loeliger wrote:
>> What do you think? Wouldn't a policy of _always_ using
>> braces even for single sub-statements have just made this
>> a _correct_ no-brainer from the onset? :-)
>
> Yeah, but it'd be ugly. :-)

Luckily, everyone is entitled to their own wrong opinion. :-)

jdl

[Stefan Roese]

On Tuesday 25 March 2008, Jon Loeliger wrote:
> > It doesn't have braces on the "if (usb_max_devs > 0)" which is
> > syntactically OK but the source of the coding violation. Please add
> > braces after the "if" and "} else {" per Wolfgang's comments.
> >
> > Thanks for making the code a little better and a little prettier, ;-)
> > gvb
>
> Hey Stefan,

Why me? :)

> What do you think? Wouldn't a policy of _always_ using
> braces even for single sub-statements have just made this
> a _correct_ no-brainer from the onset? :-)

I think I read some time on LKML, that braces should be used on all multi-line
paragraphs. So even one statement with a comment should have braces. That's
what I would prefer. And braces on both parts of the "else" if one needs
them. So something like is what I would vote for:

if (a == b) {
/* some comment */
c = 1;
} else {
c = 2;
}

But I still prefer using no braces on single line paragraph:

if (a == b)
c = 1;
else
c = 2;

Best regards,
Stefan

=====================================================================

DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de

=====================================================================

[Markus Klotzbücher]

Aras Vaichas <ar...@magtech.com.au> writes:

> ______________________________________________________________________
> This email has been scanned by the MessageLabs Email Security System.
> For more information please visit http://www.messagelabs.com/email

> ______________________________________________________________________--- a/include/usb.h 2008-03-19 13:47:18.000000000 +1100

Its my fault for not paying attention correctly, but this line
essentially broke git-am so that this chunk didn't get applied
correctly.

> -void usb_stor_info(void)
> +int usb_stor_info(void)
> {
> int i;
>
> - if (usb_max_devs > 0)
> + if (usb_max_devs > 0) {

> for (i = 0; i < usb_max_devs; i++) {
> printf (" Device %d: ", i);
> dev_print(&usb_dev_desc[i]);

> + return 0;
> }

Returning here will result in only the first of all storage devices to
be printed.

> - else
> + } else {

> printf("No storage devices, perhaps not 'usb start'ed..?\n");

> + return 1;
> + }
> }
>
> /*********************************************************************************

I committed the following patch to fix this

diff --git a/common/usb_storage.c b/common/usb_storage.c
index 81d2f92..d263b6c 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -196,12 +196,12 @@ int usb_stor_info(void)

for (i = 0; i < usb_max_devs; i++) {
printf (" Device %d: ", i);
dev_print(&usb_dev_desc[i]);

- return 0;
}
- } else {
- printf("No storage devices, perhaps not 'usb start'ed..?\n");
- return 1;
+ return 0;
}
+
+ printf("No storage devices, perhaps not 'usb start'ed..?\n");
+ return 1;
}

/*********************************************************************************

Best regards

Markus

--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace