[PATCH mtd-utils] nanddump: check write function result for errors

[PATCH mtd-utils] nanddump: check write function result for errors

[Rafał Miłecki]

Errors may happen, it's e.g. easy on embedded devices to run out of space
when dumping big partitions. This patch adds a helper function for
writing. It deals with partial writes and just returns 0 on success or
error number.

The old code didn't check for errors at all which could result in
incomplete dumps without exiting with an error.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 nand-utils/nanddump.c | 50 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 6 deletions(-)

diff --git a/nand-utils/nanddump.c b/nand-utils/nanddump.c
index 4ee7ed4..0bbc97f 100644
--- a/nand-utils/nanddump.c
+++ b/nand-utils/nanddump.c
@@ -293,6 +293,25 @@ nil:
 	linebuf[lx++] = '\0';
 }
 
+/**
+ * ofd_write - writes whole buffer to the file associated with a descriptor
+ *
+ * On failure an error (negative number) is returned. Otherwise 0 is returned.
+ */
+static int ofd_write(int ofd, const unsigned char *buf, size_t nbyte)
+{
+	ssize_t bytes;
+
+	while (nbyte) {
+		bytes = write(ofd, buf, nbyte);
+		if (bytes < 0)
+			return -errno;
+		buf += bytes;
+		nbyte -= bytes;
+	}
+
+	return 0;
+}
 
 /*
  * Main program
@@ -309,6 +328,7 @@ int main(int argc, char * const argv[])
 	bool eccstats = false;
 	unsigned char *readbuf = NULL, *oobbuf = NULL;
 	libmtd_t mtd_desc;
+	int err;
 
 	process_options(argc, argv);
 
@@ -443,10 +463,19 @@ int main(int argc, char * const argv[])
 			for (i = 0; i < bs; i += PRETTY_ROW_SIZE) {
 				pretty_dump_to_buffer(readbuf + i, PRETTY_ROW_SIZE,
 						pretty_buf, PRETTY_BUF_LEN, true, canonical, ofs + i);
-				write(ofd, pretty_buf, strlen(pretty_buf));
+				err = write(ofd, pretty_buf, strlen(pretty_buf));
+				if (err) {
+					errmsg("ofd_write: %s\n", strerror(-err));
+					goto closeall;
+				}
 			}
-		} else
-			write(ofd, readbuf, bs);
+		} else {
+			err = ofd_write(ofd, readbuf, bs);
+			if (err) {
+				errmsg("ofd_write: %d %s\n", err, strerror(-err));
+				goto closeall;
+			}
+		}
 
 		if (omitoob)
 			continue;
@@ -466,10 +495,19 @@ int main(int argc, char * const argv[])
 			for (i = 0; i < mtd.oob_size; i += PRETTY_ROW_SIZE) {
 				pretty_dump_to_buffer(oobbuf + i, mtd.oob_size - i,
 						pretty_buf, PRETTY_BUF_LEN, false, canonical, 0);
-				write(ofd, pretty_buf, strlen(pretty_buf));
+				err = write(ofd, pretty_buf, strlen(pretty_buf));
+				if (err) {
+					errmsg("ofd_write: %s\n", strerror(-err));
+					goto closeall;
+				}
 			}
-		} else
-			write(ofd, oobbuf, mtd.oob_size);
+		} else {
+			err = ofd_write(ofd, oobbuf, mtd.oob_size);
+			if (err) {
+				errmsg("ofd_write: %d %s\n", err, strerror(-err));
+				goto closeall;
+			}
+		}
 	}
 
 	/* Close the output file and MTD device, free memory */
-- 
1.8.4.5

Re: [PATCH mtd-utils] nanddump: check write function result for errors

[Richard Weinberger]

On Mon, Jul 18, 2016 at 11:18 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
> Errors may happen, it's e.g. easy on embedded devices to run out of space
> when dumping big partitions. This patch adds a helper function for
> writing. It deals with partial writes and just returns 0 on success or
> error number.
>
> The old code didn't check for errors at all which could result in
> incomplete dumps without exiting with an error.

Is this patch different from my version, does it fix more?
http://lists.infradead.org/pipermail/linux-mtd/2016-April/067234.html

Brian, if this is fine with you I'd apply one of these patches.

-- 
Thanks,
//richard

Re: [PATCH mtd-utils] nanddump: check write function result for errors

[Rafał Miłecki]

On 18 July 2016 at 13:33, Richard Weinberger
<richard.weinberger@gmail.com> wrote:
> On Mon, Jul 18, 2016 at 11:18 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
>> Errors may happen, it's e.g. easy on embedded devices to run out of space
>> when dumping big partitions. This patch adds a helper function for
>> writing. It deals with partial writes and just returns 0 on success or
>> error number.
>>
>> The old code didn't check for errors at all which could result in
>> incomplete dumps without exiting with an error.
>
> Is this patch different from my version, does it fix more?
> http://lists.infradead.org/pipermail/linux-mtd/2016-April/067234.html
>
> Brian, if this is fine with you I'd apply one of these patches.

