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",

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)

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); }

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",