[U-Boot] [PATCH] boot cmds: convert to getenv_yesno() with autostart

[U-Boot] [PATCH] boot cmds: convert to getenv_yesno() with autostart

[Mike Frysinger]

Use the new helper func to clean up duplicate logic handling of the
autostart env var.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 common/cmd_fdc.c  |    3 +--
 common/cmd_fdos.c |    2 +-
 common/cmd_ide.c  |    2 +-
 common/cmd_nand.c |    4 ++--
 common/cmd_net.c  |    2 +-
 common/cmd_scsi.c |    2 +-
 common/cmd_usb.c  |    2 +-
 7 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/common/cmd_fdc.c b/common/cmd_fdc.c
index 831a07f..1655bdc 100644
--- a/common/cmd_fdc.c
+++ b/common/cmd_fdc.c
@@ -721,7 +721,6 @@ int do_fdcboot (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	image_header_t *hdr;  /* used for fdc boot */
 	unsigned char boot_drive;
 	int i,nrofblk;
-	char *ep;
 	int rcode = 0;
 #if defined(CONFIG_FIT)
 	const void *fit_hdr = NULL;
@@ -824,7 +823,7 @@ int do_fdcboot (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	load_addr = addr;
 
 	/* Check if we should attempt an auto-start */
-	if (((ep = getenv("autostart")) != NULL) && (strcmp(ep,"yes") == 0)) {
+	if (getenv_yesno("autostart")) {
 		char *local_args[2];
 		extern int do_bootm (cmd_tbl_t *, int, int, char *[]);
 
diff --git a/common/cmd_fdos.c b/common/cmd_fdos.c
index a8822d9..5bb5c64 100644
--- a/common/cmd_fdos.c
+++ b/common/cmd_fdos.c
@@ -99,7 +99,7 @@ int do_fdosboot(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	   size, load_addr);
 
     /* Check if we should attempt an auto-start */
-    if (((ep = getenv("autostart")) != NULL) && (strcmp(ep,"yes") == 0)) {
+    if (getenv_yesno("autostart")) {
 	char *local_args[2];
 	extern int do_bootm (cmd_tbl_t *, int, int, char *[]);
 	local_args[0] = argv[0];
diff --git a/common/cmd_ide.c b/common/cmd_ide.c
index ea0f4a7..f684e78 100644
--- a/common/cmd_ide.c
+++ b/common/cmd_ide.c
@@ -496,7 +496,7 @@ int do_diskboot (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	load_addr = addr;
 
 	/* Check if we should attempt an auto-start */
-	if (((ep = getenv("autostart")) != NULL) && (strcmp(ep,"yes") == 0)) {
+	if (getenv_yesno("autostart")) {
 		char *local_args[2];
 		extern int do_bootm (cmd_tbl_t *, int, int, char *[]);
 
diff --git a/common/cmd_nand.c b/common/cmd_nand.c
index 634d036..f79bd6d 100644
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -711,7 +711,7 @@ static int nand_load_image(cmd_tbl_t *cmdtp, nand_info_t *nand,
 			   ulong offset, ulong addr, char *cmd)
 {
 	int r;
-	char *ep, *s;
+	char *s;
 	size_t cnt;
 	image_header_t *hdr;
 #if defined(CONFIG_FIT)
@@ -787,7 +787,7 @@ static int nand_load_image(cmd_tbl_t *cmdtp, nand_info_t *nand,
 	load_addr = addr;
 
 	/* Check if we should attempt an auto-start */
-	if (((ep = getenv("autostart")) != NULL) && (strcmp(ep, "yes") == 0)) {
+	if (getenv_yesno("autostart")) {
 		char *local_args[2];
 		extern int do_bootm(cmd_tbl_t *, int, int, char *[]);
 
diff --git a/common/cmd_net.c b/common/cmd_net.c
index 44d17db..3320104 100644
--- a/common/cmd_net.c
+++ b/common/cmd_net.c
@@ -213,7 +213,7 @@ netboot_common (proto_t proto, cmd_tbl_t *cmdtp, int argc, char * const argv[])
 	flush_cache(load_addr, size);
 
 	/* Loading ok, check if we should attempt an auto-start */
-	if (((s = getenv("autostart")) != NULL) && (strcmp(s,"yes") == 0)) {
+	if (getenv_yesno("autostart")) {
 		char *local_args[2];
 		local_args[0] = argv[0];
 		local_args[1] = NULL;
diff --git a/common/cmd_scsi.c b/common/cmd_scsi.c
index 6b937f9..0a3dd07 100644
--- a/common/cmd_scsi.c
+++ b/common/cmd_scsi.c
@@ -327,7 +327,7 @@ int do_scsiboot (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	flush_cache (addr, (cnt+1)*info.blksz);
 
 	/* Check if we should attempt an auto-start */
-	if (((ep = getenv("autostart")) != NULL) && (strcmp(ep,"yes") == 0)) {
+	if (getenv_yesno("autostart")) {
 		char *local_args[2];
 		extern int do_bootm (cmd_tbl_t *, int, int, char *[]);
 		local_args[0] = argv[0];
diff --git a/common/cmd_usb.c b/common/cmd_usb.c
index 226ea0d..a7cffa5 100644
--- a/common/cmd_usb.c
+++ b/common/cmd_usb.c
@@ -488,7 +488,7 @@ int do_usbboot(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	flush_cache(addr, (cnt+1)*info.blksz);
 
 	/* Check if we should attempt an auto-start */
-	if (((ep = getenv("autostart")) != NULL) && (strcmp(ep, "yes") == 0)) {
+	if (getenv_yesno("autostart")) {
 		char *local_args[2];
 		extern int do_bootm(cmd_tbl_t *, int, int, char *[]);
 		local_args[0] = argv[0];
-- 
1.7.3.1

[U-Boot] [PATCH] boot cmds: convert to getenv_yesno() with autostart

[Wolfgang Denk]

Dear Mike Frysinger,

In message <1287573475-6737-1-git-send-email-vapier@gentoo.org> you wrote:
> Use the new helper func to clean up duplicate logic handling of the
> autostart env var.
> 
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
>  common/cmd_fdc.c  |    3 +--
>  common/cmd_fdos.c |    2 +-
>  common/cmd_ide.c  |    2 +-
>  common/cmd_nand.c |    4 ++--
>  common/cmd_net.c  |    2 +-
>  common/cmd_scsi.c |    2 +-
>  common/cmd_usb.c  |    2 +-
>  7 files changed, 8 insertions(+), 9 deletions(-)

Applied to "next" branch, 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: wd at denx.de
Work 8 hours, sleep 8 hours; but not the same 8 hours.

[U-Boot] [PATCH] boot cmds: convert to getenv_yesno() with autostart

[Mike Frysinger]

On Sunday, November 28, 2010 16:02:49 Wolfgang Denk wrote:
> Mike Frysinger wrote:
> > Use the new helper func to clean up duplicate logic handling of the
> > autostart env var.
> > 
> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> > ---
> > 
> >  common/cmd_fdc.c  |    3 +--
> >  common/cmd_fdos.c |    2 +-
> >  common/cmd_ide.c  |    2 +-
> >  common/cmd_nand.c |    4 ++--
> >  common/cmd_net.c  |    2 +-
> >  common/cmd_scsi.c |    2 +-
> >  common/cmd_usb.c  |    2 +-
> >  7 files changed, 8 insertions(+), 9 deletions(-)
> 
> Applied to "next" branch, thanks.

hrm, after running this through our test bench, it seems the old code and new 
code are not functionality equivalent.  it boils down to default values when 
the env var is not set.  some places interpret this to mean "yes" while others 
expect "no".  getenv_yesno() takes the "no" default which doesnt always work.

i can update the API to take a 2nd arg (the default value), or we can punt 
this patch.  it's in "next", so there's less pressure to get it fixed 
immediately ...
-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/20101201/25734285/attachment.pgp 

[U-Boot] [PATCH] boot cmds: convert to getenv_yesno() with autostart

[Wolfgang Denk]

Dear Mike Frysinger,

In message <201012010634.19596.vapier@gentoo.org> you wrote:
>
> hrm, after running this through our test bench, it seems the old code and new 
> code are not functionality equivalent.  it boils down to default values when 
> the env var is not set.  some places interpret this to mean "yes" while others 
> expect "no".  getenv_yesno() takes the "no" default which doesnt always work.

In addition to the default values, there is the difference that the
documentation states that "autostart" has to be set to "yes", i. e. a
plain "y" is not supposed to mean 'yes'.

> i can update the API to take a 2nd arg (the default value), or we can punt
> this patch.  it's in "next", so there's less pressure to get it fixed 
> immediately ...

I think the easiest way to fix this is to revert the commit.

Do you agree?

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: wd at denx.de
Unix is supported by IBM, like a hanging man is supported by rope.
		        - Don Libes & Sandy Ressler: _Life With Unix_

[U-Boot] [PATCH] boot cmds: convert to getenv_yesno() with autostart

[Mike Frysinger]

On Tuesday, December 07, 2010 16:51:41 Wolfgang Denk wrote:
> Mike Frysinger wrote:
> > hrm, after running this through our test bench, it seems the old code and
> > new code are not functionality equivalent.  it boils down to default
> > values when the env var is not set.  some places interpret this to mean
> > "yes" while others expect "no".  getenv_yesno() takes the "no" default
> > which doesnt always work.
> 
> In addition to the default values, there is the difference that the
> documentation states that "autostart" has to be set to "yes", i. e. a
> plain "y" is not supposed to mean 'yes'.

consider it an enhancement then ?  i dont see a problem with making the values 
fuzzier.  personally, i make funcs like this accept any of the common strings 
such as true/false/0/1/y/n/yes/no.

> > i can update the API to take a 2nd arg (the default value), or we can
> > punt this patch.  it's in "next", so there's less pressure to get it
> > fixed immediately ...
> 
> I think the easiest way to fix this is to revert the commit.

it's in "next", so it could even just be dropped
-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/20101207/702559df/attachment.pgp 

[U-Boot] [PATCH] boot cmds: convert to getenv_yesno() with autostart

[Wolfgang Denk]

Dear Mike Frysinger,

In message <201012071657.42065.vapier@gentoo.org> you wrote:
>
> > In addition to the default values, there is the difference that the
> > documentation states that "autostart" has to be set to "yes", i. e. a
> > plain "y" is not supposed to mean 'yes'.
>
> consider it an enhancement then ?  i dont see a problem with making the values 
> fuzzier.  personally, i make funcs like this accept any of the common strings 
> such as true/false/0/1/y/n/yes/no.

That should be documented, then.  And it should be sensible not to
accept anything.  A typo like "yno" should IMHO _not_ be interpreted
as "yes".

> > I think the easiest way to fix this is to revert the commit.
>
> it's in "next", so it could even just be dropped

Yes, but then I have to rebase next...

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: wd at denx.de
"You shouldn't make my toaster angry." - Household security explained
in "Johnny Quest"

[U-Boot] [PATCH] boot cmds: convert to getenv_yesno() with autostart

[Mike Frysinger]

On Tuesday, December 07, 2010 17:19:38 Wolfgang Denk wrote:
> Mike Frysinger wrote:
> > > In addition to the default values, there is the difference that the
> > > documentation states that "autostart" has to be set to "yes", i. e. a
> > > plain "y" is not supposed to mean 'yes'.
> > 
> > consider it an enhancement then ?  i dont see a problem with making the
> > values fuzzier.  personally, i make funcs like this accept any of the
> > common strings such as true/false/0/1/y/n/yes/no.
> 
> That should be documented, then.  And it should be sensible not to
> accept anything.  A typo like "yno" should IMHO _not_ be interpreted
> as "yes".

good point ... i hadnt thought of that

> > > I think the easiest way to fix this is to revert the commit.
> > 
> > it's in "next", so it could even just be dropped
> 
> Yes, but then I have to rebase next...

imo, keeping the "next" branch clean at the cost of unstableness isnt a 
problem.  this seems to be the convention that Linux started and other people 
follow.  personally, when i use "next" from anywhere, the first thing i do is 
reset my local "next" branch to whatever the current upstream state.
-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/20101207/6bd7d39b/attachment.pgp 

[U-Boot] [PATCH] boot cmds: convert to getenv_yesno() with autostart

[Wolfgang Denk]

Dear Mike Frysinger,

In message <201012071657.42065.vapier@gentoo.org> you wrote:
>
> On Tuesday, December 07, 2010 16:51:41 Wolfgang Denk wrote:
> > Mike Frysinger wrote:
> > > hrm, after running this through our test bench, it seems the old code and
> > > new code are not functionality equivalent.  it boils down to default
> > > values when the env var is not set.  some places interpret this to mean
> > > "yes" while others expect "no".  getenv_yesno() takes the "no" default
> > > which doesnt always work.
> > 
> > In addition to the default values, there is the difference that the
> > documentation states that "autostart" has to be set to "yes", i. e. a
> > plain "y" is not supposed to mean 'yes'.
>
> consider it an enhancement then ?  i dont see a problem with making the values 
> fuzzier.  personally, i make funcs like this accept any of the common strings 
> such as true/false/0/1/y/n/yes/no.
>
> > > i can update the API to take a 2nd arg (the default value), or we can
> > > punt this patch.  it's in "next", so there's less pressure to get it
> > > fixed immediately ...
> > 
> > I think the easiest way to fix this is to revert the commit.
>
> it's in "next", so it could even just be dropped

As it was left unfixed until now I had no other choice but to revert
this commit.

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: wd at denx.de
The universe does not have laws - it has habits, and  habits  can  be
broken.

[U-Boot] [PATCH] boot cmds: convert to getenv_yesno() with autostart

[Mike Frysinger]

On Tuesday, January 11, 2011 15:04:37 Wolfgang Denk wrote:
> Mike Frysinger wrote:
> > On Tuesday, December 07, 2010 16:51:41 Wolfgang Denk wrote:
> > > Mike Frysinger wrote:
> > > > hrm, after running this through our test bench, it seems the old code
> > > > and new code are not functionality equivalent.  it boils down to
> > > > default values when the env var is not set.  some places interpret
> > > > this to mean "yes" while others expect "no".  getenv_yesno() takes
> > > > the "no" default which doesnt always work.
> > > 
> > > In addition to the default values, there is the difference that the
> > > documentation states that "autostart" has to be set to "yes", i. e. a
> > > plain "y" is not supposed to mean 'yes'.
> > 
> > consider it an enhancement then ?  i dont see a problem with making the
> > values fuzzier.  personally, i make funcs like this accept any of the
> > common strings such as true/false/0/1/y/n/yes/no.
> > 
> > > > i can update the API to take a 2nd arg (the default value), or we can
> > > > punt this patch.  it's in "next", so there's less pressure to get it
> > > > fixed immediately ...
> > > 
> > > I think the easiest way to fix this is to revert the commit.
> > 
> > it's in "next", so it could even just be dropped
> 
> As it was left unfixed until now I had no other choice but to revert
> this commit.

i explicitly asked about fixing it and you explicitly said you just wanted to 
revert.  so i obviously didnt work on it.
-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/20110111/4be0b3a3/attachment.pgp