[PATCH v2 0/6] i2cset changes

[PATCH v2 0/6] i2cset changes

[Guenter Roeck]

The following series of patches removes the obsolete means to specify the value mask,
improves parameter error detection for the i2cset command, and cleans up the code a bit.

I switched to git for patch management, thus the patch diffs were created with
"git format-patch" instead of "svn diff". Hope that doesn't cause any problems.

v2:
- Split single patch into multiple individual patches
- Included code review feedback

[PATCH v2 1/6] i2cset: Remove deprecated method to provide the value mask

[Guenter Roeck]

The old method to provide the value mask has long since been deprecated,
remove it.
---
 CHANGES        |    1 +
 tools/i2cset.c |   12 ------------
 2 files changed, 1 insertions(+), 12 deletions(-)

diff --git a/CHANGES b/CHANGES
index 5aee418..999ff26 100644
--- a/CHANGES
+++ b/CHANGES
@@ -4,6 +4,7 @@ i2c-tools CHANGES
 SVN
   i2c-dev.h: Make value arrays const for block write functions
   i2cset: Add support for SMBus and I2C block writes
+          Remove obsolete means to specify value mask
 
 3.0.3 (2010-12-12)
   Makefile: Let the environment set CC and CFLAGS
diff --git a/tools/i2cset.c b/tools/i2cset.c
index 392262b..8856d71 100644
--- a/tools/i2cset.c
+++ b/tools/i2cset.c
@@ -265,18 +265,6 @@ int main(int argc, char *argv[])
 		pec = argv[flags+5][1] == 'p';
 	}
 
