i2ctools/i2cset: Remove obsolete means to specify value mask

Attached patch removes the obsolete means to specify the value mask from i2cset. It also streamlines and improves parameter validation.

i2cset.c +18 -0

+	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 argument '%s'\n", argv[argc-1]);
+			help();

i2cset.c +7 -25

+			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 (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++) {
-				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;
-		}
-	}
-
-	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);
-			if (*end || value < 0) {
-				fprintf(stderr, "Error: Data value invalid!\n");
+			if (maskp) {
+				fprintf(stderr, "Error: Mask not supported for block writes!\n");
 				help();
 			}
+		} else if (argc != flags + 6) {
+			fprintf(stderr, "Error: Bad number of arguments!\n");
+			help();
 		}
-	} else {

i2cset.c +7 -8

+	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();
 		}
-		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");
+		if ((size == I2C_SMBUS_BYTE_DATA && value > 0xff)
+	 	    || (size == I2C_SMBUS_WORD_DATA && value > 0xffff)) {

i2cset.c +5 -7

+			block[len] = value;
+		}
+		break;
+	default:
+		break;
 	}
 
 	if (maskp) {

@@ -285,13 +289,6 @@
 		}
 	}
 
-	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)

Index: CHANGES
===================================================================

CHANGES +2 -0

@@ -4,6 +4,8 @@
 SVN
   i2c-dev.h: Make value arrays const for block write functions
   i2cset: Add support for SMBus and I2C block writes
+          Removed obsolete method to specify write mask
+          More stringent parameter validation
 
 3.0.3 (2010-12-12)
   Makefile: Let the environment set CC and CFLAGS

Hi Guenter,

Sorry for the late reply.

Quoted 2 lines

Attached patch removes the obsolete means to specify the value mask from i2cset. It also streamlines and improves parameter validation.

I'm afraid these are too many changes at once for an easy review. I'll try still, but ideally this would have been two patches at least.

Overall it looks good and I agree the new code is slightly easier to follow than the original code. I didn't really have a problem with the original code, but my view is certainly biased as I wrote most of it ;)

I did some tests and did not find any problem.

i2cset.c line 159

Quoted 11 lines

--
Index: tools/i2cset.c
===================================================================

i2cset.c +1 -1

@@ -156,7 +156,7 @@
 	char *end;
 	const char *maskp = NULL;
 	int res, i2cbus, address, size, file;
-	int value, daddress, vmask = 0;
+	int value = -1, daddress, vmask = 0;

I'd rather move this initialization to where it is needed (the default case in the switch block which reads the values from the command line). Pre-initializing variables unconditionally to prevent gcc warnings voids the value of these warnings.

Quoted 7 lines

i2cset.c +0 -0

 	char filename[20];
 	int pec = 0;
 	int flags = 0;

@@ -207,74 +207,78 @@
 		help();
 	}
 

i2cset.c line 210

Quoted 2 lines

i2cset.c +0 -1

-	/* check for block data */
 	len = 0;

It's confusing why you initialize len here. I think it would make more sense to initialize it later, where you set it for block transactions.

i2cset.c line 212

Quoted 1 line

i2cset.c +0 -1

-	if (argc > flags + 5) {

You could add a comment here, saying that you are now checking for the mode.

Quoted 24 lines

i2cset.c +21 -0

+	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 argument '%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;

i2cset.c line 236

Quoted 4 lines

i2cset.c +1 -2

 		default:
-			size = 0;
-			break;
+			fprintf(stderr, "Error: Invalid argument '%s'\n", argv[argc-1]);

The original code complained about an "invalid mode", rather than an "invalid argument". I think this wording was more helpful, as it tells the user what was expected for the argument.

Quoted 45 lines

i2cset.c +7 -27

+			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 (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++) {
-				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;
-		}
-	}
-
-	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);
-			if (*end || value < 0) {
-				fprintf(stderr, "Error: Data value invalid!\n");
+			if (maskp) {
+				fprintf(stderr, "Error: Mask not supported for block writes!\n");
 				help();
 			}
+		} else if (argc != flags + 6) {
+			fprintf(stderr, "Error: Bad number of arguments!\n");
+			help();
 		}
-	} else {
-		size = I2C_SMBUS_BYTE;
-		value = -1;
 	}
 

i2cset.c line 262

Quoted 6 lines

i2cset.c +0 -6

-	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");

You could add a comment here, saying that you are now reading the values from the command line.

i2cset.c line 255

Quoted 1 line

i2cset.c +1 -0

+	switch(size) {

Coding style: missing space before opening parenthesis.

Quoted 23 lines

i2cset.c +8 -11

+	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();
 		}
-		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");
+		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();
 		}
-		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];

i2cset.c line 271

Quoted 4 lines

i2cset.c +4 -0

