[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: Patch / RFC: add "flashcp" applet
From:       Denys Vlasenko <vda.linux () googlemail ! com>
Date:       2009-11-21 16:31:18
Message-ID: 200911211835.52171.vda.linux () googlemail ! com
[Download RAW message or body]

On Saturday 14 November 2009 20:00, 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 ;)

+       /* 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;

Bug: e.length will be wrong if !verbose.


+static void progress(int mode, int count, int total)

"int total"? This is 2009, 2 Gb limit is too tight.
There are many more instances where you don't pay attention
to the sizes of integers. int filesize_kb. size_t todo, done.



+      printf("\r%s: %d/%d (%d%%) "

"%d"? Signed percents and sizes?


               /* not using xlseek here. Failure of the lseek will only hurt during
                  verification stage and xlseek is significantly bigger */
               lseek(fd_f, 0, SEEK_SET);
               lseek(fd_d, 0, SEEK_SET);

But failure to *not act on seek error* is much worse!


                                       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, devicename, done + ret,
                                               (unsigned long)statb.st_size, (long)ret);

Too verbose (and again buggy wrt sizes), how about:
                                       bb_perror_msg_and_die("write error at 0x%"OFF_FMT"x on %s, "
                                               "write returned %d",
                                               done, devicename, ret);

+     "\n       -r      reboot after flashing" \

Upstream does not have -r option. You are adding incompatibility and bloat
by succumbing to featuritis.


     "\n        -h      this help message" \

Bloat.

     "\n        <filename>      file you want to copy to flash" \
     "\n        <device>        flash device to write (/dev/mtd0, ...)"

Well... we expect users to have a brain cell or two.



Applied to git with much editing, please pull and test.
--
vda
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]