[U-Boot] [PATCH 1/5] tools: dumpimage: Provide more feedback on error

[U-Boot] [PATCH 1/5] tools: dumpimage: Provide more feedback on error

[Martyn Welch]

The dumpimage utility errors out in a number of places without providing
sufficient feedback to allow the user to easily determine what they have
done wrong. Add addtional error messages to make the cause of the failure
more obvious.

Signed-off-by: Martyn Welch <martyn.welch@collabora.com>
---

 tools/dumpimage.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/dumpimage.c b/tools/dumpimage.c
index 7115df04c1..2847e6c0b4 100644
--- a/tools/dumpimage.c
+++ b/tools/dumpimage.c
@@ -80,6 +80,8 @@ int main(int argc, char **argv)
 		case 'T':
 			params.type = genimg_get_type_id(optarg);
 			if (params.type < 0) {
+				fprintf(stderr, "%s: Invalid type\n",
+					params.cmdname);
 				usage();
 			}
 			break;
@@ -101,8 +103,10 @@ int main(int argc, char **argv)
 		}
 	}
 
-	if (optind >= argc)
+	if (optind >= argc) {
+		fprintf(stderr, "%s: image file missing\n", params.cmdname);
 		usage();
+	}
 
 	/* set tparams as per input type_id */
 	tparams = imagetool_get_type(params.type);
@@ -117,8 +121,11 @@ int main(int argc, char **argv)
 	 * as per image type to be generated/listed
 	 */
 	if (tparams->check_params) {
-		if (tparams->check_params(&params))
+		if (tparams->check_params(&params)) {
+			fprintf(stderr, "%s: Parameter check failed\n",
+				params.cmdname);
 			usage();
+		}
 	}
 
 	if (params.iflag)
-- 
2.20.1

[U-Boot] [PATCH 2/5] tools: dumpimage: Simplify arguments

[Martyn Welch]

The dump image utility has very confusing syntax. If called to list image
contents ("-l") it takes the image name as a positional argument. If the
utility is called to extract something from the image, the image must be
provided via the optional argument "-i" as well as the positional argument
but the value passed in the positional argument will be completely
ignored.

Simplify dumpimage by always providing the image as the first positional
argument. Assume we want to dump something from the image if we do not
provide the "-l" option for now.

Signed-off-by: Martyn Welch <martyn.welch@collabora.com>
---

 tools/dumpimage.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/tools/dumpimage.c b/tools/dumpimage.c
index 2847e6c0b4..1b92dec364 100644
--- a/tools/dumpimage.c
+++ b/tools/dumpimage.c
@@ -65,17 +65,14 @@ int main(int argc, char **argv)
 
 	params.cmdname = *argv;
 
-	while ((opt = getopt(argc, argv, "li:o:T:p:V")) != -1) {
+	while ((opt = getopt(argc, argv, "lo:T:p:V")) != -1) {
 		switch (opt) {
 		case 'l':
 			params.lflag = 1;
 			break;
-		case 'i':
-			params.imagefile = optarg;
-			params.iflag = 1;
-			break;
 		case 'o':
 			params.outfile = optarg;
+			params.iflag = 1;
 			break;
 		case 'T':
 			params.type = genimg_get_type_id(optarg);
@@ -108,6 +105,8 @@ int main(int argc, char **argv)
 		usage();
 	}
 
+	params.imagefile = argv[optind];
+
 	/* set tparams as per input type_id */
 	tparams = imagetool_get_type(params.type);
 	if (tparams == NULL) {
@@ -128,12 +127,11 @@ int main(int argc, char **argv)
 		}
 	}
 
-	if (params.iflag)
-		params.datafile = argv[optind];
-	else
-		params.imagefile = argv[optind];
-	if (!params.outfile)
-		params.outfile = params.datafile;
+	if (!params.lflag && !params.outfile) {
+		fprintf(stderr, "%s: No output file provided\n",
+			params.cmdname);
+		exit(EXIT_FAILURE);
+	}
 
 	ifd = open(params.imagefile, O_RDONLY|O_BINARY);
 	if (ifd < 0) {
@@ -204,10 +202,9 @@ static void usage(void)
 		"          -l ==> list image header information\n",
 		params.cmdname);
 	fprintf(stderr,
-		"       %s -i image -T type [-p position] [-o outfile] data_file\n"
-		"          -i ==> extract from the 'image' a specific 'data_file'\n"
+		"       %s -T type [-p position] [-o outfile] image\n"
 		"          -T ==> set image type to 'type'\n"
-		"          -p ==> 'position' (starting at 0) of the 'data_file' inside the 'image'\n",
+		"          -p ==> 'position' (starting at 0) of the component to extract from image\n",
 		params.cmdname);
 	fprintf(stderr,
 		"       %s -V ==> print version information and exit\n",
-- 
2.20.1

[U-Boot] [PATCH 3/5] tools: dumpimage: Simplify internal logic

[Martyn Welch]

There are 3 supported modes of operation:

1) Show version
2) List image contents
3) Extract image component