-	/* Old method to provide the value mask, deprecated and no longer
-	   documented but still supported for compatibility */
-	if (argc > flags + 6) {
-		if (maskp) {
-			fprintf(stderr, "Error: Data value mask provided twice!\n");
-			help();
-		}
-		fprintf(stderr, "Warning: Using deprecated way to set the data value mask!\n");
-		fprintf(stderr, "         Please switch to using -m.\n");
-		maskp = argv[flags+6];
-	}
-
 	if (maskp) {
 		vmask = strtol(maskp, &end, 0);
 		if (*end || vmask == 0) {
-- 
1.7.0.4

Re: [PATCH v2 1/6] i2cset: Remove deprecated method to provide the value mask

[Jean Delvare]

Hi Guenter,

Wow, 6 patches :)

On Sun, 13 Feb 2011 12:17:21 -0800, Guenter Roeck wrote:
> The old method to provide the value mask has long since been deprecated,
> remove it.
> ---
>  CHANGES        |    1 +
>  tools/i2cset.c |   12 ------------
>  2 files changed, 1 insertions(+), 12 deletions(-)
> 
> diff --git a/CHANGES b/CHANGES
> index 5aee418..999ff26 100644
> --- a/CHANGES
> +++ b/CHANGES
> @@ -4,6 +4,7 @@ i2c-tools CHANGES
>  SVN
>    i2c-dev.h: Make value arrays const for block write functions
>    i2cset: Add support for SMBus and I2C block writes
> +          Remove obsolete means to specify value mask
>  
>  3.0.3 (2010-12-12)
>    Makefile: Let the environment set CC and CFLAGS
> diff --git a/tools/i2cset.c b/tools/i2cset.c
> index 392262b..8856d71 100644
> --- a/tools/i2cset.c
> +++ b/tools/i2cset.c
> @@ -265,18 +265,6 @@ int main(int argc, char *argv[])
>  		pec = argv[flags+5][1] == 'p';
>  	}
>  
> -	/* Old method to provide the value mask, deprecated and no longer
> -	   documented but still supported for compatibility */
> -	if (argc > flags + 6) {
> -		if (maskp) {
> -			fprintf(stderr, "Error: Data value mask provided twice!\n");
> -			help();
> -		}
> -		fprintf(stderr, "Warning: Using deprecated way to set the data value mask!\n");
> -		fprintf(stderr, "         Please switch to using -m.\n");
> -		maskp = argv[flags+6];
> -	}
> -
>  	if (maskp) {
>  		vmask = strtol(maskp, &end, 0);
>  		if (*end || vmask == 0) {

This looks good, however I think the code can be cleaned up further by
getting rid of maskp. I introduced maskp [1] because the mask could be
at two different places on the command line, but now it is no longer
needed.

[1] http://www.lm-sensors.org/changeset/5390

Also, I see that we don't do range checks on the mask value. As you are
in the process to make command line parsing more strict, it would
probably be a good idea to add range changes on the mask value too.

-- 
Jean Delvare

Re: [PATCH v2 1/6] i2cset: Remove deprecated method to provide the value mask

[Guenter Roeck]

On Mon, Feb 14, 2011 at 03:48:47AM -0500, Jean Delvare wrote:
> Hi Guenter,
> 
> Wow, 6 patches :)
> 
Just because I switched to git. Makes it much easier to me to handle 
a sequence of patches. Yes, I know, it probably works with svn as well.
Just a personal preference ...

> On Sun, 13 Feb 2011 12:17:21 -0800, Guenter Roeck wrote:
> > The old method to provide the value mask has long since been deprecated,
> > remove it.
> > ---
> >  CHANGES        |    1 +
> >  tools/i2cset.c |   12 ------------
> >  2 files changed, 1 insertions(+), 12 deletions(-)
> > 
> > diff --git a/CHANGES b/CHANGES
> > index 5aee418..999ff26 100644
> > --- a/CHANGES
> > +++ b/CHANGES
> > @@ -4,6 +4,7 @@ i2c-tools CHANGES
> >  SVN
> >    i2c-dev.h: Make value arrays const for block write functions
> >    i2cset: Add support for SMBus and I2C block writes
> > +          Remove obsolete means to specify value mask
> >  
> >  3.0.3 (2010-12-12)
> >    Makefile: Let the environment set CC and CFLAGS
> > diff --git a/tools/i2cset.c b/tools/i2cset.c
> > index 392262b..8856d71 100644
> > --- a/tools/i2cset.c
> > +++ b/tools/i2cset.c
> > @@ -265,18 +265,6 @@ int main(int argc, char *argv[])
> >  		pec = argv[flags+5][1] == 'p';
> >  	}
> >  
> > -	/* Old method to provide the value mask, deprecated and no longer
> > -	   documented but still supported for compatibility */
> > -	if (argc > flags + 6) {
> > -		if (maskp) {
> > -			fprintf(stderr, "Error: Data value mask provided twice!\n");
> > -			help();
> > -		}
> > -		fprintf(stderr, "Warning: Using deprecated way to set the data value mask!\n");
> > -		fprintf(stderr, "         Please switch to using -m.\n");
> > -		maskp = argv[flags+6];
> > -	}
> > -
> >  	if (maskp) {
> >  		vmask = strtol(maskp, &end, 0);
> >  		if (*end || vmask == 0) {
> 
> This looks good, however I think the code can be cleaned up further by
> getting rid of maskp. I introduced maskp [1] because the mask could be
> at two different places on the command line, but now it is no longer
> needed.
> 
Not sure how, though, since we either need the index to argv[mask] or the pointer, 
or we would have to read the mask while checking the parameters. The latter
doesn't look very clean to me.

> [1] http://www.lm-sensors.org/changeset/5390
> 
> Also, I see that we don't do range checks on the mask value. As you are
> in the process to make command line parsing more strict, it would
> probably be a good idea to add range changes on the mask value too.
> 
Yes, I'll add that.

Guenter

Re: [PATCH v2 1/6] i2cset: Remove deprecated method to provide the value mask

[Jean Delvare]

On Mon, 14 Feb 2011 07:46:10 -0800, Guenter Roeck wrote:
> On Mon, Feb 14, 2011 at 03:48:47AM -0500, Jean Delvare wrote:
> > Hi Guenter,
> > 
> > Wow, 6 patches :)
> > 
> Just because I switched to git. Makes it much easier to me to handle 
> a sequence of patches. Yes, I know, it probably works with svn as well.
> Just a personal preference ...

Actually, no, SVN would be horrible for this I think. Quilt, OTOH,
would work just fine.

> > (...)
> > This looks good, however I think the code can be cleaned up further by
> > getting rid of maskp. I introduced maskp [1] because the mask could be
> > at two different places on the command line, but now it is no longer
> > needed.
>
> Not sure how, though, since we either need the index to argv[mask] or the pointer, 
> or we would have to read the mask while checking the parameters. The latter
> doesn't look very clean to me.

The latter is actually what I had in mind. But if you don't like the
idea, just leave the code as it is.

-- 
Jean Delvare

Re: [PATCH v2 1/6] i2cset: Remove deprecated method to provide the value mask

[Guenter Roeck]

Hi Jean,

On Mon, Feb 14, 2011 at 11:17:35AM -0500, Jean Delvare wrote:
> On Mon, 14 Feb 2011 07:46:10 -0800, Guenter Roeck wrote:
> > On Mon, Feb 14, 2011 at 03:48:47AM -0500, Jean Delvare wrote:
> > > Hi Guenter,
> > > 
> > > Wow, 6 patches :)
> > > 
> > Just because I switched to git. Makes it much easier to me to handle 
> > a sequence of patches. Yes, I know, it probably works with svn as well.
> > Just a personal preference ...
> 
> Actually, no, SVN would be horrible for this I think. Quilt, OTOH,
> would work just fine.
> 
For some reason, Quilt and me seem to be at odds with each other. No idea
why. I tried, but it just doesn't work for me. Maybe it is too simple for me
to understand ;).

