Dear David Wagner,

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

s/nnm/nm/

> 
> Signed-off-by: David Wagner <david.wag...@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: w...@denx.de
You can observe a lot just by watchin'.                  - Yogi Berra
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to