Dear David Wagner,
In message <1318612616-16799-1-git-send-email-david.wag...@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: w...@denx.de
I am more bored than you could ever possibly be. Go back to work.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
