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