Patchwork [U-Boot,RFC,35/44] mkimage: Put FIT loading in function and tidy error handling

login
register
mail settings
Submitter Simon Glass
Date Jan. 5, 2013, 1:52 a.m.
Message ID <1357350734-13737-36-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/209612/
State Superseded, archived
Headers show

Comments

Simon Glass - Jan. 5, 2013, 1:52 a.m.
The fit_handle_file() function is quiet long - split out the part that
loads and checks a FIT into its own function. We will use this
function for storing public keys into a destination FDT file.

The error handling is currently a bit repetitive - tidy it.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 tools/fit_image.c |   96 +++++++++++++++++++++++++++++++---------------------
 1 files changed, 57 insertions(+), 39 deletions(-)
Marek Vasut - Jan. 5, 2013, 8:24 a.m.
Dear Simon Glass,

> The fit_handle_file() function is quiet long

quite ;-)

> - split out the part that
> loads and checks a FIT into its own function. We will use this
> function for storing public keys into a destination FDT file.
> 
> The error handling is currently a bit repetitive - tidy it.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>  tools/fit_image.c |   96
> +++++++++++++++++++++++++++++++--------------------- 1 files changed, 57
> insertions(+), 39 deletions(-)
> 
> diff --git a/tools/fit_image.c b/tools/fit_image.c
> index e0675d7..0f619a2 100644
> --- a/tools/fit_image.c
> +++ b/tools/fit_image.c
> @@ -47,6 +47,48 @@ static int fit_check_image_types (uint8_t type)
>  		return EXIT_FAILURE;
>  }
> 
> +int mmap_fdt(struct mkimage_params *params, const char *fname, void
> **blobp, +		struct stat *sbuf)
> +{
> +	void *ptr;
> +	int fd;
> +
> +	/* load FIT blob into memory */
> +	fd = open(fname, O_RDWR|O_BINARY);

Why is it RDWR even?

otherwise

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut
Simon Glass - Jan. 5, 2013, 9:51 p.m.
Hi Marek,

On Sat, Jan 5, 2013 at 12:24 AM, Marek Vasut <marex@denx.de> wrote:
> Dear Simon Glass,
>
>> The fit_handle_file() function is quiet long
>
> quite ;-)

Quite.

>
>> - split out the part that
>> loads and checks a FIT into its own function. We will use this
>> function for storing public keys into a destination FDT file.
>>
>> The error handling is currently a bit repetitive - tidy it.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>  tools/fit_image.c |   96
>> +++++++++++++++++++++++++++++++--------------------- 1 files changed, 57
>> insertions(+), 39 deletions(-)
>>
>> diff --git a/tools/fit_image.c b/tools/fit_image.c
>> index e0675d7..0f619a2 100644
>> --- a/tools/fit_image.c
>> +++ b/tools/fit_image.c
>> @@ -47,6 +47,48 @@ static int fit_check_image_types (uint8_t type)
>>               return EXIT_FAILURE;
>>  }
>>
>> +int mmap_fdt(struct mkimage_params *params, const char *fname, void
>> **blobp, +            struct stat *sbuf)
>> +{
>> +     void *ptr;
>> +     int fd;
>> +
>> +     /* load FIT blob into memory */
>> +     fd = open(fname, O_RDWR|O_BINARY);
>
> Why is it RDWR even?

Because the blob gets updated (e.g. by writing hashes to the FIT). I
will add a comment.

>
> otherwise
>
> Reviewed-by: Marek Vasut <marex@denx.de>
>
> Best regards,
> Marek Vasut

Regards,
Simon

Patch

diff --git a/tools/fit_image.c b/tools/fit_image.c
index e0675d7..0f619a2 100644
--- a/tools/fit_image.c
+++ b/tools/fit_image.c
@@ -47,6 +47,48 @@  static int fit_check_image_types (uint8_t type)
 		return EXIT_FAILURE;
 }
 