Option (1) terminates early, so only options (2) and (3) remain. Remove
redundant check for these modes.

Signed-off-by: Martyn Welch <martyn.welch@collabora.com>
---

 tools/dumpimage.c | 85 +++++++++++++++++++++--------------------------
 1 file changed, 37 insertions(+), 48 deletions(-)

diff --git a/tools/dumpimage.c b/tools/dumpimage.c
index 1b92dec364..e17e979b04 100644
--- a/tools/dumpimage.c
+++ b/tools/dumpimage.c
@@ -60,7 +60,7 @@ int main(int argc, char **argv)
 	int ifd = -1;
 	struct stat sbuf;
 	char *ptr;
-	int retval = 0;
+	int retval = EXIT_SUCCESS;
 	struct image_type_params *tparams = NULL;
 
 	params.cmdname = *argv;
@@ -135,65 +135,54 @@ int main(int argc, char **argv)
 
 	ifd = open(params.imagefile, O_RDONLY|O_BINARY);
 	if (ifd < 0) {
-		fprintf(stderr, "%s: Can't open \"%s\": %s\n",
-			params.cmdname, params.imagefile,
-			strerror(errno));
+		fprintf(stderr, "%s: Can't open \"%s\": %s\n", params.cmdname,
+			params.imagefile, strerror(errno));
 		exit(EXIT_FAILURE);
 	}
 
-	if (params.lflag || params.iflag) {
-		if (fstat(ifd, &sbuf) < 0) {
-			fprintf(stderr, "%s: Can't stat \"%s\": %s\n",
-				params.cmdname, params.imagefile,
-				strerror(errno));
-			exit(EXIT_FAILURE);
-		}
+	if (fstat(ifd, &sbuf) < 0) {
+		fprintf(stderr, "%s: Can't stat \"%s\": %s\n", params.cmdname,
+			params.imagefile, strerror(errno));
+		exit(EXIT_FAILURE);
+	}
 
-		if ((uint32_t)sbuf.st_size < tparams->header_size) {
-			fprintf(stderr,
-				"%s: Bad size: \"%s\" is not valid image\n",
-				params.cmdname, params.imagefile);
-			exit(EXIT_FAILURE);
-		}
+	if ((uint32_t)sbuf.st_size < tparams->header_size) {
+		fprintf(stderr, "%s: Bad size: \"%s\" is not valid image\n",
+			params.cmdname, params.imagefile);
+		exit(EXIT_FAILURE);
+	}
 
-		ptr = mmap(0, sbuf.st_size, PROT_READ, MAP_SHARED, ifd, 0);
-		if (ptr == MAP_FAILED) {
-			fprintf(stderr, "%s: Can't read \"%s\": %s\n",
-				params.cmdname, params.imagefile,
-				strerror(errno));
-			exit(EXIT_FAILURE);
-		}
+	ptr = mmap(0, sbuf.st_size, PROT_READ, MAP_SHARED, ifd, 0);
+	if (ptr == MAP_FAILED) {
+		fprintf(stderr, "%s: Can't read \"%s\": %s\n", params.cmdname,
+			params.imagefile, strerror(errno));
+		exit(EXIT_FAILURE);
+	}
 
+	/*
+	 * Both calls bellow scan through dumpimage registry for all
+	 * supported image types and verify the input image file
+	 * header for match
+	 */
+	if (params.iflag) {
 		/*
-		 * Both calls bellow scan through dumpimage registry for all
-		 * supported image types and verify the input image file
-		 * header for match
+		 * Extract the data files from within the matched
+		 * image type. Returns the error code if not matched
 		 */
-		if (params.iflag) {
-			/*
-			 * Extract the data files from within the matched
-			 * image type. Returns the error code if not matched
-			 */
-			retval = dumpimage_extract_subimage(tparams, ptr,
-					&sbuf);
-		} else {
-			/*
-			 * Print the image information for matched image type
-			 * Returns the error code if not matched
-			 */
-			retval = imagetool_verify_print_header(ptr, &sbuf,
-					tparams, &params);
-		}
-
-		(void)munmap((void *)ptr, sbuf.st_size);
-		(void)close(ifd);
-
-		return retval;
+		retval = dumpimage_extract_subimage(tparams, ptr, &sbuf);
+	} else {
+		/*
+		 * Print the image information for matched image type
+		 * Returns the error code if not matched
+		 */
+		retval = imagetool_verify_print_header(ptr, &sbuf, tparams,
+						       &params);
 	}
 
+	(void)munmap((void *)ptr, sbuf.st_size);
 	(void)close(ifd);
 
-	return EXIT_SUCCESS;
+	return retval;
 }
 
 static void usage(void)
