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
