From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Frysinger Date: Thu, 2 Feb 2012 16:21:16 -0500 Subject: [U-Boot] [PATCH v7 4/4] EXYNOS: SMDK5250: Add MMC SPL support In-Reply-To: <1328173887-24983-5-git-send-email-chander.kashyap@linaro.org> References: <1328173887-24983-1-git-send-email-chander.kashyap@linaro.org> <1328173887-24983-5-git-send-email-chander.kashyap@linaro.org> Message-ID: <201202021621.19149.vapier@gentoo.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 \n", argv[0]); > + 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. > + 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]); > + 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 -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: This is a digitally signed message part. URL: