Patch / RFC: add "flashcp" applet

[Tito]

Hi,
just some ideas I had while looking at your patch,
hope they can help you to improve/optimize it.

Ciao,
Tito

PS.: progress() is used only in two sites, is it worth the hassle, couldn't it be inlined?


On Saturday 14 November 2009 20:00:44 Stefan Seyfried wrote:
> Hi,
> 
> the attached patch implements a "flashcp" similar the one form
> mtd-utils.
> 
> I have read the style-guide and I think that I did everything right.
> If so, please "git am", if not, please comment ;)
> 
> Have fun,
> 
> 	Stefan
> 

 
> int flashcp_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
> +int flashcp_main(int argc, char *argv[])
> +{
> +	int fd_f, fd_d; /* input file and mtd device file descriptors */
> +	int i, count;
> +	int filesize_kb;
> +	unsigned opts;
> +	size_t todo, done;
> +	ssize_t ret;
> +	struct mtd_info_user mtd;
> +	struct erase_info_user e;
> +	struct stat f;
> +	uint8_t buf[BUFSIZE], buf2[BUFSIZE];
> +
> +	opts = getopt32(argv, "rvh");
> +
> +	if (optind + 2 != argc || opts & FCP_OPT_h) {
> +		bb_show_usage();
> +	}
> +	argv += optind;
> +
> +	/* open input file and mtd device and do sanity checks */
> +	fd_d = xopen(argv[1], O_SYNC | O_RDWR);

	Use xioctl here?

> +	if (ioctl(fd_d, MEMGETINFO, &mtd) < 0) {
> +		bb_error_msg_and_die("%s is no valid MTD flash device", argv[1]);
> +	}
> +
> +	fd_f = xopen(argv[0], O_RDONLY);

	Use xstat here?
	xstat(argv[0], &f);

> +	if (fstat (fd_f, &f) < 0) {
> +		bb_error_msg_and_die("fstat");
> +	}
> +	if (f.st_size > mtd.size) {
> +		bb_error_msg_and_die("%s bigger than %s", argv[0], argv[1]);
> +	}
> +
	I know this is a gccism, still couldn't resist.... ;-)

	filesize_kb = ( f.st_size / 1024 ) ? : 1; 

> +	filesize_kb = f.st_size / 1024;
> +	if (filesize_kb == 0) {	/* wrong output if size is < 1k, but avoids division by zero */
> +		filesize_kb++;	/* in addition, this reduces the size ?!? (at least on i386) */ +	}
> +
> +	/* always erase a complete block */
> +	e.start = 0;
> +	e.length = mtd.erasesize;
> +	count = (f.st_size + mtd.erasesize - 1) / mtd.erasesize;
> +
	Maybe keep it simple here and skip this optimization to save size?

> +	/* optimization: if not verbose, erase in one go */
> +	if (!(opts & FCP_OPT_v)) {
> +		e.length = mtd.erasesize * count;
> +		count = 1;
> +	}
> +
> +	/* erase 1 block at a time to be able to give verbose output */
> +	e.length = mtd.erasesize;
> +	for (i = 1; i <= count; i++) {
> +		if (opts & FCP_OPT_v) {
> +			progress(0, i, count);
> +		}

		Use xioctl here ?

> +		if (ioctl(fd_d, MEMERASE, &e) < 0) {
> +			bb_perror_msg_and_die("MEMERASE blocks 0x%.8x-0x%.8x on %s",
> +				(unsigned)e.start, (unsigned)(e.start + e.length), argv[1]);
> +		}
> +		e.start += mtd.erasesize;
> +	}
> +	if (opts & FCP_OPT_v) {

		is puts("") smaller ? 

> +		printf("\n");
> +	}
> +
> +	/* doing this outer loop gives significantly smaller code
> +	   than doing two separate loops for writing and verifying */
> +	for (i = 1; i <= 2; i++) {
> +		/* not using xlseek here. Failure of the lseek will only hurt during
> +		   verification stage and xlseek is signifikantly bigger */
> +		lseek(fd_f, 0, SEEK_SET);
> +		lseek(fd_d, 0, SEEK_SET);
> +		todo = f.st_size;
> +		done = 0;
> +		count = BUFSIZE;
> +		while (todo) {
> +			if (todo < BUFSIZE) {
> +				count = todo;
> +			}
> +			if (opts & FCP_OPT_v) {
> +				progress(i, (done + count) / 1024, filesize_kb);
> +			}


			Use xread(int fd, void *buf, size_t count) here?

> +			ret = full_read(fd_f, buf, count);
> +			if (ret != count) {
> +				bb_perror_msg_and_die("short read of %s", argv[0]);
> +			}
> +			if (i == 1) {

				Use xwrite(int fd, const void *buf, size_t count) here ?

> +				ret = write(fd_d, buf, count);
> +				if (ret != count) {
> +					bb_perror_msg_and_die("while writing to x%.8x-0x%.8x on %s: "
> +						"%d/%lu bytes written to flash. returncode: %ld\n",
> +						done, done + count, argv[1], done + ret,
> +						(unsigned long)f.st_size, (long)ret);
> +				}
> +			} else { /* i == 2 */
> +				full_read(fd_d, buf2, count);

				No error checking on this full_read ? Use xread instead?

> +				if (memcmp(buf, buf2, count)) {

				Maybe shorten error message?

> +					bb_error_msg_and_die("file does not match flash data, "
> +						"first mismatch at 0x%.8x-0x%.8x\n",
> +						done, done + count);
> +				}
> +			}
> +
> +			done += count;
> +			todo -= count;
> +		}
> +
> +		if (opts & FCP_OPT_v) {

			is puts("") smaller ? 

> +			printf("\n");
> +		}
> +	}
> +
> +	/* we won't come here if there was an error */
> +	if (opts & FCP_OPT_r) {
	
		is puts("rebooting...") smaller ? 

> +		printf("rebooting...\n");
> +		sleep(1);
> +		reboot(RB_AUTOBOOT);
> +	}
> +
> +	return EXIT_SUCCESS;