I wasn't aware of your patch, having it accepted would save me some
time for sure.

Two minor advantages I see in my version:
1) It handles partial writes, just retries if only some part of buffer
has been written
2) It displays error number and string which may provide some extra
hint to the user

In my case (when I was running out of space), the successful last
write was partial because there wasn't enough space for the whole
buffer. I needed to retry a write to get a real error code.

-- 
Rafał

Re: [PATCH mtd-utils] nanddump: check write function result for errors

[Richard Weinberger]

Rafał,

Am 18.07.2016 um 13:59 schrieb Rafał Miłecki:
> On 18 July 2016 at 13:33, Richard Weinberger
> <richard.weinberger@gmail.com> wrote:
>> On Mon, Jul 18, 2016 at 11:18 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
>>> Errors may happen, it's e.g. easy on embedded devices to run out of space
>>> when dumping big partitions. This patch adds a helper function for
>>> writing. It deals with partial writes and just returns 0 on success or
>>> error number.
>>>
>>> The old code didn't check for errors at all which could result in
>>> incomplete dumps without exiting with an error.
>>
>> Is this patch different from my version, does it fix more?
>> http://lists.infradead.org/pipermail/linux-mtd/2016-April/067234.html
>>
>> Brian, if this is fine with you I'd apply one of these patches.
> 
> I wasn't aware of your patch, having it accepted would save me some
> time for sure.
> 
> Two minor advantages I see in my version:
> 1) It handles partial writes, just retries if only some part of buffer
> has been written

That's a plus point. :-)

> 2) It displays error number and string which may provide some extra
> hint to the user

My patch too since it uses mtd-util's logging functions.
sys_errmsg(...)

Could you please merge both patches and resend?

Thanks,
//richard

Re: [PATCH mtd-utils] nanddump: check write function result for errors

[Rafał Miłecki]

On 18 July 2016 at 16:10, Richard Weinberger <richard@nod.at> wrote:
> Rafał,
>
> Am 18.07.2016 um 13:59 schrieb Rafał Miłecki:
>> On 18 July 2016 at 13:33, Richard Weinberger
>> <richard.weinberger@gmail.com> wrote:
>>> On Mon, Jul 18, 2016 at 11:18 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
>>>> Errors may happen, it's e.g. easy on embedded devices to run out of space
>>>> when dumping big partitions. This patch adds a helper function for
>>>> writing. It deals with partial writes and just returns 0 on success or
>>>> error number.
>>>>
>>>> The old code didn't check for errors at all which could result in
>>>> incomplete dumps without exiting with an error.
>>>
>>> Is this patch different from my version, does it fix more?
>>> http://lists.infradead.org/pipermail/linux-mtd/2016-April/067234.html
>>>
>>> Brian, if this is fine with you I'd apply one of these patches.
>>
>> I wasn't aware of your patch, having it accepted would save me some
>> time for sure.
>>
>> Two minor advantages I see in my version:
>> 1) It handles partial writes, just retries if only some part of buffer
>> has been written
>
> That's a plus point. :-)
>
>> 2) It displays error number and string which may provide some extra
>> hint to the user
>
> My patch too since it uses mtd-util's logging functions.
> sys_errmsg(...)
>
> Could you please merge both patches and resend?

Oh, I didn't realize sys_errmsg does that. I assumed it logs into
system log (which seemed like a weird idea to do). A bit confusing
name. Thanks, I'll send V2.

-- 
Rafał

Re: [PATCH mtd-utils] nanddump: check write function result for errors

[Richard Weinberger]

Rafał,

Am 18.07.2016 um 16:51 schrieb Rafał Miłecki:
> On 18 July 2016 at 16:10, Richard Weinberger <richard@nod.at> wrote:
>> My patch too since it uses mtd-util's logging functions.
>> sys_errmsg(...)
>>
>> Could you please merge both patches and resend?
> 
> Oh, I didn't realize sys_errmsg does that. I assumed it logs into
> system log (which seemed like a weird idea to do). A bit confusing
> name. Thanks, I'll send V2.
> 

The sys_ prefix denotes that this function is used for syscall
errors. :-)

Thanks,
//richard

[PATCH V2] nanddump: check write function result for errors

[Rafał Miłecki]

Errors may happen, it's e.g. easy on embedded devices to run out of space
when dumping big partitions. This patch adds a helper function for
writing. It deals with partial writes and just returns 0 on success or
error number.