-- 
2.20.1

[U-Boot] [PATCH 4/5] tools: dumpimage: Add help option and make error paths consistent

[Martyn Welch]

The utility dumpimage has error paths that display the usage and others
that exit without displaying usage. Add an explicit help option to
dumpimage to display the usage and remove it's use in error paths to make
the error messages more obvious and errors paths more consistent.

Signed-off-by: Martyn Welch <martyn.welch@collabora.com>
---

 tools/dumpimage.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/tools/dumpimage.c b/tools/dumpimage.c
index e17e979b04..5c9ad36322 100644
--- a/tools/dumpimage.c
+++ b/tools/dumpimage.c
@@ -65,7 +65,7 @@ int main(int argc, char **argv)
 
 	params.cmdname = *argv;
 
-	while ((opt = getopt(argc, argv, "lo:T:p:V")) != -1) {
+	while ((opt = getopt(argc, argv, "hlo:T:p:V")) != -1) {
 		switch (opt) {
 		case 'l':
 			params.lflag = 1;
@@ -79,7 +79,7 @@ int main(int argc, char **argv)
 			if (params.type < 0) {
 				fprintf(stderr, "%s: Invalid type\n",
 					params.cmdname);
-				usage();
+				exit(EXIT_FAILURE);
 			}
 			break;
 		case 'p':
@@ -94,15 +94,20 @@ int main(int argc, char **argv)
 		case 'V':
 			printf("dumpimage version %s\n", PLAIN_VERSION);
 			exit(EXIT_SUCCESS);
+		case 'h':
+			usage();
 		default:
 			usage();
 			break;
 		}
 	}
 
+	if (argc < 2)
+		usage();
+
 	if (optind >= argc) {
 		fprintf(stderr, "%s: image file missing\n", params.cmdname);
-		usage();
+		exit(EXIT_FAILURE);
 	}
 
 	params.imagefile = argv[optind];
@@ -123,7 +128,7 @@ int main(int argc, char **argv)
 		if (tparams->check_params(&params)) {
 			fprintf(stderr, "%s: Parameter check failed\n",
 				params.cmdname);
-			usage();
+			exit(EXIT_FAILURE);
 		}
 	}
 
@@ -195,9 +200,12 @@ static void usage(void)
 		"          -T ==> set image type to 'type'\n"
 		"          -p ==> 'position' (starting at 0) of the component to extract from image\n",
 		params.cmdname);
+	fprintf(stderr,
+		"       %s -h ==> print usage information and exit\n",
+		params.cmdname);
 	fprintf(stderr,
 		"       %s -V ==> print version information and exit\n",
 		params.cmdname);
 
-	exit(EXIT_FAILURE);
+	exit(EXIT_SUCCESS);
 }
-- 
2.20.1

[U-Boot] [PATCH 5/5] tools: dumpimage: Clarify help

[Martyn Welch]

