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

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

[Marco]

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.

Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>
---

diff -uprN u-boot-2009.03-orig/tools/imls.c u-boot-2009.03/tools/imls.c
--- u-boot-2009.03-orig/tools/imls.c	1970-01-01 01:00:00.000000000 +0100
+++ u-boot-2009.03/tools/imls.c	2009-04-04 13:39:00.000000000 +0200
@@ -0,0 +1,229 @@
+/*
+ * (C) Copyright 2009 Marco Stornelli
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include "imls.h"
+
+extern unsigned long crc32(unsigned long crc, const char *buf, unsigned int len);
+static void usage(void);
+static int image_verify_header(char *ptr, int fd);
+
+char	*cmdname;
+char	*devicefile;
+
+unsigned int sectorcount = 0;
+int sflag = 0;
+unsigned int sectoroffset = 0;
+unsigned int sectorsize = 0;
+int cflag = 0;
+
+int main (int argc, char **argv)
+{
+	int fd = -1, err = 0, readbyte = 0, j;
+	uint32_t mtdsize, dim, diff;
+	struct mtd_info_user mtdinfo;
+	char *buf;
+	int found = 0;
+
+	cmdname = *argv;
+
+	while (--argc > 0 && **++argv == '-') {
+		while (*++*argv) {
+			switch (**argv) {
+			case 's':
+				if (--argc <= 0) 
+					usage ();
+				sectorcount = (unsigned int)atoi(*++argv);
+				sflag = 1;
+				goto NXTARG;
+			case 'o':
+				if (--argc <= 0)
+					usage ();
+				sectoroffset = (unsigned int)atoi(*++argv);
+				goto NXTARG;
+
+			case 'c':
+				if (--argc <= 0)
+					usage ();
+				sectorsize = (unsigned int)atoi(*++argv);
+				cflag = 1;
+				goto NXTARG;
+			default:
+				usage ();
+			}
+		}
+NXTARG:		;
+	}
+
+	if (argc != 1 || cflag == 0 || sflag == 0)
+		usage();
+
+	devicefile = *argv;
+
+	fd = open(devicefile, O_RDONLY);
+	if (fd < 0) {
+		fprintf (stderr, "%s: Can't open %s: %s\n",
+		cmdname, devicefile, strerror(errno));
+		exit(EXIT_FAILURE);
+	}
+
+	err = ioctl(fd, MEMGETINFO, &mtdinfo);
+	if (err < 0) {
+		fprintf(stderr, "%s: Cannot get MTD information: %s\n",cmdname,strerror(errno));
+		exit(EXIT_FAILURE);
+	}
+
+	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);
+	}
+
+	if (sectorsize * sectoroffset >= mtdsize) {
+		fprintf(stderr, "%s: Partition size (%d) incompatible with sector offset given\n", cmdname, mtdsize);
+		exit(EXIT_FAILURE);
+	}
+
+	if (sectoroffset > sectorcount - 1) {
+		fprintf(stderr, "%s: Sector offset cannot be grater than sector count minus one\n", cmdname);
+		exit(EXIT_FAILURE);
+	}
+
+	dim = image_get_header_size();
+
+	if (dim > sectorsize) {
+		fprintf(stderr, "%s: Image header bigger than sector size\n", cmdname);
+		exit(EXIT_FAILURE);
+	}
+
+	diff = sectorsize - dim;
+
+	buf = malloc(sizeof(char)*dim);
+	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);
+
+	printf("Searching....\n");
+
+	for (j = sectoroffset; j < sectorcount; ++j) {
+
+		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 */
+
+	}	
+
+	free(buf);
+
+	close(fd);
+
+	if(!found)
+		printf("No images found\n");
+
+	exit(EXIT_SUCCESS);
+}
+
+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);
+
+	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));
+	if (!data) {
+		fprintf (stderr, "%s: Can't allocate memory: %s\n",
+		cmdname, strerror(errno));
+		exit(EXIT_FAILURE);
+	}
+	
+	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);
+	
+	if (crc32(0, data, len) != image_get_dcrc(hdr)) {
+		fprintf (stderr,
+			"%s: Maybe image found but it has corrupted data!\n",
+			cmdname);
+		free(data);
+		return 0;
+	}
+	
+	free(data);
+	
+	return 1;
+}
+
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 @@
+/*
+ * (C) Copyright 2009 Marco Stornelli
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#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>
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
@@ -21,10 +21,10 @@
 # MA 02111-1307 USA
 #
 
-BIN_FILES	= img2srec$(SFX) mkimage$(SFX) envcrc$(SFX) ubsha1$(SFX) gen_eth_addr$(SFX) bmp_logo$(SFX)
+BIN_FILES	= img2srec$(SFX) mkimage$(SFX) imls$(SFX) envcrc$(SFX) ubsha1$(SFX) gen_eth_addr$(SFX) bmp_logo$(SFX)
 
 OBJ_LINKS	= env_embedded.o crc32.o md5.o sha1.o image.o
-OBJ_FILES	= img2srec.o mkimage.o envcrc.o ubsha1.o gen_eth_addr.o bmp_logo.o
+OBJ_FILES	= img2srec.o mkimage.o imls.o envcrc.o ubsha1.o gen_eth_addr.o bmp_logo.o
 
 ifeq ($(ARCH),mips)
 BIN_FILES	+= inca-swap-bytes$(SFX)
@@ -151,6 +151,10 @@ $(obj)mkimage$(SFX):	$(obj)mkimage.o $(o
 		$(CC) $(CFLAGS) $(HOST_LDFLAGS) -o $@ $^
 		$(STRIP) $@
 
+$(obj)imls$(SFX):	$(obj)imls.o $(obj)crc32.o $(obj)image.o $(obj)md5.o $(obj)sha1.o $(LIBFDT_OBJ_FILES)
+		$(CC) $(CFLAGS) $(HOST_LDFLAGS) -o $@ $^
+		$(STRIP) $@
+
 $(obj)ncb$(SFX):	$(obj)ncb.o
 		$(CC) $(CFLAGS) $(HOST_LDFLAGS) -o $@ $^
 		$(STRIP) $@
@@ -196,6 +200,9 @@ $(obj)image.o:	$(obj)image.c
 $(obj)mkimage.o:	$(src)mkimage.c
 		$(CC) -g $(FIT_CFLAGS) -c -o $@ $<
 
+$(obj)imls.o:	$(src)imls.c
+		$(CC) -g $(FIT_CFLAGS) -c -o $@ $<
+
 $(obj)ncb.o:		$(src)ncb.c
 		$(CC) -g $(CFLAGS) -c -o $@ $<
 

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

[Wolfgang Denk]

Dear Marco,

In message <49D74E7B.904@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.

[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@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

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

[Wolfgang Denk]

Dear Marco,

In message <49D8E8F3.4010300@gmail.com> you wrote:
> 
> >> +	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).

Please think about this again. We use a single lseek() before each
read() to position the read pointer at the next sector boundary, i.e.
where we need it. We don't care whe reit is after the read().

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

No. Who claims you must read the whole image at once? Use a fixed
buffer of a convenient size (say PAGE_SIZE), and read the data
sequentially. No need to allocate zillions of megabytes of memory at
once.

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

Your thinking is inverted. Don't try to fix the read pointer after
each read(). Set it as needed before the read() when necessary.

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
A memorandum is written not to inform the reader, but to protect  the
writer.                                               -- Dean Acheson

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

[Marco Stornelli]

Hi Wolfgang,

2009/4/5 Wolfgang Denk <wd@denx.de>:
> Dear Marco,
>
> In message <49D8E8F3.4010300@gmail.com> you wrote:
>>
>> >> + ?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).
>
> Please think about this again. We use a single lseek() before each
> read() to position the read pointer at the next sector boundary, i.e.
> where we need it. We don't care whe reit is after the read().
>

Oops sorry, I've read SEEK_CUR instead of SEEK_SET, ok in this way works.

> ...
>> >> + ?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.
>
> No. Who claims you must read the whole image at once? Use a fixed
> buffer of a convenient size (say PAGE_SIZE), and read the data
> sequentially. No need to allocate zillions of megabytes of memory at
> once.
>

Ok, I'll try to do that sequentially.

>> >> + ?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).
>
> Your thinking is inverted. Don't try to fix the read pointer after
> each read(). Set it as needed before the read() when necessary.
>
> 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
> A memorandum is written not to inform the reader, but to protect ?the
> writer. ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? -- Dean Acheson
>

I'll send a new patch with the modification as soon as