The old code didn't check for errors at all which could result in
incomplete dumps without exiting with an error.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
V2: Use sys_errmsg as in patch pointed by Richard.
    Call ofd_write also when using pretty output.
---
 nand-utils/nanddump.c | 48 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/nand-utils/nanddump.c b/nand-utils/nanddump.c
index 4ee7ed4..db1e1e5 100644
--- a/nand-utils/nanddump.c
+++ b/nand-utils/nanddump.c
@@ -293,6 +293,31 @@ nil:
 	linebuf[lx++] = '\0';
 }
 
+/**
+ * ofd_write - writes whole buffer to the file associated with a descriptor
+ *
+ * On failure an error (negative number) is returned. Otherwise 0 is returned.
+ */
+static int ofd_write(int ofd, const void *buf, size_t nbyte)
+{
+	const unsigned char *data = buf;
+	ssize_t bytes;
+
+	while (nbyte) {
+		bytes = write(ofd, data, nbyte);
+		if (bytes < 0) {
+			int err = -errno;
+
+			sys_errmsg("Unable to write to output");
+
+			return err;
+		}
+		data += bytes;
+		nbyte -= bytes;
+	}
+
+	return 0;
+}
 
 /*
  * Main program
@@ -309,6 +334,7 @@ int main(int argc, char * const argv[])
 	bool eccstats = false;
 	unsigned char *readbuf = NULL, *oobbuf = NULL;
 	libmtd_t mtd_desc;
+	int err;
 
 	process_options(argc, argv);
 
@@ -443,10 +469,15 @@ int main(int argc, char * const argv[])
 			for (i = 0; i < bs; i += PRETTY_ROW_SIZE) {
 				pretty_dump_to_buffer(readbuf + i, PRETTY_ROW_SIZE,
 						pretty_buf, PRETTY_BUF_LEN, true, canonical, ofs + i);
-				write(ofd, pretty_buf, strlen(pretty_buf));
+				err = ofd_write(ofd, pretty_buf, strlen(pretty_buf));
+				if (err)
+					goto closeall;
 			}
-		} else
-			write(ofd, readbuf, bs);
+		} else {
+			err = ofd_write(ofd, readbuf, bs);
+			if (err)
+				goto closeall;
+		}
 
 		if (omitoob)
 			continue;
@@ -466,10 +497,15 @@ int main(int argc, char * const argv[])
 			for (i = 0; i < mtd.oob_size; i += PRETTY_ROW_SIZE) {
 				pretty_dump_to_buffer(oobbuf + i, mtd.oob_size - i,
 						pretty_buf, PRETTY_BUF_LEN, false, canonical, 0);
-				write(ofd, pretty_buf, strlen(pretty_buf));
+				err = ofd_write(ofd, pretty_buf, strlen(pretty_buf));
+				if (err)
+					goto closeall;
 			}
-		} else
-			write(ofd, oobbuf, mtd.oob_size);
+		} else {
+			err = ofd_write(ofd, oobbuf, mtd.oob_size);
+			if (err)
+				goto closeall;
+		}
 	}
 
 	/* Close the output file and MTD device, free memory */
-- 
1.8.4.5

Re: [PATCH V2] nanddump: check write function result for errors

[Rafał Miłecki]

On 18 July 2016 at 17:09, Rafał Miłecki <zajec5@gmail.com> wrote:
> Errors may happen, it's e.g. easy on embedded devices to run out of space
> when dumping big partitions. This patch adds a helper function for
> writing. It deals with partial writes and just returns 0 on success or
> error number.
>
> The old code didn't check for errors at all which could result in
> incomplete dumps without exiting with an error.
>
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>

Can this finally be pushed, please?

Re: [PATCH V2] nanddump: check write function result for errors

[David Oberhollenzer]

On 09/06/2016 09:42 PM, Rafał Miłecki wrote:
> On 18 July 2016 at 17:09, Rafał Miłecki <zajec5@gmail.com> wrote:
>> Errors may happen, it's e.g. easy on embedded devices to run out of space
>> when dumping big partitions. This patch adds a helper function for
>> writing. It deals with partial writes and just returns 0 on success or
>> error number.
>>
>> The old code didn't check for errors at all which could result in
>> incomplete dumps without exiting with an error.
>>
>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> 
> Can this finally be pushed, please?

Unfortunately the mtd-utils appear to be currently not very well maintained and
a number of patches submitted to the mailing list got more or less neglected.

However there _is_ currently an effort on the way to do a major overhaul/cleanup of
the mtd-utils that also incorporates a bunch of fixes that accumulated on the mailing
list, also including this patch.

You can take a look at the current progress here:

https://github.com/sigma-star/mtd-utils/commits/wip_v2-rc1


David