[U-Boot] [PATCH v3] Add imls utility command

[Wolfgang Denk]

Dear Marco,

In message <49D74E7B.904 at gmail.com> you wrote:
> Fixed a memory leak in image_verify_header function.
> Used the functions image_get_xxxx() from image.h.
> Fixed two sector boundary misalignment while reading.
...
> +	err = ioctl(fd, MEMGETINFO, &mtdinfo);
> +	if (err < 0) {
> +		fprintf(stderr, "%s: Cannot get MTD information: %s\n",cmdname,strerror(errno));

Line too long (check also rest of file).

> +		exit(EXIT_FAILURE);
> +	}

You probably want to check for unsupported flash types, too - see
example in "tools/env/fw_env.c"

> +	mtdsize = mtdinfo.size;
> +
> +	if (sectorsize * sectorcount != mtdsize) {
> +		fprintf(stderr, "%s: Partition size (%d) incompatible with sector size and count\n", cmdname, mtdsize);
> +		exit(EXIT_FAILURE);
> +	}

Hmm... what happens if an image starts in one of the small sectors in
a bottom boot type flash?

...
> +	buf = malloc(sizeof(char)*dim);

Why not just use a dynamic array on the stack? Get rid of all this
malloc() and free() stuff...

> +	if (!buf) {
> +		fprintf (stderr, "%s: Can't allocate memory: %s\n",
> +		cmdname, strerror(errno));
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	if (sectoroffset != 0)
> +		lseek(fd, sectoroffset*sectorsize, SEEK_SET);

Delete these two lines and mode them into the loop...

> +	printf("Searching....\n");
> +
> +	for (j = sectoroffset; j < sectorcount; ++j) {

i. e. add

		if (lseek(fd, j*sectorsize, SEEK_SET) != j*sectorsize) {
			error handling goes here
		}

> +		readbyte = read(fd, buf, dim);
> +		if (readbyte != dim) {
> +			fprintf(stderr, "%s: Can't read from device: %s\n",
> +			cmdname, strerror(errno));
> +			exit(EXIT_FAILURE);
> +		}
> +
> +		if (fdt_check_header(buf)) {
> +			/* old-style image */
> +			if (image_verify_header(buf, fd)) {
> +				found = 1;
> +				image_print_contents((image_header_t *)buf);
> +			}
> +		} else {
> +			/* FIT image */
> +			fit_print_contents(buf);
> +		}
> +
> +		lseek(fd, diff, SEEK_CUR); /* Align to next sector boundary */

Delete this line then, too.

> +void usage()
> +{
> +	fprintf (stderr, "Usage:\n"
> +			 "       %s [-o offset] -s count -c size \n"
> +			 "          -o ==> number of sectors to use as offset\n"
> +			 "          -s ==> number of sectors\n"
> +			 "          -c ==> size of sectors\n",
> +		cmdname);

Usage message is not correct - the device argument is missing.

> +	exit (EXIT_FAILURE);
> +}
> +
> +static int image_verify_header(char *ptr, int fd)
> +{
> +	int len, n;
> +	char *data;
> +	uint32_t checksum;
> +	image_header_t *hdr = (image_header_t *)ptr;
> +
> +	if (image_get_magic(hdr) != IH_MAGIC) {
> +		return 0;
> +	}
> +
> +	data = (char *)hdr;
> +	len  = image_get_header_size();
> +
> +	checksum = image_get_hcrc(hdr);
> +	hdr->ih_hcrc = htonl(0);	/* clear for re-calculation */
> +
> +	if (crc32(0, data, len) != checksum) {
> +		fprintf (stderr,
> +			"%s: Maybe image found but it has bad header checksum!\n",
> +			cmdname);
> +		return 0;
> +	}
> +
> +	len = image_get_size(hdr);
> +	data = malloc(len * sizeof(char));

Come on. sizeof(char) ? You must be joking.

> +	if (!data) {
> +		fprintf (stderr, "%s: Can't allocate memory: %s\n",
> +		cmdname, strerror(errno));
> +		exit(EXIT_FAILURE);
> +	}

Get rid of this malloc(), too.

> +	n = read(fd, data, len);
> +	if (n != len) {
> +		fprintf (stderr,
> +			"%s: Error while reading: %s\n",
> +			cmdname, strerror(errno));
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	lseek(fd, -len, SEEK_CUR);

Why would this lseek() be needed? [And ifit ws needed, you should
check it's return value.]

> diff -uprN u-boot-2009.03-orig/tools/imls.h u-boot-2009.03/tools/imls.h
> --- u-boot-2009.03-orig/tools/imls.h	1970-01-01 01:00:00.000000000 +0100
> +++ u-boot-2009.03/tools/imls.h	2009-03-29 11:43:31.000000000 +0200
> @@ -0,0 +1,41 @@
...
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stddef.h>
> +#include <string.h>
> +#include <sys/types.h>
> +#include <sys/ioctl.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> +
> +#ifdef MTD_OLD
> +# include <stdint.h>
> +# include <linux/mtd/mtd.h>
> +#else
> +# define  __user	/* nothing */
> +# include <mtd/mtd-user.h>
> +#endif
> +
> +#include <sha1.h>
> +#include "fdt_host.h"
> +#include <image.h>

Such a header file makes no sense, especially with just a single user.
Dump it.

> diff -uprN u-boot-2009.03-orig/tools/Makefile u-boot-2009.03/tools/Makefile
> --- u-boot-2009.03-orig/tools/Makefile	2009-03-21 22:04:41.000000000 +0100
> +++ u-boot-2009.03/tools/Makefile	2009-03-28 18:47:38.000000000 +0100

Please also rebase against current top of tree.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Swap read error.  You lose your mind.