[PATCH [mtd-utils] 1/3] nand{dump, test, write}: clean up --help handling

[PATCH [mtd-utils] 1/3] nand{dump, test, write}: clean up --help handling

[Mike Frysinger]

We should send the output to stdout when the user passes -h/--help
and then exit(0), but otherwise the output should go to stderr and
then exit(1).

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 nanddump.c  | 24 ++++++++++++------------
 nandtest.c  | 16 ++++++++++------
 nandwrite.c | 26 +++++++++++++-------------
 3 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/nanddump.c b/nanddump.c
index 4b3e14d..4ee7ed4 100644
--- a/nanddump.c
+++ b/nanddump.c
@@ -33,13 +33,13 @@
 #include "common.h"
 #include <libmtd.h>
 
-static void display_help(void)
+static void display_help(int status)
 {
-	printf(
+	fprintf(status == EXIT_SUCCESS ? stdout : stderr,
 "Usage: %s [OPTIONS] MTD-device\n"
 "Dumps the contents of a nand mtd partition.\n"
 "\n"
-"           --help               Display this help and exit\n"
+"-h         --help               Display this help and exit\n"
 "           --version            Output version information and exit\n"
 "           --bb=METHOD          Choose bad block handling method (see below).\n"
 "-a         --forcebinary        Force printing of binary data to tty\n"
@@ -58,7 +58,7 @@ static void display_help(void)
 "    dumpbad: dump flash data, including any bad blocks\n"
 "    skipbad: dump good data, completely skipping any bad blocks (default)\n",
 	PROGRAM_NAME);
-	exit(EXIT_SUCCESS);
+	exit(status);
 }
 
 static void display_version(void)
@@ -101,12 +101,12 @@ static void process_options(int argc, char * const argv[])
 
 	for (;;) {
 		int option_index = 0;
-		static const char *short_options = "s:f:l:opqnca";
+		static const char short_options[] = "hs:f:l:opqnca";
 		static const struct option long_options[] = {
-			{"help", no_argument, 0, 0},
 			{"version", no_argument, 0, 0},
 			{"bb", required_argument, 0, 0},
 			{"omitoob", no_argument, 0, 0},
+			{"help", no_argument, 0, 'h'},
 			{"forcebinary", no_argument, 0, 'a'},
 			{"canonicalprint", no_argument, 0, 'c'},
 			{"file", required_argument, 0, 'f'},
@@ -129,12 +129,9 @@ static void process_options(int argc, char * const argv[])
 			case 0:
 				switch (option_index) {
 					case 0:
-						display_help();
-						break;
-					case 1:
 						display_version();
 						break;
-					case 2:
+					case 1:
 						/* Handle --bb=METHOD */
 						if (!strcmp(optarg, "padbad"))
 							bb_method = padbad;
@@ -145,7 +142,7 @@ static void process_options(int argc, char * const argv[])
 						else
 							error++;
 						break;
-					case 3: /* --omitoob */
+					case 2: /* --omitoob */
 						if (oob_default) {
 							oob_default = false;
 							omitoob = true;
@@ -186,6 +183,9 @@ static void process_options(int argc, char * const argv[])
 			case 'n':
 				noecc = true;
 				break;
+			case 'h':
+				display_help(EXIT_SUCCESS);
+				break;
 			case '?':
 				error++;
 				break;
@@ -213,7 +213,7 @@ static void process_options(int argc, char * const argv[])
 	}
 
 	if ((argc - optind) != 1 || error)
-		display_help();
+		display_help(EXIT_FAILURE);
 
 	mtddev = argv[optind];
 }
diff --git a/nandtest.c b/nandtest.c
index 3437b57..1876bb2 100644
--- a/nandtest.c
+++ b/nandtest.c
@@ -16,9 +16,10 @@
 #include <asm/types.h>
 #include "mtd/mtd-user.h"
 
-void usage(void)
+void usage(int status)
 {
-	fprintf(stderr, "usage: %s [OPTIONS] <device>\n\n"
+	fprintf(status ? stderr : stdout,
+		"usage: %s [OPTIONS] <device>\n\n"
 		"  -h, --help           Display this help output\n"
 		"  -m, --markbad        Mark blocks bad if they appear so\n"
 		"  -s, --seed           Supply random seed\n"
@@ -27,7 +28,7 @@ void usage(void)
 		"  -l, --length         Length of flash to test\n"
 		"  -k, --keep           Restore existing contents after test\n",
 		PROGRAM_NAME);
-	exit(1);
+	exit(status);
 }
 
 struct mtd_info_user meminfo;
@@ -142,7 +143,7 @@ int main(int argc, char **argv)
 	seed = time(NULL);
 
 	for (;;) {
-		static const char *short_options="hkl:mo:p:s:";
+		static const char short_options[] = "hkl:mo:p:s:";
 		static const struct option long_options[] = {
 			{ "help", no_argument, 0, 'h' },
 			{ "markbad", no_argument, 0, 'm' },
@@ -160,8 +161,11 @@ int main(int argc, char **argv)
 
 		switch (c) {
 		case 'h':
+			usage(0);
+			break;
+
 		case '?':
-			usage();
+			usage(1);
 			break;
 
 		case 'm':
@@ -191,7 +195,7 @@ int main(int argc, char **argv)
 		}
 	}
 	if (argc - optind != 1)
-		usage();
+		usage(1);
 
 	fd = open(argv[optind], O_RDWR);
 	if (fd < 0) {
diff --git a/nandwrite.c b/nandwrite.c
index a6b6581..edf9f83 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -42,9 +42,9 @@
 #include "common.h"
 #include <libmtd.h>
 
-static void display_help(void)
+static void display_help(int status)
 {
-	printf(
+	fprintf(status == EXIT_SUCCESS ? stdout : stderr,
 "Usage: nandwrite [OPTION] MTD_DEVICE [INPUTFILE|-]\n"
 "Writes to the specified MTD device.\n"
 "\n"
@@ -58,10 +58,10 @@ static void display_help(void)
 "  -p, --pad               Pad to page size\n"
 "  -b, --blockalign=1|2|4  Set multiple of eraseblocks to align to\n"
 "  -q, --quiet             Don't display progress messages\n"
-"      --help              Display this help and exit\n"
+"  -h, --help              Display this help and exit\n"
 "      --version           Output version information and exit\n"
 	);
-	exit(EXIT_SUCCESS);
+	exit(status);
 }
 
 static void display_version(void)
@@ -99,10 +99,10 @@ static void process_options(int argc, char * const argv[])
 
 	for (;;) {
 		int option_index = 0;
-		static const char *short_options = "b:mnNoOpqs:a";
+		static const char short_options[] = "hb:mnNoOpqs:a";
 		static const struct option long_options[] = {
-			{"help", no_argument, 0, 0},
 			{"version", no_argument, 0, 0},
+			{"help", no_argument, 0, 'h'},
 			{"blockalign", required_argument, 0, 'b'},
 			{"markbad", no_argument, 0, 'm'},
 			{"noecc", no_argument, 0, 'n'},
@@ -124,12 +124,9 @@ static void process_options(int argc, char * const argv[])
 		switch (c) {
 		case 0:
 			switch (option_index) {
-				case 0:
-					display_help();
-					break;
-				case 1:
-					display_version();
-					break;
+			case 0: /* --version */
+				display_version();
+				break;
 			}
 			break;
 		case 'q':
@@ -163,6 +160,9 @@ static void process_options(int argc, char * const argv[])
 		case 'a':
 			autoplace = true;
 			break;
+		case 'h':
+			display_help(EXIT_SUCCESS);
+			break;
 		case '?':
 			error++;
 			break;
@@ -192,7 +192,7 @@ static void process_options(int argc, char * const argv[])
 	 */
 
 	if (argc < 1 || argc > 2 || error)
-		display_help();
+		display_help(EXIT_FAILURE);
 
 	mtd_device = argv[0];
 
-- 
1.8.2.1

[PATCH [mtd-utils] 2/3] nandwrite: clean up length types

[Mike Frysinger]

We use 'int' in many places to represent offsets/sizes.  That obviously
does not play well with larger NAND devices on 32bit systems.  Instead,
use the right type as needed:
 - long long to represent the length of the image
 - use fstat() rather than lseek();lseek(); to get the length of the image
 - use size_t/ssize_t when working with read()
 - tweak the printf formats as needed

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 nandwrite.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/nandwrite.c b/nandwrite.c
index edf9f83..14414b6 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -218,10 +218,10 @@ static void erase_buffer(void *buffer, size_t size)
  */
 int main(int argc, char * const argv[])
 {
-	int cnt = 0;
 	int fd = -1;
 	int ifd = -1;
-	int imglen = 0, pagelen;
+	int pagelen;
+	long long imglen = 0;
 	bool baderaseblock = false;
 	long long blockstart = -1;
 	struct mtd_dev_info mtd;
@@ -313,8 +313,12 @@ int main(int argc, char * const argv[])
 	if (ifd == STDIN_FILENO) {
 	    imglen = pagelen;
 	} else {
-	    imglen = lseek(ifd, 0, SEEK_END);
-	    lseek(ifd, 0, SEEK_SET);
+		struct stat st;
+		if (fstat(ifd, &st)) {
+			sys_errmsg("unable to stat input image");
+			goto closeall;
+		}
+	    imglen = st.st_size;
 	}
 
 	/* Check, if file is page-aligned */
@@ -326,7 +330,7 @@ int main(int argc, char * const argv[])
 
 	/* Check, if length fits into device */
 	if ((imglen / pagelen) * mtd.min_io_size > mtd.size - mtdoffset) {
-		fprintf(stderr, "Image %d bytes, NAND page %d bytes, OOB area %d"
+		fprintf(stderr, "Image %lld bytes, NAND page %d bytes, OOB area %d"
 				" bytes, device size %lld bytes\n",
 				imglen, pagelen, mtd.oob_size, mtd.size);
 		sys_errmsg("Input file does not fit into device");
@@ -412,9 +416,10 @@ int main(int argc, char * const argv[])
 
 		/* Read more data from the input if there isn't enough in the buffer */
 		if (writebuf + mtd.min_io_size > filebuf + filebuf_len) {
-			int readlen = mtd.min_io_size;
-			int alreadyread = (filebuf + filebuf_len) - writebuf;
-			int tinycnt = alreadyread;
+			size_t readlen = mtd.min_io_size;
+			size_t alreadyread = (filebuf + filebuf_len) - writebuf;
+			size_t tinycnt = alreadyread;
+			ssize_t cnt = 0;
 
 			while (tinycnt < readlen) {
 				cnt = read(ifd, writebuf + tinycnt, readlen - tinycnt);
@@ -444,7 +449,7 @@ int main(int argc, char * const argv[])
 			if (tinycnt < readlen) {
 				if (!pad) {
 					fprintf(stderr, "Unexpected EOF. Expecting at least "
-							"%d more bytes. Use the padding option.\n",
+							"%zu more bytes. Use the padding option.\n",
 							readlen - tinycnt);
 					goto closeall;
 				}
@@ -465,9 +470,10 @@ int main(int argc, char * const argv[])
 
 			/* Read more data for the OOB from the input if there isn't enough in the buffer */
 			if (oobbuf + mtd.oob_size > filebuf + filebuf_len) {
-				int readlen = mtd.oob_size;
-				int alreadyread = (filebuf + filebuf_len) - oobbuf;
-				int tinycnt = alreadyread;
+				size_t readlen = mtd.oob_size;
+				size_t alreadyread = (filebuf + filebuf_len) - oobbuf;
+				size_t tinycnt = alreadyread;
+				ssize_t cnt;
 
 				while (tinycnt < readlen) {
 					cnt = read(ifd, oobbuf + tinycnt, readlen - tinycnt);
@@ -482,7 +488,7 @@ int main(int argc, char * const argv[])
 
 				if (tinycnt < readlen) {
 					fprintf(stderr, "Unexpected EOF. Expecting at least "
-							"%d more bytes for OOB\n", readlen - tinycnt);
+							"%zu more bytes for OOB\n", readlen - tinycnt);
 					goto closeall;
 				}
 
@@ -505,7 +511,7 @@ int main(int argc, char * const argv[])
 				writeoob ? mtd.oob_size : 0,
 				write_mode);
 		if (ret) {
-			int i;
+			long long i;
 			if (errno != EIO) {
 				sys_errmsg("%s: MTD write failure", mtd_device);
 				goto closeall;
@@ -520,9 +526,8 @@ int main(int argc, char * const argv[])
 				if (mtd_erase(mtd_desc, &mtd, fd, i / mtd.eb_size)) {
 					int errno_tmp = errno;
 					sys_errmsg("%s: MTD Erase failure", mtd_device);
-					if (errno_tmp != EIO) {
+					if (errno_tmp != EIO)
 						goto closeall;
-					}
 				}
 			}
 
-- 
1.8.2.1

[PATCH [mtd-utils] 3/3] nandwrite: add --input-{skip,size} options

[Mike Frysinger]

If you have a file image and want to copy sub-portions out and into
NAND, there's no easy way to do that.  You can use dd to extract it
to a temp file, or pipe it to nandwrite 1 page at a time.  Both suck.

Add two new flags to explicitly set the size and offset of the input
file.  Seeking stdin isn't currently supported as I'm not sure it's
necessary.  It wouldn't be hard to add though...

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 nandwrite.c | 54 ++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 20 deletions(-)

diff --git a/nandwrite.c b/nandwrite.c
index 14414b6..70579a3 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -52,11 +52,13 @@ static void display_help(int status)
 "  -m, --markbad           Mark blocks bad if write fails\n"
 "  -n, --noecc             Write without ecc\n"
 "  -N, --noskipbad         Write without bad block skipping\n"
-"  -o, --oob               Image contains oob data\n"
-"  -O, --onlyoob           Image contains oob data and only write the oob part\n"
-"  -s addr, --start=addr   Set start address (default is 0)\n"
-"  -p, --pad               Pad to page size\n"
+"  -o, --oob               Input contains oob data\n"
+"  -O, --onlyoob           Input contains oob data and only write the oob part\n"
+"  -s addr, --start=addr   Set output start address (default is 0)\n"
+"  -p, --pad               Pad writes to page size\n"
 "  -b, --blockalign=1|2|4  Set multiple of eraseblocks to align to\n"
+"      --input-skip=length Skip |length| bytes of the input file\n"
+"      --input-size=length Only read |length| bytes of the input file\n"
 "  -q, --quiet             Don't display progress messages\n"
 "  -h, --help              Display this help and exit\n"
 "      --version           Output version information and exit\n"
@@ -83,6 +85,8 @@ static void display_version(void)
 static const char	*standard_input = "-";
 static const char	*mtd_device, *img;
 static long long	mtdoffset = 0;
+static long long	inputskip = 0;
+static long long	inputsize = 0;
 static bool		quiet = false;
 static bool		writeoob = false;
 static bool		onlyoob = false;
@@ -101,7 +105,10 @@ static void process_options(int argc, char * const argv[])
 		int option_index = 0;
 		static const char short_options[] = "hb:mnNoOpqs:a";
 		static const struct option long_options[] = {
+			/* Order of these args with val==0 matters; see option_index. */
 			{"version", no_argument, 0, 0},
+			{"input-skip", required_argument, 0, 0},
+			{"input-size", required_argument, 0, 0},
 			{"help", no_argument, 0, 'h'},
 			{"blockalign", required_argument, 0, 'b'},
 			{"markbad", no_argument, 0, 'm'},
@@ -127,6 +134,12 @@ static void process_options(int argc, char * const argv[])
 			case 0: /* --version */
 				display_version();
 				break;
+			case 1: /* --input-skip */
+				inputskip = simple_strtoll(optarg, &error);
+				break;
+			case 2: /* --input-size */
+				inputsize = simple_strtoll(optarg, &error);
+				break;
 			}
 			break;
 		case 'q':
@@ -299,26 +312,27 @@ int main(int argc, char * const argv[])
 
 	pagelen = mtd.min_io_size + ((writeoob) ? mtd.oob_size : 0);
 
-	/*
-	 * For the standard input case, the input size is merely an
-	 * invariant placeholder and is set to the write page
-	 * size. Otherwise, just use the input file size.
-	 *
-	 * TODO: Add support for the -l,--length=length option (see
-	 * previous discussion by Tommi Airikka <tommi.airikka@ericsson.com> at
-	 * <http://lists.infradead.org/pipermail/linux-mtd/2008-September/
-	 * 022913.html>
-	 */
-
 	if (ifd == STDIN_FILENO) {
-	    imglen = pagelen;
+		imglen = inputsize ? : pagelen;
+		if (inputskip) {
+			errmsg("seeking stdin does not work");
+			goto closeall;
+		}
 	} else {
-		struct stat st;
-		if (fstat(ifd, &st)) {
-			sys_errmsg("unable to stat input image");
+		if (!inputsize) {
+			struct stat st;
+			if (fstat(ifd, &st)) {
+				sys_errmsg("unable to stat input image");
+				goto closeall;
+			}
+			imglen = st.st_size;
+		} else
+			imglen = inputsize;
+
+		if (inputskip && lseek(ifd, inputskip, SEEK_CUR) == -1) {
+			sys_errmsg("lseek input by %lld failed", inputskip);
 			goto closeall;
 		}
-	    imglen = st.st_size;
 	}
 
 	/* Check, if file is page-aligned */
-- 
1.8.2.1

RE: [PATCH [mtd-utils] 3/3] nandwrite: add --input-{skip,size} options

[Gupta, Pekon]

>  	pagelen = mtd.min_io_size + ((writeoob) ? mtd.oob_size : 0);
> 

> +		imglen = inputsize ? : pagelen;
> +		if (inputskip) {
> +			errmsg("seeking stdin does not work");
> +			goto closeall;
> +		}
>  	} else {
> -		struct stat st;
> -		if (fstat(ifd, &st)) {
> -			sys_errmsg("unable to stat input image");
> +		if (!inputsize) {
> +			struct stat st;
> +			if (fstat(ifd, &st)) {
> +				sys_errmsg("unable to stat input
> image");
> +				goto closeall;
> +			}
> +			imglen = st.st_size;
> +		} else
> +			imglen = inputsize;
> +

[Pekon]: It would be good to check that both 'inputsize' & 'inputskip' 
are multiples of:
-  mtd->writesize : if not using '-o' option, OR
- (mtd->writesize + mtd->oobsize): is using '-o' option.
Or Simply, both arguments should be aligned to 'pagelen' boundary.


with regards, pekon

Re: [PATCH [mtd-utils] 3/3] nandwrite: add --input-{skip, size} options

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1179 bytes --]

On Thursday 09 May 2013 06:42:01 Gupta, Pekon wrote:
> >  	pagelen = mtd.min_io_size + ((writeoob) ? mtd.oob_size : 0);
> > +		imglen = inputsize ? : pagelen;
> > +		if (inputskip) {
> > +			errmsg("seeking stdin does not work");
> > +			goto closeall;
> > +		}
> >  	} else {
> > -		struct stat st;
> > -		if (fstat(ifd, &st)) {
> > -			sys_errmsg("unable to stat input image");
> > +		if (!inputsize) {
> > +			struct stat st;
> > +			if (fstat(ifd, &st)) {
> > +				sys_errmsg("unable to stat input
> > image");
> > +				goto closeall;
> > +			}
> > +			imglen = st.st_size;
> > +		} else
> > +			imglen = inputsize;
> 
> [Pekon]: It would be good to check that both 'inputsize' & 'inputskip'
> are multiples of:
> -  mtd->writesize : if not using '-o' option, OR
> - (mtd->writesize + mtd->oobsize): is using '-o' option.
> Or Simply, both arguments should be aligned to 'pagelen' boundary.

mmm, i don't think so.  the input alignment (inputskip) is entirely irrelevant 
to the output.

as for inputsize, we already do that check.  if you read the code further 
(like 5 lines below the diff here), you'll see the --pad flag handling.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH [mtd-utils] 3/3] nandwrite: add --input-{skip,size} options

[Gupta, Pekon]

 
> On Thursday 09 May 2013 06:42:01 Gupta, Pekon wrote:
> > >  	pagelen = mtd.min_io_size + ((writeoob) ? mtd.oob_size : 0);
> > > +		imglen = inputsize ? : pagelen;
> > > +		if (inputskip) {
> > > +			errmsg("seeking stdin does not work");
> > > +			goto closeall;
> > > +		}
> > >  	} else {
> > > -		struct stat st;
> > > -		if (fstat(ifd, &st)) {
> > > -			sys_errmsg("unable to stat input image");
> > > +		if (!inputsize) {
> > > +			struct stat st;
> > > +			if (fstat(ifd, &st)) {
> > > +				sys_errmsg("unable to stat input
> > > image");
> > > +				goto closeall;
> > > +			}
> > > +			imglen = st.st_size;
> > > +		} else
> > > +			imglen = inputsize;
> >
> > [Pekon]: It would be good to check that both 'inputsize' & 'inputskip'
> > are multiples of:
> > -  mtd->writesize : if not using '-o' option, OR
> > - (mtd->writesize + mtd->oobsize): is using '-o' option.
> > Or Simply, both arguments should be aligned to 'pagelen' boundary.
> 
> mmm, i don't think so.  the input alignment (inputskip) is entirely
> irrelevant
> to the output.
> 
[Pekon]: I'm coming from the back-ground that most users 'dd' or 
'nanddump' to generate write-data for 'nandwrite'. 
Thus the write-data would be mostly 'pagelen' aligned.
Example:
If the write-data contains OOB data, and user forgets to take that 
into account while calculating 'inputskip' offset, then everything 
would be wrongly shifted by the offset.
So you need not flag an Error, but a warning may do, so user is 
hinted at his mistake.


> as for inputsize, we already do that check.  if you read the code further
> (like 5 lines below the diff here), you'll see the --pad flag handling.
> 
[Pekon]: Ok.. yes, its there..

> -mike

with regards, pekon

Re: [PATCH [mtd-utils] 1/3] nand{dump, test, write}: clean up --help handling

[Artem Bityutskiy]

On Wed, 2013-05-08 at 19:02 -0400, Mike Frysinger wrote:
> We should send the output to stdout when the user passes -h/--help
> and then exit(0), but otherwise the output should go to stderr and
> then exit(1).
> 
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

Pushed all 3 to mtd-utils.git, thanks!

I guess it is time to make an mtd-utils release...

-- 
Best Regards,
Artem Bityutskiy