* [U-Boot] [PATCH 2/5] tools: dumpimage: Simplify arguments
2019-01-26 2:31 [U-Boot] [PATCH 1/5] tools: dumpimage: Provide more feedback on error Martyn Welch
@ 2019-01-26 2:31 ` Martyn Welch
2019-02-03 1:09 ` [U-Boot] [U-Boot,2/5] " Tom Rini
2019-01-26 2:31 ` [U-Boot] [PATCH 3/5] tools: dumpimage: Simplify internal logic Martyn Welch
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Martyn Welch @ 2019-01-26 2:31 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [U-Boot] [U-Boot,2/5] tools: dumpimage: Simplify arguments
2019-01-26 2:31 ` [U-Boot] [PATCH 2/5] tools: dumpimage: Simplify arguments Martyn Welch
@ 2019-02-03 1:09 ` Tom Rini
0 siblings, 0 replies; 10+ messages in thread
From: Tom Rini @ 2019-02-03 1:09 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 3/5] tools: dumpimage: Simplify internal logic
2019-01-26 2:31 [U-Boot] [PATCH 1/5] tools: dumpimage: Provide more feedback on error Martyn Welch
2019-01-26 2:31 ` [U-Boot] [PATCH 2/5] tools: dumpimage: Simplify arguments Martyn Welch
@ 2019-01-26 2:31 ` Martyn Welch
2019-02-03 1:09 ` [U-Boot] [U-Boot, " Tom Rini
2019-01-26 2:31 ` [U-Boot] [PATCH 4/5] tools: dumpimage: Add help option and make error paths consistent Martyn Welch
` (2 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Martyn Welch @ 2019-01-26 2:31 UTC (permalink / raw)
To: u-boot
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, ¶ms);
- }
-
- (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,
+ ¶ms);
}
+ (void)munmap((void *)ptr, sbuf.st_size);
(void)close(ifd);
- return EXIT_SUCCESS;
+ return retval;
}
static void usage(void)
--
2.20.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [U-Boot] [PATCH 4/5] tools: dumpimage: Add help option and make error paths consistent
2019-01-26 2:31 [U-Boot] [PATCH 1/5] tools: dumpimage: Provide more feedback on error Martyn Welch
2019-01-26 2:31 ` [U-Boot] [PATCH 2/5] tools: dumpimage: Simplify arguments Martyn Welch
2019-01-26 2:31 ` [U-Boot] [PATCH 3/5] tools: dumpimage: Simplify internal logic Martyn Welch
@ 2019-01-26 2:31 ` Martyn Welch
2019-02-03 1:09 ` [U-Boot] [U-Boot, " Tom Rini
2019-01-26 2:31 ` [U-Boot] [PATCH 5/5] tools: dumpimage: Clarify help Martyn Welch
2019-02-03 1:09 ` [U-Boot] [U-Boot, 1/5] tools: dumpimage: Provide more feedback on error Tom Rini
4 siblings, 1 reply; 10+ messages in thread
From: Martyn Welch @ 2019-01-26 2:31 UTC (permalink / raw)
To: u-boot
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(¶ms)) {
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [U-Boot] [PATCH 5/5] tools: dumpimage: Clarify help
2019-01-26 2:31 [U-Boot] [PATCH 1/5] tools: dumpimage: Provide more feedback on error Martyn Welch
` (2 preceding siblings ...)
2019-01-26 2:31 ` [U-Boot] [PATCH 4/5] tools: dumpimage: Add help option and make error paths consistent Martyn Welch
@ 2019-01-26 2:31 ` Martyn Welch
2019-02-03 1:09 ` [U-Boot] [U-Boot,5/5] " Tom Rini
2019-02-03 1:09 ` [U-Boot] [U-Boot, 1/5] tools: dumpimage: Provide more feedback on error Tom Rini
4 siblings, 1 reply; 10+ messages in thread
From: Martyn Welch @ 2019-01-26 2:31 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [U-Boot] [U-Boot, 1/5] tools: dumpimage: Provide more feedback on error
2019-01-26 2:31 [U-Boot] [PATCH 1/5] tools: dumpimage: Provide more feedback on error Martyn Welch
` (3 preceding siblings ...)
2019-01-26 2:31 ` [U-Boot] [PATCH 5/5] tools: dumpimage: Clarify help Martyn Welch
@ 2019-02-03 1:09 ` Tom Rini
4 siblings, 0 replies; 10+ messages in thread
From: Tom Rini @ 2019-02-03 1:09 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 10+ messages in thread