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

Dear David Wagner,

In message 1318612616-16799-1-git-send-email-david.wagner@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