[U-Boot] [RFC] Suppressing Diagnostic Output for 'fw_setenv'

[U-Boot] [RFC] Suppressing Diagnostic Output for 'fw_setenv'

[Grant Erickson]

The u-boot companion command line tool 'fw_setenv' emits verbose output
during a set operation of the form:

    Unlocking flash...
    Done
    Erasing old environment...
    Done
    Writing environment to /dev/mtd4...
    Done
    Locking ...
    Done

While this is nice for debugging and troubleshooting, it is a bit verbose in
a production environment. I propose two alternatives for suppressing this
output:

1) Compile-time: Change 'printf' to 'debug' in which case the above
   diagnostics will only be output when the tool is built with 'DEBUG'
   asserted.

2) Run-time: Change the implementation such that the invocation usage of
   'fw_setenv' is:

    Usage: fw_setenv [ -q ] name [ value ... ]

I'd favor (2); however, I welcome strong opinions one way or another before
I submit a patch.

Regards,

Grant

[U-Boot] [RFC] Suppressing Diagnostic Output for 'fw_setenv'

[Markus Klotzbücher]

Dear Grant,

On Fri, Aug 29, 2008 at 08:32:39PM -0700, Grant Erickson wrote:

> The u-boot companion command line tool 'fw_setenv' emits verbose output
> during a set operation of the form:
> 
>     Unlocking flash...
>     Done
>     Erasing old environment...
>     Done
>     Writing environment to /dev/mtd4...
>     Done
>     Locking ...
>     Done

I'm all with you.

> While this is nice for debugging and troubleshooting, it is a bit verbose in
> a production environment. I propose two alternatives for suppressing this
> output:
> 
> 1) Compile-time: Change 'printf' to 'debug' in which case the above
>    diagnostics will only be output when the tool is built with 'DEBUG'
>    asserted.
> 
> 2) Run-time: Change the implementation such that the invocation usage of
>    'fw_setenv' is:
> 
>     Usage: fw_setenv [ -q ] name [ value ... ]
> 
> I'd favor (2); however, I welcome strong opinions one way or another before
> I submit a patch.

I'd prefer (2) too, but I'd make it quiet by default:

Usage: fw_setenv [ -v ] name [ value ... ]

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: office at denx.de")

[U-Boot] [PATCH] Suppressing Diagnostic Output for 'fw_setenv'

[Grant Erickson]

Added support for a ``-v'' option for fw_setenv to explicitly enable
what was formerly default verbose diagnostic output when updating the
environment.

Signed-off-by: Grant Erickson <gerickson@nuovations.com>
---
 tools/env/fw_env.c      |   43 +++++++++++++++++++++++++++++--------------
 tools/env/fw_env_main.c |    4 +++-
 2 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/tools/env/fw_env_main.c b/tools/env/fw_env_main.c
index 7f631c4..c195de9 100644
--- a/tools/env/fw_env_main.c
+++ b/tools/env/fw_env_main.c
@@ -30,13 +30,15 @@
  *                "name", the ``name=value'' pairs of one or more
  *                environment variables "name", or the whole
  *                environment if no names are specified.
- *	fw_setenv name [ value ... ]
+ *	fw_setenv [ -v ] name [ value ... ]
  *		- If a name without any values is given, the variable
  *		  with this name is deleted from the environment;
  *		  otherwise, all "value" arguments are concatenated,
  *		  separated by single blank characters, and the
  *		  resulting string is assigned to the environment
  *		  variable "name"
+ *		- With the ``-v''option asserted, verbose progress is
+ *		  displayed as the environment is updated.
  */
 
 #include <stdio.h>
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index b8bca91..836a9b9 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -23,6 +23,7 @@
 
 #include <errno.h>
 #include <fcntl.h>
+#include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <stddef.h>
@@ -159,7 +160,7 @@ static char default_environment[] = {
 	"\0"			/* Termimate env_t data with 2 NULs */
 };
 
