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

login
register
mail settings
Submitter Jehan Bing
Date June 9, 2009, 11:04 p.m.
Message ID <h0mppp$7v2$1@ger.gmane.org>
Download mbox | patch
Permalink /patch/28351/
State New
Headers show

Comments

Jehan Bing - June 9, 2009, 11:04 p.m.
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>
Artem Bityutskiy - June 10, 2009, 4:03 p.m.
On Tue, 2009-06-09 at 16:04 -0700, Jehan Bing wrote:
> -		readlen = meminfo.writesize;
>  
> -		if (ifd != STDIN_FILENO) {
> -			int tinycnt = 0;
> -
> -			if (pad && (imglen < readlen))
> -			{
> -				readlen = imglen;
> -				erase_buffer(writebuf + readlen, meminfo.writesize - readlen);
> -			}
> +		{
> +			readlen = meminfo.writesize;
>  

Err, why do you need these spare { } ?

> -			/* Read Page Data from input file */
> -			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;

Err, is it normal C do do stuff like
{
	readlen = meminfo.writesize;
	int tinycnt += cnt;

? I think this is C++.
Artem Bityutskiy - June 10, 2009, 4:05 p.m.
On Wed, 2009-06-10 at 19:03 +0300, Artem Bityutskiy wrote:
> Err, is it normal C do do stuff like
> {
> 	readlen = meminfo.writesize;
> 	int tinycnt += cnt;

Sorry, I meant
{
	readlen = meminfo.writesize;
	int tinycnt = 0;
Jehan Bing - June 10, 2009, 5:11 p.m.
Artem Bityutskiy wrote:
> On Tue, 2009-06-09 at 16:04 -0700, Jehan Bing wrote:
>   
>> -		readlen = meminfo.writesize;
>>  
>> -		if (ifd != STDIN_FILENO) {
>> -			int tinycnt = 0;
>> -
>> -			if (pad && (imglen < readlen))
>> -			{
>> -				readlen = imglen;
>> -				erase_buffer(writebuf + readlen, meminfo.writesize - readlen);
>> -			}
>> +		{
>> +			readlen = meminfo.writesize;
>>  
>>     
>
> Err, why do you need these spare { } ?
>   

Right, I wanted to comment on that but forgot by the time I started the 
email.
My idea was to make the patches clearer. Patch 3/3 puts that code inside 
a condition. So to avoid extra changes just because of the indentation, 
I added the extra { } in this patch instead since I was already heavily 
modifying it anyway.
Do you prefer me to remove them?


>> -			/* Read Page Data from input file */
>> -			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;
>>     
>
> Err, is it normal C do do stuff like
> {
> 	readlen = meminfo.writesize;
> 	int tinycnt += cnt;
>
> ? I think this is C++.
>   

gcc didn't complain. Easy to fix.
Jamie Lokier - June 10, 2009, 5:23 p.m.
Artem Bityutskiy wrote:
> On Wed, 2009-06-10 at 19:03 +0300, Artem Bityutskiy wrote:
> > Err, is it normal C do do stuff like
> > {
> > 	readlen = meminfo.writesize;
> > 	int tinycnt += cnt;
> 
> Sorry, I meant
> {
> 	readlen = meminfo.writesize;
> 	int tinycnt = 0;

It's C99, and GCC has accepted that syntax since GCC 3.0.

If you want to disable it, use -Werror=declaration-after-statement in
current GCC, or -pedantic (but that has other effects).

-- Jamie
Artem Bityutskiy - June 11, 2009, 7:32 a.m.
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!

Patch

--- a/nandwrite.c	2009-06-08 13:33:32.000000000 -0700
+++ b/nandwrite.c	2009-06-09 13:15:17.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,18 @@  int main(int argc, char * const argv[])
 
 		}
 
-		readlen = meminfo.writesize;
 
-		if (ifd != STDIN_FILENO) {
-			int tinycnt = 0;
-
-			if (pad && (imglen < readlen))
-			{
-				readlen = imglen;
-				erase_buffer(writebuf + readlen, meminfo.writesize - readlen);
-			}
+		{
+			readlen = meminfo.writesize;
 
-			/* Read Page Data from input file */
-			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) {
+			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 +494,30 @@  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 (pad && (tinycnt < readlen)) {
-				erase_buffer(writebuf + tinycnt, meminfo.writesize - tinycnt);
+			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);
+			}
+
+			/* No more bytes - we are done after writing the remaining bytes */
+			if ((ifd == STDIN_FILENO) && (cnt == 0)) {
+				imglen = 0;
 			}
 		}