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

[Thomas Petazzoni <thomas.petazzoni@free-electrons.com>]

Hello,

Le Fri,  5 Aug 2011 16:49:58 +0200,
David Wagner <david.wagner@free-electrons.com> a ?crit :

> This tool takes a key=value configuration file (same as would a `printenv' show)
> and generates the corresponding environnment image, ready to be flashed.

Nice tool. I'm currently using a crappy shell-based equivalent of this,
but it'd be a lot better to have a clean and nice version integrated in
U-Boot. However, see some comments below:

> +	       "\t-r : the environnment is redundand\n"

"the environment has two copies in flash" might be clearer that this
"redundand" word maybe.

> +static int make_binary_config(FILE* txt_file, unsigned char *envptr, int envsize)
> +{
> +	int i;
> +	int ret;
> +
> +	ret = fread(envptr, envsize, 1, txt_file);
> +	for (i = 0 ; i < envsize ; i++)
> +		if (envptr[i] == '\n')
> +			envptr[i] = '\0';
> +
> +	return 0;
> +}

The name of the function sounds a bit strange, and the function's job
is really small. Maybe it should just be merged into the main function.

> +int main(int argc, char **argv)
> +{
> +	uint32_t crc;
> +	char *txt_filename = NULL, *bin_filename = NULL;
> +	FILE *txt_file, *bin_file;
> +	unsigned char *dataptr, *envptr;
> +	unsigned int envsize, datasize = 0;
> +	int bigendian = 0;
> +	int redundant = 0;
> +
> +	int option;
> +	int ret = EXIT_SUCCESS;
> +
> +	opterr = 0;
> +
> +
> +	/* Parse the cmdline */
> +	while ((option = getopt(argc, argv, "s:o:rbh")) != -1)

I'd prefer to have an opening brace here, even if there is technically
a single statement inside the while() loop.

> +		switch (option)
> +		{
> +		case 's':
> +			datasize = atoi(optarg);
> +			break;

It'd be nicer if sizes could be given in decimal *or* hexadecimal
formats. 0x20000 is much easier to type than 131072.

> +		default:
> +			if (bin_filename)
> +				free(bin_filename);

Don't try to do needless cleanup, and let the operating system do it
for you. It's a short-lived program, I don't think it's worth worrying
about cleaning-up things.

> +			fprintf(stderr, "Wrong option\n");
> +			usage();
> +			return EXIT_FAILURE;
> +		}
> +
> +	if (datasize == 0) {
> +		printf("Please specify the size of the envrionnment partition.\n");

                                                       ^^^^^ typo
The message should probably be printed to stderr:

		fprintf(stderr, "blabla\n");

> +		usage();
> +		ret = EXIT_FAILURE;
> +		goto out;

just return EXIT_FAILURE, not need to clean up.

> +	}
> +	
> +
> +	txt_filename = strdup(argv[optind]);
> +	if (!txt_filename) {
> +		ret = ENOMEM;
> +		goto out;

no need to clean up.

> +	}
> +
> +	txt_file = fopen(txt_filename, "r");
> +	if (!txt_file)
> +		goto out;

You should check whether the size of the environment given in the file
doesn't exceed the size of the environment passed on the command line.

> +	/* Read the raw configuration file and transform it */
> +	dataptr = calloc(datasize, 1);
> +	if (!dataptr)
> +		goto out;

No need to clean up.

> +	envsize = datasize - (CRC_SIZE + redundant);
> +	envptr = dataptr + CRC_SIZE + redundant;
> +
> +	ret = make_binary_config(txt_file, envptr, envsize);
> +	ret = fclose(txt_file);
> +
> +	crc = crc32(0, envptr, envsize);
> +	printf("crc: 0x%08X\n", crc);

I don't think printing the CRC is useful, just drop.

> +	*((uint32_t*) dataptr) = bigendian ? htobe32(crc) : htole32(crc);

I think it'd be better to have :

struct env_normal {
	uint32_t	crc;
	char		data[0];
}

struct env_redund {
	uint32_t	crc;
	char		flags;
	char		data[0];
}

rather than this cast.

> +	bin_file = fopen(bin_filename, "w");
> +	if (fwrite(dataptr, 1, datasize, bin_file) != datasize)
> +		fprintf(stderr, "fwrite() failed: %s\n", strerror(errno));

Missing exit with error here.

> +	ret = fclose(bin_file);
> +
> +out:
> +	if (txt_filename)
> +		free(txt_filename);
> +	if (bin_filename)
> +		free(bin_filename);
> +	return ret;

No need to do useless clean up.

Regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com