-static int flash_io (int mode);
+static int flash_io (int mode, bool v_flag);
 static char *envmatch (char * s1, char * s2);
 static int env_init (void);
 static int parse_config (void);
@@ -288,7 +289,7 @@ int fw_printenv (int argc, char *argv[])
  */
 int fw_setenv (int argc, char *argv[])
 {
-	int i, len;
+	int i, len, v_flag;
 	char *env, *nxt;
 	char *oldval = NULL;
 	char *name;
@@ -297,6 +298,16 @@ int fw_setenv (int argc, char *argv[])
 		return (EINVAL);
 	}
 
+	if (strcmp (argv[1], "-v") == 0) {
+		v_flag = true;
+		++argv;
+		--argc;
+		if (argc < 2)
+		    return (EINVAL);
+	} else {
+		v_flag = false;
+	}
+
 	if (env_init ())
 		return (errno);
 
@@ -386,7 +397,7 @@ int fw_setenv (int argc, char *argv[])
 	environment.crc = crc32 (0, (uint8_t*) environment.data, ENV_SIZE);
 
 	/* write environment back to flash */
-	if (flash_io (O_RDWR)) {
+	if (flash_io (O_RDWR, v_flag)) {
 		fprintf (stderr, "Error: can't write fw_env to flash\n");
 		return (-1);
 	}
@@ -394,11 +405,14 @@ int fw_setenv (int argc, char *argv[])
 	return (0);
 }
 
-static int flash_io (int mode)
+static int flash_io (int mode, bool v_flag)
 {
 	int fd, fdr, rc, otherdev, len, resid;
 	erase_info_t erase;
 	char *data = NULL;
+	FILE *diag = NULL;
+
+	diag = (v_flag ? stdout : fopen("/dev/null", "r+"));
 
 	if ((fd = open (DEVNAME (curdev), mode)) < 0) {
 		fprintf (stderr,
@@ -427,7 +441,7 @@ static int flash_io (int mode)
 			otherdev = curdev;
 			fdr = fd;
 		}
-		printf ("Unlocking flash...\n");
+		fprintf (diag, "Unlocking flash...\n");
 		erase.length = DEVESIZE (otherdev);
 		erase.start = DEVOFFSET (otherdev);
 		ioctl (fdr, MEMUNLOCK, &erase);
@@ -439,7 +453,7 @@ static int flash_io (int mode)
 			environment.flags = active_flag;
 		}
 
-		printf ("Done\n");
+		fprintf (diag, "Done\n");
 		resid = DEVESIZE (otherdev) - CFG_ENV_SIZE;
 		if (resid) {
 			if ((data = malloc (resid)) == NULL) {
@@ -465,7 +479,7 @@ static int flash_io (int mode)
 			}
 		}
 
-		printf ("Erasing old environment...\n");
+		fprintf (diag, "Erasing old environment...\n");
 
 		erase.length = DEVESIZE (otherdev);
 		erase.start = DEVOFFSET (otherdev);
@@ -476,9 +490,10 @@ static int flash_io (int mode)
 			return (-1);
 		}
 
-		printf ("Done\n");
+		fprintf (diag, "Done\n");
 
-		printf ("Writing environment to %s...\n", DEVNAME (otherdev));
+		fprintf (diag, "Writing environment to %s...\n",
+			 DEVNAME (otherdev));
 		if (lseek (fdr, DEVOFFSET (otherdev), SEEK_SET) == -1) {
 			fprintf (stderr,
 				"seek error on %s: %s\n",
@@ -522,8 +537,8 @@ static int flash_io (int mode)
 				return (-1);
 			}
 		}
-		printf ("Done\n");
-		printf ("Locking ...\n");
+		fprintf (diag, "Done\n");
+		fprintf (diag, "Locking ...\n");
 		erase.length = DEVESIZE (otherdev);
 		erase.start = DEVOFFSET (otherdev);
 		ioctl (fdr, MEMLOCK, &erase);