+int mmap_fdt(struct mkimage_params *params, const char *fname, void **blobp,
+		struct stat *sbuf)
+{
+	void *ptr;
+	int fd;
+
+	/* load FIT blob into memory */
+	fd = open(fname, O_RDWR|O_BINARY);
+
+	if (fd < 0) {
+		fprintf(stderr, "%s: Can't open %s: %s\n",
+				params->cmdname, fname, strerror(errno));
+		unlink(fname);
+		return -1;
+	}
+
+	if (fstat(fd, sbuf) < 0) {
+		fprintf(stderr, "%s: Can't stat %s: %s\n",
+				params->cmdname, fname, strerror(errno));
+		unlink(fname);
+		return -1;
+	}
+
+	ptr = mmap(0, sbuf->st_size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
+	if (ptr == MAP_FAILED) {
+		fprintf(stderr, "%s: Can't read %s: %s\n",
+				params->cmdname, fname, strerror(errno));
+		unlink(fname);
+		return -1;
+	}
+
+	/* check if ptr has a valid blob */
+	if (fdt_check_header(ptr)) {
+		fprintf(stderr, "%s: Invalid FIT blob\n", params->cmdname);
+		unlink(fname);
+		return -1;
+	}
+
+	*blobp = ptr;
+	return fd;
+}
+
 /**
  * fit_handle_file - main FIT file processing function
  *
@@ -65,7 +107,7 @@  static int fit_handle_file (struct mkimage_params *params)
 	char cmd[MKIMAGE_MAX_DTC_CMDLINE_LEN];
 	int tfd;
 	struct stat sbuf;
-	unsigned char *ptr;
+	void *ptr;
 
 	/* Flattened Image Tree (FIT) format  handling */
 	debug ("FIT format handling\n");
@@ -87,57 +129,25 @@  static int fit_handle_file (struct mkimage_params *params)
 	if (system (cmd) == -1) {
 		fprintf (stderr, "%s: system(%s) failed: %s\n",
 				params->cmdname, cmd, strerror(errno));
-		unlink (tmpfile);
-		return (EXIT_FAILURE);
-	}
-
-	/* load FIT blob into memory */
-	tfd = open (tmpfile, O_RDWR|O_BINARY);
-
-	if (tfd < 0) {
-		fprintf (stderr, "%s: Can't open %s: %s\n",
-				params->cmdname, tmpfile, strerror(errno));
-		unlink (tmpfile);
-		return (EXIT_FAILURE);
+		goto err_system;
 	}
 
-	if (fstat (tfd, &sbuf) < 0) {
-		fprintf (stderr, "%s: Can't stat %s: %s\n",
-				params->cmdname, tmpfile, strerror(errno));
-		unlink (tmpfile);
-		return (EXIT_FAILURE);
-	}
-
-	ptr = mmap (0, sbuf.st_size, PROT_READ|PROT_WRITE, MAP_SHARED,
-				tfd, 0);
-	if (ptr == MAP_FAILED) {
-		fprintf (stderr, "%s: Can't read %s: %s\n",
-				params->cmdname, tmpfile, strerror(errno));
-		unlink (tmpfile);
-		return (EXIT_FAILURE);
-	}
-
-	/* check if ptr has a valid blob */
-	if (fdt_check_header (ptr)) {
-		fprintf (stderr, "%s: Invalid FIT blob\n", params->cmdname);
-		unlink (tmpfile);
-		return (EXIT_FAILURE);
-	}
+	tfd = mmap_fdt(params, tmpfile, &ptr, &sbuf);
+	if (tfd < 0)
+		goto err_mmap;
 
 	/* set hashes for images in the blob */
 	if (fit_add_verification_data(NULL, NULL, ptr, NULL, 0)) {
 		fprintf (stderr, "%s Can't add hashes to FIT blob",
 				params->cmdname);
-		unlink (tmpfile);
-		return (EXIT_FAILURE);
+		goto err_add_hashes;
 	}
 
 	/* add a timestamp at offset 0 i.e., root  */
 	if (fit_set_timestamp (ptr, 0, sbuf.st_mtime)) {
 		fprintf (stderr, "%s: Can't add image timestamp\n",
 				params->cmdname);
-		unlink (tmpfile);
-		return (EXIT_FAILURE);
+		goto err_add_timestamp;
 	}
 	debug ("Added timestamp successfully\n");
 
@@ -153,6 +163,14 @@  static int fit_handle_file (struct mkimage_params *params)
 		return (EXIT_FAILURE);
 	}
 	return (EXIT_SUCCESS);
+
+err_add_timestamp:
+err_add_hashes:
+	munmap(ptr, sbuf.st_size);
+err_mmap:
+err_system:
+	unlink(tmpfile);
+	return -1;
 }
 
 static int fit_check_params (struct mkimage_params *params)