Hi Mike,
On 3 February 2012 02:51, Mike Frysinger <vap...@gentoo.org> wrote:
> On Thursday 02 February 2012 04:11:27 Chander Kashyap wrote:
>> --- /dev/null
>> +++ b/board/samsung/smdk5250/tools/mkexynos_image.c
>
> tools should be in tools/. there are already plenty of examples in there (see
> tools/msxboot.c as the first example i found by reading tools/Makefile).
>
>> +int main(int argc, char **argv)
>> +{
>> ...
>> + unsigned char buffer[BUFSIZE] = {0};
>
> this is an implicit memset() and from what i can see in the code, useless.
> you read() the entire buffer, so there's no need to initialize it.
>
>> + if (argc != 3) {
>> + printf(" %d Wrong number of arguments\n", argc);
>
> this should tell the user how to use the tool.
> fprintf(stderr, "Usage: %s <infile> <outfile>\n", argv[0]);
Yes That's better.
>
>> + if (ifd)
>> + close(ifd);
>
> this if() is wrong (0 is a valid fd) and useless (you already abort if ifd did
> not succeed). just delete the if statement.
Ah yes. I will fix it.
>
>> + len = lseek(ifd, 0, SEEK_END);
>> + lseek(ifd, 0, SEEK_SET);
>
> lazy man's stat() :P. just use stat(). and change the type of "len" to
> "off_t".
>
>> + count = (len < CHECKSUM_OFFSET) ? len : CHECKSUM_OFFSET;
>> +
>> + if (read(ifd, buffer, count) != count) {
>
> count should be a ssize_t. although, this doesn't handle partial interrupted
> reads, so i wonder if this could shouldn't just be changed to use stdio
> fopen/fread. probably would be simpler that way too.
>
>> + if (ifd)
>> + close(ifd);
>> + if (ofd)
>> + close(ofd);
>
> these if checks are wrong for the same reason mentioned above
>
>> + unsigned long checksum = 0;
>> ...
>> + for (i = 0, checksum = 0; i < CHECKSUM_OFFSET; i++)
>> + checksum += buffer[i];
>> + memcpy(&buffer[CHECKSUM_OFFSET], &checksum, sizeof(checksum));
>
> pretty sure this fails if this tool is run on a big-endian machine, as well as
> 64bit vs 32bit. change the type of "checksum" to uint32_t, then use something
> like:
> put_unaligned_le32(checksum, &buffer[CHECKSUM_OFFSET]);
>
Will take care of it.
>> + if (write(ofd, buffer, BUFSIZE) != BUFSIZE) {
>
> same issues as the read() mentioned above
>
>> + if (ifd)
>> + close(ifd);
>> + if (ofd)
>> + close(ofd);
>
> same bad if() logic
>
>> + if (ifd)
>> + close(ifd);
>> + if (ofd)
>> + close(ofd);
>
> same bad if() logic
> -mike
--
with warm regards,
Chander Kashyap
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