@@ -539,7 +554,7 @@ static int flash_io (int mode)
 				return (-1);
 			}
 		}
-		printf ("Done\n");
+		fprintf (diag, "Done\n");
 	} else {
 
 		if (lseek (fd, DEVOFFSET (curdev), SEEK_SET) == -1) {
@@ -614,7 +629,7 @@ static int env_init (void)
 	/* read environment from FLASH to local buffer */
 	environment.data = addr1;
 	curdev = 0;
-	if (flash_io (O_RDONLY)) {
+	if (flash_io (O_RDONLY, false)) {
 		return (errno);
 	}
 
@@ -638,8 +653,7 @@ static int env_init (void)
 		}
 		environment.data = addr2;
 
-		if (flash_io (O_RDONLY)) {
+		if (flash_io (O_RDONLY, false)) {
 			return (errno);
 		}
 
-- 
1.6.0

[U-Boot] [PATCH] Suppressing Diagnostic Output for 'fw_setenv'

[Markus Klotzbücher]

On Fri, Aug 29, 2008 at 11:52:41PM -0700, Grant Erickson wrote:
> Added support for a ``-v'' option for fw_setenv to explicitly enable
> what was formerly default verbose diagnostic output when updating the
> environment.
> 
> Signed-off-by: Grant Erickson <gerickson@nuovations.com>
Acked-by: Markus Klotzbuecher <mk@denx.de>

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: office at denx.de")

[U-Boot] [PATCH] Suppressing Diagnostic Output for 'fw_setenv'

[Wolfgang Denk]

Dear Grant Erickson,

In message <1220079161-18217-1-git-send-email-gerickson@nuovations.com> you wrote:
> Added support for a ``-v'' option for fw_setenv to explicitly enable
> what was formerly default verbose diagnostic output when updating the
> environment.
> 
> Signed-off-by: Grant Erickson <gerickson@nuovations.com>
> ---
>  tools/env/fw_env.c      |   43 +++++++++++++++++++++++++++++--------------
>  tools/env/fw_env_main.c |    4 +++-
>  2 files changed, 32 insertions(+), 15 deletions(-)

I will not apply this patch for now, as I expect that  Guennadi  will
address  this  issue  in  his  rewrite  of the fw_setenv command when
adding NAND support.

Please check again and  resubmit  (if  still  needed)  later  (affter
Guennadi's patch went in).

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
Make it right before you make it faster.

[U-Boot] [PATCH] Suppressing Diagnostic Output for 'fw_setenv'

[Guennadi Liakhovetski]

On Sun, 7 Sep 2008, Wolfgang Denk wrote:

> Dear Grant Erickson,
> 
> In message <1220079161-18217-1-git-send-email-gerickson@nuovations.com> you wrote:
> > Added support for a ``-v'' option for fw_setenv to explicitly enable
> > what was formerly default verbose diagnostic output when updating the
> > environment.
> > 
> > Signed-off-by: Grant Erickson <gerickson@nuovations.com>
> > ---
> >  tools/env/fw_env.c      |   43 +++++++++++++++++++++++++++++--------------
> >  tools/env/fw_env_main.c |    4 +++-
> >  2 files changed, 32 insertions(+), 15 deletions(-)
> 
> I will not apply this patch for now, as I expect that  Guennadi  will
> address  this  issue  in  his  rewrite  of the fw_setenv command when
> adding NAND support.
> 
> Please check again and  resubmit  (if  still  needed)  later  (affter
> Guennadi's patch went in).

In my last patch version (v2, submitted last Thursday, 4 Sep), I put all 
debugging output in "#ifdef DEBUG" blocks. So, if this patch is accepted 
in its current form, you will have to look at those ifdef's and see which 
of them you want to make dynamically selectable per '-v' switch.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

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: office at denx.de