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

[Mike Frysinger]

On Fri, Aug 5, 2011 at 09:23, David Wagner wrote:
> +#include <endian.h>

this is not portable.  include compiler.h and use the already existing
uswap/cpu_to/to_cpu/etc... helpers.

> +static void usage(void)
> +{
> +       printf("envcrc [-h] [-r] [-b] -s <envrionnment partition size> -o <output> "

funny, "envcrc" isnt what the filename actually is ...

typo with "environment"

seems like this should also take a padding byte so people can use a
more sensible 0xff rather than 0x00

> +              "\t-b : the target is big endian (default is little endian)\n");
> +
> +}

kill useless newline before brace

> +       opterr = 0;

unnecessary ... punt it

> +               switch (option)
> +               {

cuddle the brace up

> +       /* Check the configuration file ...*/
> +       txt_filename = strdup(argv[optind]);

no need to strdup this.  argv isnt going anywhere.

> +       ret = stat(txt_filename, &txt_file_stat);
> +       if (ret == -1) {
> +               fprintf(stderr, "Can't stat() on configuration file: %s",
> +                               strerror(errno));
> +               return EXIT_FAILURE;
> +       }
> +       if (txt_file_stat.st_size > envsize) {
> +               fprintf(stderr, "The configuration file is larger than the "
> +                               "envrionnment partition size\n");
> +               return EXIT_FAILURE;
> +       }
> +
> +       /* ... and open it. */
> +       txt_file = fopen(txt_filename, "r");

fopen() it, then fstat() the fileno(txt_file) to avoid possible race
conditions.  also, use "rb".

> +       /* Read the raw configuration file and transform it */
> +       ret = fread(envptr, txt_file_stat.st_size, 1, txt_file);

you've swapped size and nemb args

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

you could use memchr() and a while loop instead ... but probably not
worth the effort

> +       ret = fclose(txt_file);

you could fclose() before the envptr conversion since you're done with
the file at that point

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

create a local uint32_t variable to set, then use memcpy to copy it
over to dataptr

> +       bin_file = fopen(bin_filename, "w");

"wb"

> +       if (fwrite(dataptr, 1, datasize, bin_file) != datasize) {

funny enough, you got the size/nemb args in the right order here ;)
-mike