Skip to contentSkip to conversation

iscsi-target: remove some dead code

PATCH Applied Dan Carpenter 4 participants 7 messages May 17, 2012

Subsystems (3)
SCSI Kernel janitors SCSI target
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 iscsi-target: remove some dead code

Neither "acceptor_values" nor "proposer_values" can be NULL here. Smatch complains because we are not allowed to pass NULL pointers to strchr().

Also I removed a second later check for "!acceptor_values" because it gets checked on the next line in the do while condition.

Signed-off-by Dan Carpenter <[email protected]>

---
Compile tested only. Please review carefully.

drivers/target/iscsi/iscsi_target_parameters.c +0 -9
index ad3b3c1..ed5241e 100644
@@ -1037,13 +1037,6 @@ static char *iscsi_check_valuelist_for_support(
 			tmp2 = strchr(acceptor_values, ',');
 			if (tmp2)
 				*tmp2 = '\0';
-			if (!acceptor_values || !proposer_values) {
-				if (tmp1)
-					*tmp1 = ',';
-				if (tmp2)
-					*tmp2 = ',';
-				return NULL;
-			}
 			if (!strcmp(acceptor_values, proposer_values)) {
 				if (tmp2)
 					*tmp2 = ',';

@@ -1053,8 +1046,6 @@ static char *iscsi_check_valuelist_for_support(
 				*tmp2++ = ',';
 
 			acceptor_values = tmp2;
-			if (!acceptor_values)
-				break;
 		} while (acceptor_values);
 		if (tmp1)
 			*tmp1++ = ',';
Applied
#1 lore raw

Hello DanC,

Quoted 16 lines · Dan Carpenter

Neither "acceptor_values" nor "proposer_values" can be NULL here. Smatch complains because we are not allowed to pass NULL pointers to strchr().

Also I removed a second later check for "!acceptor_values" because it gets checked on the next line in the do while condition.

Signed-off-by Dan Carpenter <[email protected]>

---
Compile tested only. Please review carefully.

drivers/target/iscsi/iscsi_target_parameters.c +0 -0
index ad3b3c1..ed5241e 100644

drivers/target/iscsi/iscsi_target_parameters.c static char *iscsi_check_valuelist_for_support( · line 1046

Quoted 10 lines
drivers/target/iscsi/iscsi_target_parameters.c +0 -7
@@ -1037,13 +1037,6 @@ static char *iscsi_check_valuelist_for_support(
 			tmp2 = strchr(acceptor_values, ',');
 			if (tmp2)
 				*tmp2 = '\0';
-			if (!acceptor_values || !proposer_values) {
-				if (tmp1)
-					*tmp1 = ',';
-				if (tmp2)
-					*tmp2 = ',';
-				return NULL;
-			}

Mmmmm, I seem to remember this code being some manner of bugfix when these two NULL checks for IS_TYPE_VALUE_LIST() type login parameters where originally added into iscsi-target code (circa 2004)..

Looking at iscsi_check_valuelist_for_support() now in modern usage I think it's fine to go ahead and drop these checks..

drivers/target/iscsi/iscsi_target_parameters.c static char *iscsi_check_valuelist_for_support( · line 1051

Quoted 12 lines
drivers/target/iscsi/iscsi_target_parameters.c +0 -2
 			if (!strcmp(acceptor_values, proposer_values)) {
 				if (tmp2)
 					*tmp2 = ',';

@@ -1053,8 +1046,6 @@ static char *iscsi_check_valuelist_for_support(
 				*tmp2++ = ',';
 
 			acceptor_values = tmp2;
-			if (!acceptor_values)
-				break;
 		} while (acceptor_values);
 		if (tmp1)
 			*tmp1++ = ',';

<nod>, looks harmless to drop as well.

Applied to lio-core, and queuing into for-next.

Thanks Dan!

--nab

#2 lore raw
In reply to Dan Carpenter

Am 17.05.2012 09:08, schrieb Dan Carpenter:

Quoted 16 lines

Neither "acceptor_values" nor "proposer_values" can be NULL here. Smatch complains because we are not allowed to pass NULL pointers to strchr().

Also I removed a second later check for "!acceptor_values" because it gets checked on the next line in the do while condition.

Signed-off-by Dan Carpenter <[email protected]>

---
Compile tested only. Please review carefully.

drivers/target/iscsi/iscsi_target_parameters.c +0 -0
index ad3b3c1..ed5241e 100644

drivers/target/iscsi/iscsi_target_parameters.c static char *iscsi_check_valuelist_for_support( · line 1039

Quoted 3 lines
drivers/target/iscsi/iscsi_target_parameters.c +0 -0
@@ -1037,13 +1037,6 @@ static char *iscsi_check_valuelist_for_support(
 			tmp2 = strchr(acceptor_values, ',');
 			if (tmp2)
 				*tmp2 = '\0';

this looks like strchrnul(). I realy do not know is this supported inside the kernel ?

Quoted 11 lines
drivers/target/iscsi/iscsi_target_parameters.c +0 -7
-			if (!acceptor_values || !proposer_values) {
-				if (tmp1)
-					*tmp1 = ',';
-				if (tmp2)
-					*tmp2 = ',';
-				return NULL;
-			}
 			if (!strcmp(acceptor_values, proposer_values)) {
 				if (tmp2)
 					*tmp2 = ',';

drivers/target/iscsi/iscsi_target_parameters.c static char *iscsi_check_valuelist_for_support( · line 1051

Quoted 8 lines
drivers/target/iscsi/iscsi_target_parameters.c +0 -2
@@ -1053,8 +1046,6 @@ static char *iscsi_check_valuelist_for_support(
 				*tmp2++ = ',';
 
 			acceptor_values = tmp2;
-			if (!acceptor_values)
-				break;
 		} while (acceptor_values);
 		if (tmp1)
 			*tmp1++ = ',';

the changes look reasonable but to be fair the code should have an explanation what it is supposed to do. I do not have the feeling that i did understand the purpose completely.

just my 2 cents,
wh

author
#3 lore raw
Quoted 14 lines · walter harms

Am 17.05.2012 09:08, schrieb Dan Carpenter:

Quoted 11 lines

Neither "acceptor_values" nor "proposer_values" can be NULL here. Smatch complains because we are not allowed to pass NULL pointers to strchr().

Also I removed a second later check for "!acceptor_values" because it gets checked on the next line in the do while condition.

Signed-off-by Dan Carpenter <[email protected]>

---
Compile tested only. Please review carefully.

Quoted 10 lines

drivers/target/iscsi/iscsi_target_parameters.c static char *iscsi_check_valuelist_for_support( · line 1039

Quoted 8 lines
drivers/target/iscsi/iscsi_target_parameters.c +0 -0
index ad3b3c1..ed5241e 100644
@@ -1037,13 +1037,6 @@ static char *iscsi_check_valuelist_for_support(
 			tmp2 = strchr(acceptor_values, ',');
 			if (tmp2)
 				*tmp2 = '\0';

this looks like strchrnul(). I realy do not know is this supported inside the kernel ?

It can't be NULL here. Adding bogus checks is the wrong idea and static checkers are right to ask about needless checks.

regards,
dan carpenter

#4 lore raw
In reply to walter harms
Quoted 4 lines · walter harms

the changes look reasonable but to be fair the code should have an explanation what it is supposed to do. I do not have the feeling that i did understand the purpose completely.

I suppose that is a decent summary for much of the target code. Patches are clearly welcome.

Jörn

--
Fancy algorithms are buggier than simple ones, and they're much harder to implement. Use simple algorithms as well as simple data structures. -- Rob Pike
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw
In reply to Dan Carpenter

Am 17.05.2012 12:26, schrieb Dan Carpenter:

Quoted 26 lines · walter harms
Quoted 14 lines

Am 17.05.2012 09:08, schrieb Dan Carpenter:

Quoted 11 lines

Neither "acceptor_values" nor "proposer_values" can be NULL here. Smatch complains because we are not allowed to pass NULL pointers to strchr().

Also I removed a second later check for "!acceptor_values" because it gets checked on the next line in the do while condition.

Signed-off-by Dan Carpenter <[email protected]>

---
Compile tested only. Please review carefully.

Quoted 10 lines

drivers/target/iscsi/iscsi_target_parameters.c static char *iscsi_check_valuelist_for_support( · line 1039

Quoted 8 lines
drivers/target/iscsi/iscsi_target_parameters.c +0 -0
index ad3b3c1..ed5241e 100644
@@ -1037,13 +1037,6 @@ static char *iscsi_check_valuelist_for_support(
 			tmp2 = strchr(acceptor_values, ',');
 			if (tmp2)
 				*tmp2 = '\0';

this looks like strchrnul(). I realy do not know is this supported inside the kernel ?

Quoted 3 lines

It can't be NULL here. Adding bogus checks is the wrong idea and static checkers are right to ask about needless checks.

sorry i was writing (talking) about something different.
the construction:
....
v=strchr()
if (v=NULL)
return '\0'
...
is like strchrnul() in glibc. Maybe this pattern is used more than once inside the kernel code.

re,
wh

author
#6 lore raw
In reply to Dan Carpenter
Quoted 32 lines · walter harms

Am 17.05.2012 12:26, schrieb Dan Carpenter:

Quoted 26 lines · walter harms
Quoted 14 lines

Am 17.05.2012 09:08, schrieb Dan Carpenter:

Quoted 11 lines

Neither "acceptor_values" nor "proposer_values" can be NULL here. Smatch complains because we are not allowed to pass NULL pointers to strchr().

Also I removed a second later check for "!acceptor_values" because it gets checked on the next line in the do while condition.

Signed-off-by Dan Carpenter <[email protected]>

---
Compile tested only. Please review carefully.

Quoted 10 lines

drivers/target/iscsi/iscsi_target_parameters.c static char *iscsi_check_valuelist_for_support( · line 1039

Quoted 8 lines
drivers/target/iscsi/iscsi_target_parameters.c +0 -0
index ad3b3c1..ed5241e 100644
@@ -1037,13 +1037,6 @@ static char *iscsi_check_valuelist_for_support(
 			tmp2 = strchr(acceptor_values, ',');
 			if (tmp2)
 				*tmp2 = '\0';

this looks like strchrnul(). I realy do not know is this supported inside the kernel ?

Quoted 3 lines

It can't be NULL here. Adding bogus checks is the wrong idea and static checkers are right to ask about needless checks.

Quoted 8 lines

sorry i was writing (talking) about something different.
the construction:
....
v=strchr()
if (v=NULL)
return '\0'
...
is like strchrnul() in glibc. Maybe this pattern is used more than once inside the kernel code.

No no. My mistake I didn't know what strchrnul() did. But there isn't a kernel version of that function.

regards,
dan carpenter