Hello,
Le Fri, 5 Aug 2011 16:49:58 +0200,
David Wagner <david.wag...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