+		break;
+	case I2C_SMBUS_BLOCK_DATA:
+	case I2C_SMBUS_I2C_BLOCK_DATA:
+		for (len = 0; len < (int)sizeof(block) && len + flags + 5 < argc; len++) {

I think it would make sense to count the values first, and complain if there are too many of these. Silently ignoring the extra values isn't helpful for the user.

i2cset.c line 274

Quoted 3 lines

i2cset.c +3 -0

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

The non-block case has separate error messages for invalid and out of range values, we could do the same here for consistency.

OTOH I don't think it adds value to include "block" in the error message, it prevents gcc from reusing the other message and doesn't add value.

i2cset.c line 276

Quoted 2 lines

i2cset.c +2 -0

+                               	help();
+                       	}

The indentation of the 3 previous lines is wrong (spaces instead of tabs.)

Quoted 35 lines

i2cset.c +5 -7

+			block[len] = value;
+		}
+		break;
+	default:
+		break;
 	}
 
 	if (maskp) {

@@ -285,13 +289,6 @@
 		}
 	}
 
-	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)

Index: CHANGES
===================================================================

CHANGES +2 -0

@@ -4,6 +4,8 @@
 SVN
   i2c-dev.h: Make value arrays const for block write functions
   i2cset: Add support for SMBus and I2C block writes
+          Removed obsolete method to specify write mask
+          More stringent parameter validation
 
 3.0.3 (2010-12-12)
   Makefile: Let the environment set CC and CFLAGS

Signature · 1 line

Jean Delvare

Hi Jean,

Quoted 4 lines · Jean Delvare

Hi Guenter,

Sorry for the late reply.

No problem. I appreciate your feedback.

Quoted 6 lines · Guenter Roeck

Quoted 2 lines

Attached patch removes the obsolete means to specify the value mask from i2cset. It also streamlines and improves parameter validation.

I'm afraid these are too many changes at once for an easy review. I'll try still, but ideally this would have been two patches at least.

No good idea how to split it, though.

Quoted 3 lines

Overall it looks good and I agree the new code is slightly easier to follow than the original code. I didn't really have a problem with the original code, but my view is certainly biased as I wrote most of it ;)

Another option would be to just keep the original code. This patch doesn't really change anything from a functionality perspective (other than being more stringent with parameter checking), and the proposed changes are not really needed. So I am perfectly fine with dropping it if you think it isn't worth it.

Quoted 18 lines

I did some tests and did not find any problem.

i2cset.c line 159

Quoted 11 lines

--
Index: tools/i2cset.c
===================================================================

i2cset.c +1 -1

@@ -156,7 +156,7 @@
 	char *end;
 	const char *maskp = NULL;
 	int res, i2cbus, address, size, file;
-	int value, daddress, vmask = 0;
+	int value = -1, daddress, vmask = 0;

I'd rather move this initialization to where it is needed (the default case in the switch block which reads the values from the command line). Pre-initializing variables unconditionally to prevent gcc warnings voids the value of these warnings.

The compiler does not understand when value is not used when passed to confirm(),

so it will have be initialized all the time, including the BLOCK_DATA case (because its use there is more of a side effect). I moved the initialization to the default and to the BLOCK_DATA cases.

Quoted 9 lines

i2cset.c line 210

Quoted 6 lines

i2cset.c +0 -1

@@ -207,74 +207,78 @@
 		help();
 	}
 
-	/* check for block data */
 	len = 0;

It's confusing why you initialize len here. I think it would make more sense to initialize it later, where you set it for block transactions.

The compiler doesn't understand that len is only used for block transactions in confirm(), and complains about an uninitialized variable otherwise. So it needs to be initialized for all commands.

Let me know if you have a better idea how to handle this - I don't like it too much either. For now I just added a comment explaining the reason for the initialization.

Quoted 2 lines

No good idea how to split it, though.

Well, if nothing else, you have two changes mentioned in file CHANGES. This would seem to be a natural split point.

You may think it's not worth it because one of the patches will be pretty small. However, please remember that the time it takes to review a patch is more or less proportional to the square of its length [1]. So even moving 10% of a patch to another may result in significant review time savings.

[1] This totally arbitrary and unproven statement is known as Jean Delvare's rule ;)

Quoted 10 lines

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +0 -1

-	/* check for block data */
 	len = 0;

It's confusing why you initialize len here. I think it would make more sense to initialize it later, where you set it for block transactions.

The compiler doesn't understand that len is only used for block transactions in confirm(), and complains about an uninitialized variable otherwise. So it needs to be initialized for all commands.

Sorry, I should have empathized my point. Once again:

It's confusing why you initialize len _here_.

Quoted 3 lines

Let me know if you have a better idea how to handle this - I don't like it too much either. For now I just added a comment explaining the reason for the initialization.

I get that it has to be initialized. I just don't think this is the most logical place.