| Message ID | 1287573475-6737-1-git-send-email-vapier@gentoo.org |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Wolfgang Denk |
| Headers | show |
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
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
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
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
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
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
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
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
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];
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(-)