Re: [PATCH 1/2] mtd: ubiformat: Add --confirm argument

[hujianyang <hujianyang@huawei.com>]

Hi Joe,

On 2014/12/31 22:58, Joe Balough wrote:
> Add support for an argument to ubiformat that will read back every
> written eraseblock and verify the read data matches the written data.
> 
> Signed-off-by: Joe Balough <jbb5044@gmail.com>
> ---
>  ubi-utils/ubiformat.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/ubi-utils/ubiformat.c b/ubi-utils/ubiformat.c
> index 21409ca..4129404 100644
> --- a/ubi-utils/ubiformat.c
> +++ b/ubi-utils/ubiformat.c
> @@ -49,6 +49,7 @@
>  
>  /* The variables below are set by command line arguments */
>  struct args {
> +	unsigned int confirm:1;
>  	unsigned int yes:1;
>  	unsigned int quiet:1;
>  	unsigned int verbose:1;
> @@ -93,6 +94,7 @@ static const char optionsstr[] =
>  "                             (default is 1)\n"
>  "-Q, --image-seq=<num>        32-bit UBI image sequence number to use\n"
>  "                             (by default a random number is picked)\n"
> +"-c, --confirm                Read back written value and verify it matches\n"
>  "-y, --yes                    assume the answer is \"yes\" for all question\n"
>  "                             this program would otherwise ask\n"
>  "-q, --quiet                  suppress progress percentage information\n"
> @@ -105,8 +107,8 @@ static const char usage[] =
>  "\t\t\t[-Q <num>] [-f <file>] [-S <bytes>] [-e <value>] [-x <num>] [-y] [-q] [-v] [-h]\n"
>  "\t\t\t[--sub-page-size=<bytes>] [--vid-hdr-offset=<offs>] [--no-volume-table]\n"
>  "\t\t\t[--flash-image=<file>] [--image-size=<bytes>] [--erase-counter=<value>]\n"
> -"\t\t\t[--image-seq=<num>] [--ubi-ver=<num>] [--yes] [--quiet] [--verbose]\n"
> -"\t\t\t[--help] [--version]\n\n"
> +"\t\t\t[--image-seq=<num>] [--ubi-ver=<num>] [--confirm] [--yes] [--quiet]\n"
> +"\t\t\t[--verbose] [--help] [--version]\n\n"
>  "Example 1: " PROGRAM_NAME " /dev/mtd0 -y - format MTD device number 0 and do\n"
>  "           not ask questions.\n"
>  "Example 2: " PROGRAM_NAME " /dev/mtd0 -q -e 0 - format MTD device number 0,\n"
> @@ -118,6 +120,7 @@ static const struct option long_options[] = {
>  	{ .name = "no-volume-table", .has_arg = 0, .flag = NULL, .val = 'n' },
>  	{ .name = "flash-image",     .has_arg = 1, .flag = NULL, .val = 'f' },
>  	{ .name = "image-size",      .has_arg = 1, .flag = NULL, .val = 'S' },
> +	{ .name = "confirm",         .has_arg = 0, .flag = NULL, .val = 'c' },
>  	{ .name = "yes",             .has_arg = 0, .flag = NULL, .val = 'y' },
>  	{ .name = "erase-counter",   .has_arg = 1, .flag = NULL, .val = 'e' },
>  	{ .name = "quiet",           .has_arg = 0, .flag = NULL, .val = 'q' },
> @@ -137,7 +140,7 @@ static int parse_opt(int argc, char * const argv[])
>  		int key, error = 0;
>  		unsigned long int image_seq;
>  
> -		key = getopt_long(argc, argv, "nh?Vyqve:x:s:O:f:S:", long_options, NULL);
> +		key = getopt_long(argc, argv, "nh?Vycqve:x:s:O:f:S:", long_options, NULL);
>  		if (key == -1)
>  			break;
>  
> @@ -178,6 +181,10 @@ static int parse_opt(int argc, char * const argv[])
>  		case 'n':
>  			args.novtbl = 1;
>  			break;
> +			
> +		case 'c':
> +			args.confirm = 1;
> +			break;
>  
>  		case 'y':
>  			args.yes = 1;

This option 'c' seems only use when *flash-image* is specified. So maybe you
can check the validity of this option depending on *args.image* in parse_opt().

> @@ -535,6 +542,20 @@ static int flash_image(libmtd_t libmtd, const struct mtd_dev_info *mtd,
>  			skip_data_read = 1;
>  			continue;
>  		}
> +		
> +		if (args.confirm) {
> +			char read_buf[mtd->eb_size];
> +			err = mtd_read(mtd, args.node_fd, eb, 0, read_buf, new_len);
> +			if (err) {
> +				sys_errmsg("cannot readback eraseblock %d for confirmation", eb);
> +				goto out_close;
> +			}
> +			
> +			if (memcmp(buf, read_buf, new_len) != 0) {
> +				sys_errmsg("readback failed on eraseblock %d", eb);
> +			}
> +		}
> +		
>  		if (++written_ebs >= img_ebs)
>  			break;
>  	}
> 

Do we have a function like *mtd_write_and_check()*? I think the operation
first write and then check it by reading may be used in many cases.

It's worthy to check the writing data of the *flash-image* without any
additional options in my considering.

Thanks,
Hu