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

[Marco]

Hi Wolfgang,

first of all thank you for your comments.

Wolfgang Denk ha scritto:
> 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).
> 

Ok. I see other files where some lines exceed 80 columns, is the limit
of 80? I'll fix it.

>> +		exit(EXIT_FAILURE);
>> +	}
> 
> You probably want to check for unsupported flash types, too - see
> example in "tools/env/fw_env.c"
Ok.

> 
>> +	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?
Do you mean with an asymmetric flash layout? My first idea was manage
only homogeneous situation. However you could use sectorsize equals to
the smallest size and then use the offset parameter to start the
searching at the bottom of the 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...
Do you mean a thing like this: char buf[sizeof(image_header_t)]; ? Ok no
problem.
> 
>> +	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
> 		}

Mmm...I don't well understand why. For example if a sector is 128k and
the header size is for example of 100 byte, after the read below, at the
beginning of the cycle, the offset will be 100 byte (we say that
sectoroffset is 0) and for example we don't find any image. At next
cycle if I do an lseek of 1*128K, I'll get (1*128K)+100 byte as offset,
at next (2*128k) + 200 byte and so on. To do this I should do an lseek
after the read as lseek(fd,-readbyte,SEEK_CUR).

> 
>> +		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.
Oops my fault. Ok.

> 
>> +	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.
I can't know in this case the size of the data, to remove the malloc I
have to allocate a fixed buffer of X byte, I think in this case a malloc
is better.

> 
>> +	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.]

It was needed to be aligned to sector boundary and set the file offset
as before to call this function (see my comment above).

> 
>> 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.

Ok.

> 
>> 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.
Ok.
> 
> 
> Best regards,
> 
> Wolfgang Denk
> 
Regards,

Marco