diff mbox

[1/3,v2,MTD-UTILS] Unified reading from standard input and from file

Message ID 4A31424D.802@orb.com
State Accepted
Commit b0eab860c9b68b74d108836041346544d925fc9f
Headers show

Commit Message

Jehan Bing June 11, 2009, 5:43 p.m. UTC 
Artem Bityutskiy wrote:
> On Tue, 2009-06-09 at 16:04 -0700, Jehan Bing wrote:
>   
>> Use same code path for reading data (not the OOB) from either the 
>> standard input or a regular file.
>>
>> Signed-off-by: Jehan Bing <jehan@orb.com>
>>     
>
> The patches look OK to me, but I do not have time to review them very
> really well. So would it please be possible to describe how you tested
> them to convince me they are ok? Then I'd push them to mtd-utils.git
> tree. Did you test writing with/without oob, from file/stdin, etc?
>
> Thanks

For the tests, I used the nandsim driver and 
"eraseall-nandwrite-nanddump-md5sum". I did  a first one with the 
official nandwrite from Ubuntu as a reference, then tried different 
options with my changes.

And here the same patch with a couple of blank lines removed and a 
comment moved. I forgot the regenerate it with the others after cleaning 
up the code. The third patch won't apply without it.


Signed-off-by: Jehan Bing <jehan@orb.com>

Comments

Artem Bityutskiy June 12, 2009, 5:30 a.m. UTC | #1 
On Thu, 2009-06-11 at 10:43 -0700, Jehan Bing wrote:
> > The patches look OK to me, but I do not have time to review them very
> > really well. So would it please be possible to describe how you tested
> > them to convince me they are ok? Then I'd push them to mtd-utils.git
> > tree. Did you test writing with/without oob, from file/stdin, etc?
> >
> > Thanks
> 
> For the tests, I used the nandsim driver and 
> "eraseall-nandwrite-nanddump-md5sum". I did  a first one with the 
> official nandwrite from Ubuntu as a reference, then tried different 
> options with my changes.
> 
> And here the same patch with a couple of blank lines removed and a 
> comment moved. I forgot the regenerate it with the others after cleaning 
> up the code. The third patch won't apply without it.

Sorry, I still feel a little bit unsure. Could you please test it
like this:

1. Compile nandwrite1 which excludes these 4 changes;
2. Compile nandwrite2 which includes these 4 changes;
3. Make sure things which work with nandwrite1 also work with
   nandwrite2. Namely:
   a. writing an image
   b. writing an image from stdin
   c. writing an image with oob
   d. writing an image with oob with stdin
4. Then test how nandwrite2 reacts on badblocks - use the 'weakblocks'
   parameter of nandsim for this (see modinfo nandsim).
5. Test now nandwrite2 reacts on write failures - use 'weakpages'
   nandsim argument

Could this please be done to make sure nandwrite does not have
regressions but only has improvement? Sorry for being PITA.

Thanks!
diff mbox

Patch

--- mtd-utils.orig/nandwrite.c	2009-06-08 13:33:32.000000000 -0700
+++ mtd-utils.unified/nandwrite.c	2009-06-09 13:20:56.000000000 -0700
@@ -260,7 +260,6 @@  int main(int argc, char * const argv[])
 	int ret, readlen;
 	int oobinfochanged = 0;
 	struct nand_oobinfo old_oobinfo;
-	int readcnt = 0;
 	bool failed = true;
 
 	process_options(argc, argv);
@@ -476,37 +475,17 @@  int main(int argc, char * const argv[])
 
 		}
 
-		readlen = meminfo.writesize;
+		{
+			readlen = meminfo.writesize;
 
-		if (ifd != STDIN_FILENO) {
 			int tinycnt = 0;
 
-			if (pad && (imglen < readlen))
-			{
-				readlen = imglen;
-				erase_buffer(writebuf + readlen, meminfo.writesize - readlen);
-			}
-
-			/* Read Page Data from input file */
-			while(tinycnt < readlen) {
+			while (tinycnt < readlen) {
 				cnt = read(ifd, writebuf + tinycnt, readlen - tinycnt);
 				if (cnt == 0) { // EOF
 					break;
 				} else if (cnt < 0) {
-					perror ("File I/O error on input file");
-					goto closeall;
-				}
-				tinycnt += cnt;
-			}
-		} else {
-			int tinycnt = 0;
-
-			while(tinycnt < readlen) {
-				cnt = read(ifd, writebuf + tinycnt, readlen - tinycnt);
-				if (cnt == 0) { // EOF
-					break;
-				} else if (cnt < 0) {
-					perror ("File I/O error on stdin");
+					perror ("File I/O error on input");
 					goto closeall;
 				}
 				tinycnt += cnt;
@@ -514,18 +493,29 @@  int main(int argc, char * const argv[])
 
 			/* No padding needed - we are done */
 			if (tinycnt == 0) {
-				imglen = 0;
+				// For standard input, set the imglen to 0 to signal
+				// the end of the "file". For non standard input, leave
+				// it as-is to detect an early EOF
+				if (ifd == STDIN_FILENO) {
+					imglen = 0;
+				}
 				break;
 			}
 
-			/* No more bytes - we are done after writing the remaining bytes */
-			if (cnt == 0) {
-				imglen = 0;
+			/* Padding */
+			if (tinycnt < readlen) {
+				if (!pad) {
+					fprintf(stderr, "Unexpected EOF. Expecting at least "
+							"%d more bytes. Use the padding option.\n",
+							readlen - tinycnt);
+					goto closeall;
+				}
+				erase_buffer(writebuf + tinycnt, readlen - tinycnt);
 			}
 
-			/* Padding */
-			if (pad && (tinycnt < readlen)) {
-				erase_buffer(writebuf + tinycnt, meminfo.writesize - tinycnt);
+			if ((ifd == STDIN_FILENO) && (cnt == 0)) {
+				/* No more bytes - we are done after writing the remaining bytes */
+				imglen = 0;
 			}
 		}