> > > (...)
> > > This looks good, however I think the code can be cleaned up further by
> > > getting rid of maskp. I introduced maskp [1] because the mask could be
> > > at two different places on the command line, but now it is no longer
> > > needed.
> >
> > Not sure how, though, since we either need the index to argv[mask] or the pointer, 
> > or we would have to read the mask while checking the parameters. The latter
> > doesn't look very clean to me.
> 
> The latter is actually what I had in mind. But if you don't like the
> idea, just leave the code as it is.
> 
Problem with that is that I can not check the value range at that time, and
I don't know if mask is a valid parameter to start with. So there would have to
be a value check later on anyway (which I just agreed to add). I think it is
better to keep reading the value and the range check together.

Thanks,
Guenter

[PATCH v2 2/6] i2cset: Check number of arguments for block data writes

[Guenter Roeck]

Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
---
 tools/i2cset.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/tools/i2cset.c b/tools/i2cset.c
index 8856d71..c59186b 100644
--- a/tools/i2cset.c
+++ b/tools/i2cset.c
@@ -219,11 +219,15 @@ int main(int argc, char *argv[])
 		}
 		if (size == I2C_SMBUS_BLOCK_DATA || size == I2C_SMBUS_I2C_BLOCK_DATA) {
 			pec = argv[argc-1][1] == 'p';
+			if (argc > (int)sizeof(block) + flags + 5) {
+				fprintf(stderr, "Error: Bad number of arguments!\n");
+				help();
+			}
 			if (pec && size == I2C_SMBUS_I2C_BLOCK_DATA) {
 				fprintf(stderr, "Error: PEC not supported for I2C block writes!\n");
 				help();
 			}
-			for (len = 0; len < (int)sizeof(block) && len + flags + 5 < argc; len++) {
+			for (len = 0; len + flags + 5 < argc; len++) {
 				value = strtol(argv[flags + len + 4], &end, 0);
 				if (*end || value < 0 || value > 0xff) {
                                 	fprintf(stderr, "Error: Block data value invalid!\n");
-- 
1.7.0.4

Re: [PATCH v2 2/6] i2cset: Check number of arguments for block data writes

[Jean Delvare]

On Sun, 13 Feb 2011 12:17:22 -0800, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
> ---
>  tools/i2cset.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/i2cset.c b/tools/i2cset.c
> index 8856d71..c59186b 100644
> --- a/tools/i2cset.c
> +++ b/tools/i2cset.c
> @@ -219,11 +219,15 @@ int main(int argc, char *argv[])
>  		}
>  		if (size == I2C_SMBUS_BLOCK_DATA || size == I2C_SMBUS_I2C_BLOCK_DATA) {
>  			pec = argv[argc-1][1] == 'p';
> +			if (argc > (int)sizeof(block) + flags + 5) {
> +				fprintf(stderr, "Error: Bad number of arguments!\n");
> +				help();
> +			}
>  			if (pec && size == I2C_SMBUS_I2C_BLOCK_DATA) {
>  				fprintf(stderr, "Error: PEC not supported for I2C block writes!\n");
>  				help();
>  			}

It would probably be more logical to swap the two blocks above, as you
set pec just before and are reading the arguments just after.

> -			for (len = 0; len < (int)sizeof(block) && len + flags + 5 < argc; len++) {
> +			for (len = 0; len + flags + 5 < argc; len++) {
>  				value = strtol(argv[flags + len + 4], &end, 0);
>  				if (*end || value < 0 || value > 0xff) {
>                                  	fprintf(stderr, "Error: Block data value invalid!\n");

Other than this, it looks good.

-- 
Jean Delvare

[PATCH v2 3/6] i2cset: Abort if value mask is set for block commands

[Guenter Roeck]

Specifying the value mask is not supported for block commands,
abort if it is specified anyway.

Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
---
 tools/i2cset.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/tools/i2cset.c b/tools/i2cset.c
index c59186b..bdc16f8 100644
--- a/tools/i2cset.c
+++ b/tools/i2cset.c
@@ -227,6 +227,10 @@ int main(int argc, char *argv[])
 				fprintf(stderr, "Error: PEC not supported for I2C block writes!\n");
 				help();
 			}
+			if (maskp) {
+				fprintf(stderr, "Error: Mask not supported for block writes!\n");
+				help();
+			}
 			for (len = 0; len + flags + 5 < argc; len++) {
 				value = strtol(argv[flags + len + 4], &end, 0);
 				if (*end || value < 0 || value > 0xff) {
-- 
1.7.0.4

Re: [PATCH v2 3/6] i2cset: Abort if value mask is set for block commands

[Jean Delvare]

On Sun, 13 Feb 2011 12:17:23 -0800, Guenter Roeck wrote:
> Specifying the value mask is not supported for block commands,
> abort if it is specified anyway.
> 
> Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
> ---
>  tools/i2cset.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/i2cset.c b/tools/i2cset.c
> index c59186b..bdc16f8 100644
> --- a/tools/i2cset.c
> +++ b/tools/i2cset.c
> @@ -227,6 +227,10 @@ int main(int argc, char *argv[])
>  				fprintf(stderr, "Error: PEC not supported for I2C block writes!\n");
>  				help();
>  			}
> +			if (maskp) {
> +				fprintf(stderr, "Error: Mask not supported for block writes!\n");
> +				help();
> +			}
>  			for (len = 0; len + flags + 5 < argc; len++) {
>  				value = strtol(argv[flags + len + 4], &end, 0);
>  				if (*end || value < 0 || value > 0xff) {

Yes, good check.

-- 
Jean Delvare

[PATCH v2 4/6] i2cset: Ensure that there is no junk after the command/mode parameter

[Guenter Roeck]

Since the value mask can no longer be set after the mode command,
we can make sure that there is no junk after the command/mode
parameter in the command line.

Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
---
 CHANGES        |    1 +
 tools/i2cset.c |    3 +++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/CHANGES b/CHANGES
index 999ff26..1ffe18f 100644
--- a/CHANGES
+++ b/CHANGES
@@ -5,6 +5,7 @@ SVN
   i2c-dev.h: Make value arrays const for block write functions
   i2cset: Add support for SMBus and I2C block writes
           Remove obsolete means to specify value mask
+          More stringent parameter validation
 
 3.0.3 (2010-12-12)
   Makefile: Let the environment set CC and CFLAGS
diff --git a/tools/i2cset.c b/tools/i2cset.c
index bdc16f8..ad7da75 100644
--- a/tools/i2cset.c
+++ b/tools/i2cset.c
@@ -240,6 +240,9 @@ int main(int argc, char *argv[])
 				block[len] = value;
 			}
 			goto dofile;
+		} else if (argc != flags + 6) {
+			fprintf(stderr, "Error: Bad number of arguments!\n");
+			help();
 		}
 	}
 
-- 
1.7.0.4

Re: [PATCH v2 4/6] i2cset: Ensure that there is no junk after the command/mode parameter

[Jean Delvare]

On Sun, 13 Feb 2011 12:17:24 -0800, Guenter Roeck wrote:
> Since the value mask can no longer be set after the mode command,
> we can make sure that there is no junk after the command/mode
> parameter in the command line.
> 
> Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
> ---
>  CHANGES        |    1 +
>  tools/i2cset.c |    3 +++
>  2 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/CHANGES b/CHANGES
> index 999ff26..1ffe18f 100644
> --- a/CHANGES
> +++ b/CHANGES
> @@ -5,6 +5,7 @@ SVN
>    i2c-dev.h: Make value arrays const for block write functions
>    i2cset: Add support for SMBus and I2C block writes
>            Remove obsolete means to specify value mask
> +          More stringent parameter validation
>  
>  3.0.3 (2010-12-12)
>    Makefile: Let the environment set CC and CFLAGS
> diff --git a/tools/i2cset.c b/tools/i2cset.c
> index bdc16f8..ad7da75 100644
> --- a/tools/i2cset.c
> +++ b/tools/i2cset.c
> @@ -240,6 +240,9 @@ int main(int argc, char *argv[])
>  				block[len] = value;
>  			}
>  			goto dofile;
> +		} else if (argc != flags + 6) {
> +			fprintf(stderr, "Error: Bad number of arguments!\n");
> +			help();
>  		}
>  	}

Good idea. But I think you can even be more explicit and say "Too many
arguments!".

-- 
Jean Delvare

[PATCH v2 5/6] i2cset: Replace blanks at beginning of line with tabs

[Guenter Roeck]

Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
---
 tools/i2cset.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/i2cset.c b/tools/i2cset.c
index ad7da75..33290ef 100644
--- a/tools/i2cset.c
+++ b/tools/i2cset.c
@@ -35,7 +35,7 @@ static void help(void) __attribute__ ((noreturn));
 static void help(void)
 {
 	fprintf(stderr,
-	        "Usage: i2cset [-f] [-y] [-m MASK] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] ... [MODE]\n"
+		"Usage: i2cset [-f] [-y] [-m MASK] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] ... [MODE]\n"
 		"  I2CBUS is an integer or an I2C bus name\n"
 		"  ADDRESS is an integer (0x03 - 0x77)\n"
 		"  MODE is one of:\n"
-- 
1.7.0.4

Re: [PATCH v2 5/6] i2cset: Replace blanks at beginning of line with tabs

[Jean Delvare]

On Sun, 13 Feb 2011 12:17:25 -0800, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
> ---
>  tools/i2cset.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/i2cset.c b/tools/i2cset.c
> index ad7da75..33290ef 100644
> --- a/tools/i2cset.c
> +++ b/tools/i2cset.c
> @@ -35,7 +35,7 @@ static void help(void) __attribute__ ((noreturn));
>  static void help(void)
>  {
>  	fprintf(stderr,
> -	        "Usage: i2cset [-f] [-y] [-m MASK] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] ... [MODE]\n"
> +		"Usage: i2cset [-f] [-y] [-m MASK] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] ... [MODE]\n"
>  		"  I2CBUS is an integer or an I2C bus name\n"
>  		"  ADDRESS is an integer (0x03 - 0x77)\n"
>  		"  MODE is one of:\n"

I had noticed that one, yes. Obviously correct, please commit.

-- 
Jean Delvare

[PATCH v2 6/6] i2cset: Get command/mode before reading data

[Guenter Roeck]

Get and validate the command/mode parameter for all commands
before reading and evaluating actual data.

Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
---
 tools/i2cset.c |   99 +++++++++++++++++++++++++++++++-------------------------
 1 files changed, 55 insertions(+), 44 deletions(-)

diff --git a/tools/i2cset.c b/tools/i2cset.c
index 33290ef..d8d51a2 100644
--- a/tools/i2cset.c
+++ b/tools/i2cset.c
@@ -207,18 +207,37 @@ int main(int argc, char *argv[])
 		help();
 	}
 
-	/* check for block data */
-	len = 0;
-	if (argc > flags + 5) {
+	/* check for command/mode */
+	if (argc == flags + 4) {
+		/* Implicit "c" */
+		size = I2C_SMBUS_BYTE;
+	} else if (argc == flags + 5) {
+		/* "c", "cp",  or implicit "b" */
+		if (!strcmp(argv[flags+4], "c")
+		 || !strcmp(argv[flags+4], "cp")) {
+			size = I2C_SMBUS_BYTE;
+			pec = argv[flags+4][1] == 'p';
+		} else {
+			size = I2C_SMBUS_BYTE_DATA;
+		}
+	} else {
+		/* All other commands */
+		if (strlen(argv[argc-1]) > 2
+		    || (strlen(argv[argc-1]) == 2 && argv[argc-1][1] != 'p')) {
+			fprintf(stderr, "Error: Invalid mode '%s'!\n", argv[argc-1]);
+			help();
+		}
 		switch (argv[argc-1][0]) {
+		case 'b': size = I2C_SMBUS_BYTE_DATA; break;
+		case 'w': size = I2C_SMBUS_WORD_DATA; break;
 		case 's': size = I2C_SMBUS_BLOCK_DATA; break;
 		case 'i': size = I2C_SMBUS_I2C_BLOCK_DATA; break;
 		default:
-			size = 0;
-			break;
+			fprintf(stderr, "Error: Invalid mode '%s'!\n", argv[argc-1]);
+			help();
 		}
+		pec = argv[argc-1][1] == 'p';
 		if (size == I2C_SMBUS_BLOCK_DATA || size == I2C_SMBUS_I2C_BLOCK_DATA) {
-			pec = argv[argc-1][1] == 'p';
 			if (argc > (int)sizeof(block) + flags + 5) {
 				fprintf(stderr, "Error: Bad number of arguments!\n");
 				help();
@@ -231,49 +250,48 @@ int main(int argc, char *argv[])
 				fprintf(stderr, "Error: Mask not supported for block writes!\n");
 				help();
 			}
-			for (len = 0; len + flags + 5 < argc; len++) {
-				value = strtol(argv[flags + len + 4], &end, 0);
-				if (*end || value < 0 || value > 0xff) {
-                                	fprintf(stderr, "Error: Block data value invalid!\n");
-                                	help();
-                        	}
-				block[len] = value;
-			}
-			goto dofile;
 		} else if (argc != flags + 6) {
 			fprintf(stderr, "Error: Bad number of arguments!\n");
 			help();
 		}
 	}
 
-	if (argc > flags + 4) {
-		if (!strcmp(argv[flags+4], "c")
-		 || !strcmp(argv[flags+4], "cp")) {
-			size = I2C_SMBUS_BYTE;
-			value = -1;
-			pec = argv[flags+4][1] == 'p';
-		} else {
-			size = I2C_SMBUS_BYTE_DATA;
-			value = strtol(argv[flags+4], &end, 0);
+	len = 0; /* Must always initialize len since it is passed to confirm() */
+
+	/* read values from command line */
+	switch (size) {
+	case I2C_SMBUS_BYTE_DATA:
+	case I2C_SMBUS_WORD_DATA:
+		value = strtol(argv[flags+4], &end, 0);
+		if (*end || value < 0) {
+			fprintf(stderr, "Error: Data value invalid!\n");
+			help();
+		}
+		if ((size == I2C_SMBUS_BYTE_DATA && value > 0xff)
+		    || (size == I2C_SMBUS_WORD_DATA && value > 0xffff)) {
+			fprintf(stderr, "Error: Data value out of range!\n");
+			help();
+		}
+		break;
+	case I2C_SMBUS_BLOCK_DATA:
+	case I2C_SMBUS_I2C_BLOCK_DATA:
+		for (len = 0; len + flags + 5 < argc; len++) {
+			value = strtol(argv[flags + len + 4], &end, 0);
 			if (*end || value < 0) {
 				fprintf(stderr, "Error: Data value invalid!\n");
 				help();
 			}
+			if (value > 0xff) {
+				fprintf(stderr, "Error: Data value out of range!\n");
+				help();
+			}
+			block[len] = value;
 		}
-	} else {
-		size = I2C_SMBUS_BYTE;
 		value = -1;
-	}
-
-	if (argc > flags + 5) {
-		switch (argv[flags+5][0]) {
-		case 'b': size = I2C_SMBUS_BYTE_DATA; break;
-		case 'w': size = I2C_SMBUS_WORD_DATA; break;
-		default:
-			fprintf(stderr, "Error: Invalid mode!\n");
-			help();
-		}
-		pec = argv[flags+5][1] == 'p';
+		break;
+	default:
+		value = -1;
+		break;
 	}
 
 	if (maskp) {
@@ -284,13 +302,6 @@ int main(int argc, char *argv[])
 		}
 	}
 
-	if ((size == I2C_SMBUS_BYTE_DATA && value > 0xff)
-	 || (size == I2C_SMBUS_WORD_DATA && value > 0xffff)) {
-		fprintf(stderr, "Error: Data value out of range!\n");
-		help();
-	}
-
-dofile:
 	file = open_i2c_dev(i2cbus, filename, sizeof(filename), 0);
 	if (file < 0
 	 || check_funcs(file, size, pec)
-- 
1.7.0.4

Re: [PATCH v2 6/6] i2cset: Get command/mode before reading data

[Jean Delvare]

On Sun, 13 Feb 2011 12:17:26 -0800, Guenter Roeck wrote:
> Get and validate the command/mode parameter for all commands
> before reading and evaluating actual data.
> 
> Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
> ---
>  tools/i2cset.c |   99 +++++++++++++++++++++++++++++++-------------------------
>  1 files changed, 55 insertions(+), 44 deletions(-)
> 
> diff --git a/tools/i2cset.c b/tools/i2cset.c
> index 33290ef..d8d51a2 100644
> --- a/tools/i2cset.c
> +++ b/tools/i2cset.c
> @@ -207,18 +207,37 @@ int main(int argc, char *argv[])
>  		help();
>  	}
>  
> -	/* check for block data */
> -	len = 0;
> -	if (argc > flags + 5) {
> +	/* check for command/mode */
> +	if (argc == flags + 4) {
> +		/* Implicit "c" */
> +		size = I2C_SMBUS_BYTE;
> +	} else if (argc == flags + 5) {
> +		/* "c", "cp",  or implicit "b" */
> +		if (!strcmp(argv[flags+4], "c")
> +		 || !strcmp(argv[flags+4], "cp")) {
> +			size = I2C_SMBUS_BYTE;
> +			pec = argv[flags+4][1] == 'p';
> +		} else {
> +			size = I2C_SMBUS_BYTE_DATA;
> +		}
> +	} else {
> +		/* All other commands */
> +		if (strlen(argv[argc-1]) > 2
> +		    || (strlen(argv[argc-1]) == 2 && argv[argc-1][1] != 'p')) {
> +			fprintf(stderr, "Error: Invalid mode '%s'!\n", argv[argc-1]);
> +			help();
> +		}
>  		switch (argv[argc-1][0]) {
> +		case 'b': size = I2C_SMBUS_BYTE_DATA; break;
> +		case 'w': size = I2C_SMBUS_WORD_DATA; break;
>  		case 's': size = I2C_SMBUS_BLOCK_DATA; break;
>  		case 'i': size = I2C_SMBUS_I2C_BLOCK_DATA; break;
>  		default:
> -			size = 0;
> -			break;
> +			fprintf(stderr, "Error: Invalid mode '%s'!\n", argv[argc-1]);
> +			help();
>  		}
> +		pec = argv[argc-1][1] == 'p';
>  		if (size == I2C_SMBUS_BLOCK_DATA || size == I2C_SMBUS_I2C_BLOCK_DATA) {
> -			pec = argv[argc-1][1] == 'p';
>  			if (argc > (int)sizeof(block) + flags + 5) {
>  				fprintf(stderr, "Error: Bad number of arguments!\n");
>  				help();
> @@ -231,49 +250,48 @@ int main(int argc, char *argv[])
>  				fprintf(stderr, "Error: Mask not supported for block writes!\n");
>  				help();
>  			}
> -			for (len = 0; len + flags + 5 < argc; len++) {
> -				value = strtol(argv[flags + len + 4], &end, 0);
> -				if (*end || value < 0 || value > 0xff) {
> -                                	fprintf(stderr, "Error: Block data value invalid!\n");
> -                                	help();
> -                        	}
> -				block[len] = value;
> -			}
> -			goto dofile;
>  		} else if (argc != flags + 6) {
>  			fprintf(stderr, "Error: Bad number of arguments!\n");
>  			help();
>  		}
>  	}
>  
> -	if (argc > flags + 4) {
> -		if (!strcmp(argv[flags+4], "c")
> -		 || !strcmp(argv[flags+4], "cp")) {
> -			size = I2C_SMBUS_BYTE;
> -			value = -1;
> -			pec = argv[flags+4][1] == 'p';
> -		} else {
> -			size = I2C_SMBUS_BYTE_DATA;
> -			value = strtol(argv[flags+4], &end, 0);
> +	len = 0; /* Must always initialize len since it is passed to confirm() */
> +
> +	/* read values from command line */
> +	switch (size) {
> +	case I2C_SMBUS_BYTE_DATA:
> +	case I2C_SMBUS_WORD_DATA:
> +		value = strtol(argv[flags+4], &end, 0);
> +		if (*end || value < 0) {
> +			fprintf(stderr, "Error: Data value invalid!\n");
> +			help();
> +		}
> +		if ((size == I2C_SMBUS_BYTE_DATA && value > 0xff)
> +		    || (size == I2C_SMBUS_WORD_DATA && value > 0xffff)) {
> +			fprintf(stderr, "Error: Data value out of range!\n");
> +			help();
> +		}
> +		break;
> +	case I2C_SMBUS_BLOCK_DATA:
> +	case I2C_SMBUS_I2C_BLOCK_DATA:
> +		for (len = 0; len + flags + 5 < argc; len++) {
> +			value = strtol(argv[flags + len + 4], &end, 0);
>  			if (*end || value < 0) {
>  				fprintf(stderr, "Error: Data value invalid!\n");
>  				help();
>  			}
> +			if (value > 0xff) {
> +				fprintf(stderr, "Error: Data value out of range!\n");
> +				help();
> +			}
> +			block[len] = value;
>  		}
> -	} else {
> -		size = I2C_SMBUS_BYTE;
>  		value = -1;
> -	}
> -
> -	if (argc > flags + 5) {
> -		switch (argv[flags+5][0]) {
> -		case 'b': size = I2C_SMBUS_BYTE_DATA; break;
> -		case 'w': size = I2C_SMBUS_WORD_DATA; break;
> -		default:
> -			fprintf(stderr, "Error: Invalid mode!\n");
> -			help();
> -		}
> -		pec = argv[flags+5][1] == 'p';
> +		break;

You could use a fall-through here.

> +	default:
> +		value = -1;
> +		break;
>  	}
>  
>  	if (maskp) {
> @@ -284,13 +302,6 @@ int main(int argc, char *argv[])
>  		}
>  	}
>  
> -	if ((size == I2C_SMBUS_BYTE_DATA && value > 0xff)
> -	 || (size == I2C_SMBUS_WORD_DATA && value > 0xffff)) {
> -		fprintf(stderr, "Error: Data value out of range!\n");
> -		help();
> -	}
> -
> -dofile:
>  	file = open_i2c_dev(i2cbus, filename, sizeof(filename), 0);
>  	if (file < 0

>  	 || check_funcs(file, size, pec)

Other than this minor comment (which you may not even agree with), this
patch looks OK to me now. Thanks.

-- 
Jean Delvare

[PATCH v3 0/6] i2cset changes

[Guenter Roeck]

The following series of patches removes the obsolete means to specify the value mask,
improves parameter error detection for the i2cset command, and cleans up the code a bit.

Note: Patch revision in headline was updated only if the individual patch is either new
or was changed.

v2:
- Split single patch into multiple individual patches
- Included code review feedback

v3:
- For block commands, check for number of arguments only after checking if pec
  is supported.
- Report "Too many arguments" instead of "Bad number of arguments".
- Committed blank replacement patch separately
- Check valid range for data value mask (patch #6)

[PATCH v2 3/6] i2cset: Abort if value mask is set for block commands

[Guenter Roeck]

Specifying the value mask is not supported for block commands,
abort if it is specified anyway.

Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
---
 tools/i2cset.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/tools/i2cset.c b/tools/i2cset.c
index e835465..7750c03 100644
--- a/tools/i2cset.c
+++ b/tools/i2cset.c
@@ -223,6 +223,10 @@ int main(int argc, char *argv[])
 				fprintf(stderr, "Error: PEC not supported for I2C block writes!\n");
 				help();
 			}
+			if (maskp) {
+				fprintf(stderr, "Error: Mask not supported for block writes!\n");
+				help();
+			}
 			if (argc > (int)sizeof(block) + flags + 5) {
 				fprintf(stderr, "Error: Too many arguments!\n");
 				help();
-- 
1.7.0.4