[U-Boot] [PATCHv10] new tool mkenvimage: generates an env image from an arbitrary config file

[Wolfgang Denk]

Dear David Wagner,

In message <1318612616-16799-1-git-send-email-david.wagner at free-electrons.com> you wrote:
> This tool takes a key=value configuration file (same as would a `printenv' show)
> and generates the corresponding environment image, ready to be flashed.
> 
> use case: flash the environment with an external tool

This patch fails to build when I try to run a plain "make
tools/mkenvimage":

tools/mkenvimage.c:35:22: fatal error: compiler.h: No such file or directory

tools/mkenvimage.c:39:24: fatal error: u-boot/crc.h: No such file or
directory



...
> +	/* Parse the cmdline */
> +	while ((option = getopt(argc, argv, "s:o:rbp:h")) != -1) {
> +		switch (option) {
> +		case 's':
> +			datasize = strtol(optarg, NULL, 0);
...
> +			padbyte = strtol(optarg, NULL, 0);

Please add error checking for invalid input formats here.

> +	if (datasize == 0) {
> +		fprintf(stderr,
> +			"Please specify the size of the envrionnment "
> +			"partition.\n");

Please don't split that string, and fix the "envrionment" typo.

> +	dataptr = malloc(datasize * sizeof(*dataptr));
> +	if (!dataptr) {
> +		fprintf(stderr, "Can't alloc dataptr.\n");

It might be helpful to know how many bytes you were trying to
allocate.

> +	/*
> +	 * envptr points to the beginning of the actual environment (after the
> +	 * crc and possible `redundant' bit

s/bit/byte/

> +	/* Open the input file ... */
> +	if (optind >= argc) {
> +		fprintf(stderr, "Please specify an input filename\n");
> +		return EXIT_FAILURE;
> +	}

Please also allow to use stdin as input when no "<input file>" arg is
given.

> +		int readlen = sizeof(*envptr) * 2048;
> +		txt_fd = STDIN_FILENO;
> +
> +		do {
> +			filebuf = realloc(filebuf, readlen);
> +			readbytes = read(txt_fd, filebuf + filesize, readlen);
> +			filesize += readbytes;
> +		} while (readbytes == readlen);

This is pretty much inefficient.  Consider a size increment of
something like  min(readlen, 4096).

Also, realloc() can fail - add error handling.

And read() can fail, too - add error handling.

> +		filesize = txt_file_stat.st_size;
> +		/* Read the raw input file and transform it */

Why don't you just use mmap() here?

> +	if (filesize >= envsize) {
> +		fprintf(stderr, "The input file is larger than the "
> +				"envrionnment partition size\n");

Please don't split such strings.  See CodingStyle:

        "However, never break user-visible strings such as printk
         messages, because that breaks the ability to grep for them."

Please fix globally.

> +		} else if (filebuf[fp] == '#') {
> +			if (fp != 0 && filebuf[fp-1] == '\n') {
> +				/* This line is a comment, let's skip it */
> +				while (fp < txt_file_stat.st_size &&
> +				       filebuf[fp] != '\n')
> +					fp++;
> +			} else {
> +				envptr[ep++] = filebuf[fp];
> +			}

printenv output does not contain any such "comments".
And - aren't you also catching embedded hashes here, like in "serial#"
for example?

...
> +
> +	/* Computes the CRC and put it at the beginning of the data */
> +	crc = crc32(0, envptr, envsize);
> +	targetendian_crc = bigendian ? cpu_to_be32(crc) : cpu_to_le32(crc);
> +
> +	memcpy(dataptr, &targetendian_crc, sizeof(uint32_t));

I fail to see where you set the redundant flag?


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
I am more bored than you could ever possibly be.  Go back to work.