Help message isn't clear over the use of the "-T" option (it's to declare
the type of image that the tool is operating on), which also is optional
as it defaults to the default image type. It's also missing a description
of the "-o" option, so add it.

Signed-off-by: Martyn Welch <martyn.welch@collabora.com>

---

 tools/dumpimage.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/dumpimage.c b/tools/dumpimage.c
index 5c9ad36322..ee3d41dda4 100644
--- a/tools/dumpimage.c
+++ b/tools/dumpimage.c
@@ -196,9 +196,10 @@ static void usage(void)
 		"          -l ==> list image header information\n",
 		params.cmdname);
 	fprintf(stderr,
-		"       %s -T type [-p position] [-o outfile] image\n"
-		"          -T ==> set image type to 'type'\n"
-		"          -p ==> 'position' (starting at 0) of the component to extract from image\n",
+		"       %s [-T type] [-p position] [-o outfile] image\n"
+		"          -T ==> declare image type as 'type'\n"
+		"          -p ==> 'position' (starting at 0) of the component to extract from image\n"
+		"          -o ==> extract component to file 'outfile'\n",
 		params.cmdname);
 	fprintf(stderr,
 		"       %s -h ==> print usage information and exit\n",
-- 
2.20.1

[U-Boot] [U-Boot, 1/5] tools: dumpimage: Provide more feedback on error

[Tom Rini]

On Sat, Jan 26, 2019 at 02:31:50AM +0000, Martyn Welch wrote:

> The dumpimage utility errors out in a number of places without providing
> sufficient feedback to allow the user to easily determine what they have
> done wrong. Add addtional error messages to make the cause of the failure
> more obvious.
> 
> Signed-off-by: Martyn Welch <martyn.welch@collabora.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190202/28cdd4f7/attachment.sig>

[U-Boot] [U-Boot,2/5] tools: dumpimage: Simplify arguments

[Tom Rini]

On Sat, Jan 26, 2019 at 02:31:51AM +0000, Martyn Welch wrote:

> The dump image utility has very confusing syntax. If called to list image
> contents ("-l") it takes the image name as a positional argument. If the
> utility is called to extract something from the image, the image must be
> provided via the optional argument "-i" as well as the positional argument
> but the value passed in the positional argument will be completely
> ignored.
> 
> Simplify dumpimage by always providing the image as the first positional
> argument. Assume we want to dump something from the image if we do not
> provide the "-l" option for now.
> 
> Signed-off-by: Martyn Welch <martyn.welch@collabora.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190202/4e8ff92f/attachment.sig>

[U-Boot] [U-Boot, 3/5] tools: dumpimage: Simplify internal logic

[Tom Rini]

On Sat, Jan 26, 2019 at 02:31:52AM +0000, Martyn Welch wrote:

> There are 3 supported modes of operation:
> 
> 1) Show version
> 2) List image contents
> 3) Extract image component
> 
> Option (1) terminates early, so only options (2) and (3) remain. Remove
> redundant check for these modes.
> 
> Signed-off-by: Martyn Welch <martyn.welch@collabora.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190202/7a683219/attachment.sig>

[U-Boot] [U-Boot, 4/5] tools: dumpimage: Add help option and make error paths consistent

[Tom Rini]

On Sat, Jan 26, 2019 at 02:31:53AM +0000, Martyn Welch wrote:

> The utility dumpimage has error paths that display the usage and others
> that exit without displaying usage. Add an explicit help option to
> dumpimage to display the usage and remove it's use in error paths to make
> the error messages more obvious and errors paths more consistent.
> 
> Signed-off-by: Martyn Welch <martyn.welch@collabora.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190202/0b39de21/attachment-0001.sig>

[U-Boot] [U-Boot,5/5] tools: dumpimage: Clarify help

[Tom Rini]

On Sat, Jan 26, 2019 at 02:31:54AM +0000, Martyn Welch wrote:

> Help message isn't clear over the use of the "-T" option (it's to declare
> the type of image that the tool is operating on), which also is optional
> as it defaults to the default image type. It's also missing a description
> of the "-o" option, so add it.
> 
> Signed-off-by: Martyn Welch <martyn.welch@collabora.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190202/3105c6e2/attachment.sig>