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

[Wolfgang Denk]

Dear David Wagner,

In message <1312885889-20222-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 environnment image, ready to be flashed.

s/nnm/nm/

> 
> Signed-off-by: David Wagner <david.wagner at free-electrons.com>
> ---
> 
> 	Hi Mike,
> 
> This 3rd version should address what you pointed out.

If this is v3, then why don't you say so in the Subject: ???


Could you please explain which usage scenarios you have in mind for
this tool?  I would probably rather just load the text file you use as
input, and run "env import -t" on it.

Checkpatch says:

total: 4 errors, 5 warnings, 228 lines checked

Please fix these.

In addition:

...
> +		default:
> +			fprintf(stderr, "Wrong option -%c\n", option);
> +			usage(argv[0]);
> +			return EXIT_FAILURE;
> +		}
> +	}
> +
> +
> +	/* Check datasize and allocate the data */

Please only one blank line in cases like this.

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

inforrect multiline comment style.

> +	/* Open the configuration file ... */
> +	txt_filename = argv[optind];
> +	if (!txt_filename) {
> +		fprintf(stderr, "Can't strdup() the configuration filename\n");
> +		return EXIT_FAILURE;

Where is there any strdup() involved ?

> +	txt_file = fopen(txt_filename, "rb");
> +	if (!txt_file) {
> +		fprintf(stderr, "Can't open configuration file: %s\n", strerror(errno));

It would probably be very useful to also print the file name.

> +	/* ... and check it */
> +	ret = fstat(fileno(txt_file), &txt_file_stat);

Why don't you simply use mmap() ?

> +	for (i = 0 ; i < txt_file_stat.st_size ; i++)
> +		if (envptr[i] == '\n')
> +			envptr[i] = '\0';

This is actually wrong.  Envrionment variables can have embedded
newlines.

> +	/* 
> +	 * Make sure there is a final '\0' (necessary if the padding byte isn't
> +	 * 0x00 or if there wasn't a newline at the end of the configuration

You did not take this into account in your file size check above, it
seems.

> +	 * file) Also, don't add it if the configuration file size is exactly
> +	 * the size of the environnment.

The double '\0' termination is _always_ needed.

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
You can observe a lot just by watchin'.                  - Yogi Berra