scsi: ch.c fix shadowed variable warnings

PATCH In review [email protected] 4 participants 9 messages Mar 28, 2008 – Apr 1, 2008 SCSI

lore

Share & tools

lore

[email protected]

author

2008-03-28 21:49 UTC #0 lore raw

PATCH 11/17 scsi: ch.c fix shadowed variable warnings

From: Harvey Harrison <[email protected]>

err shadows the array of errors in this driver, switch to ch_err drivers/scsi/ch.c:271:6: warning: symbol 'err' shadows an earlier one drivers/scsi/ch.c:116:3: originally declared here

cmd shadows the argument to this function, switch to ch_cmd drivers/scsi/ch.c:724:11: warning: symbol 'cmd' shadows an earlier one drivers/scsi/ch.c:596:20: originally declared here

Small code cleanup as well in if() statement.

[[email protected]: coding-style fixes]

Signed-off-by Harvey Harrison <[email protected]>

Cc James Bottomley <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 drivers/scsi/ch.c |   29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

drivers/scsi/ch.c renamed from drivers/scsi/ch.c~scsi-chc-fix-shadowed-variable-warnings +15 ~~-14~~

diff -puN drivers/scsi/ch.c~scsi-chc-fix-shadowed-variable-warnings drivers/scsi/ch.c
@@ -268,16 +268,16 @@ ch_read_element_status(scsi_changer *ch,
 static int
 ch_init_elem(scsi_changer *ch)
 {
-	int err;
+	int ch_err;
 	u_char cmd[6];
 
 	vprintk("INITIALIZE ELEMENT STATUS, may take some time ...\n");
 	memset(cmd,0,sizeof(cmd));
 	cmd[0] = INITIALIZE_ELEMENT_STATUS;
 	cmd[1] = ch->device->lun << 5;
-	err = ch_do_scsi(ch, cmd, NULL, 0, DMA_NONE);
+	ch_err = ch_do_scsi(ch, cmd, NULL, 0, DMA_NONE);
 	vprintk("... finished\n");
-	return err;
+	return ch_err;
 }
 
 static int

@@ -721,7 +721,7 @@ static long ch_ioctl(struct file *file,
 	case CHIOGELEM:
 	{
 		struct changer_get_element cge;
-		u_char  cmd[12];
+		u_char  ch_cmd[12];
 		u_char  *buffer;
 		unsigned int elem;
 		int     result,i;

@@ -739,17 +739,18 @@ static long ch_ioctl(struct file *file,
 		mutex_lock(&ch->lock);
 
 	voltag_retry:
-		memset(cmd,0,sizeof(cmd));
-		cmd[0] = READ_ELEMENT_STATUS;
-		cmd[1] = (ch->device->lun << 5) |
-			(ch->voltags ? 0x10 : 0) |
-			ch_elem_to_typecode(ch,elem);
-		cmd[2] = (elem >> 8) & 0xff;
-		cmd[3] = elem        & 0xff;
-		cmd[5] = 1;
-		cmd[9] = 255;
+		memset(ch_cmd, 0, sizeof(ch_cmd));
+		ch_cmd[0] = READ_ELEMENT_STATUS;
+		ch_cmd[1] = (ch->device->lun << 5) |
+			    (ch->voltags ? 0x10 : 0) |
+			    ch_elem_to_typecode(ch, elem);
+		ch_cmd[2] = (elem >> 8) & 0xff;
+		ch_cmd[3] = elem        & 0xff;
+		ch_cmd[5] = 1;
+		ch_cmd[9] = 255;
 
-		if (0 == (result = ch_do_scsi(ch, cmd, buffer, 256, DMA_FROM_DEVICE))) {
+		result = ch_do_scsi(ch, ch_cmd, buffer, 256, DMA_FROM_DEVICE);
+		if (!result) {
 			cge.cge_status = buffer[18];
 			cge.cge_flags = 0;
 			if (buffer[18] & CESTATUS_EXCEPT) {

James Bottomley

maintainer

2008-03-30 17:22 UTC #1 lore raw

Quoted 25 lines · On Fri, 2008-03-28 at 14:48 -0700, [email protected] wrote:

From: Harvey Harrison <[email protected]>
err shadows the array of errors in this driver, switch to ch_err drivers/scsi/ch.c:271:6: warning: symbol 'err' shadows an earlier one drivers/scsi/ch.c:116:3: originally declared here
cmd shadows the argument to this function, switch to ch_cmd drivers/scsi/ch.c:724:11: warning: symbol 'cmd' shadows an earlier one drivers/scsi/ch.c:596:20: originally declared here
Small code cleanup as well in if() statement.
[[email protected]: coding-style fixes]
Signed-off-by Harvey Harrison <[email protected]>
Cc James Bottomley <[email protected]>
Signed-off-by Andrew Morton <[email protected]>
---
 drivers/scsi/ch.c |   29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)
diff -puN drivers/scsi/ch.c~scsi-chc-fix-shadowed-variable-warnings drivers/scsi/ch.c

drivers/scsi/ch.c ch_read_element_status(scsi_changer *ch, · line 271

Quoted 5 lines

drivers/scsi/ch.c renamed from drivers/scsi/ch.c~scsi-chc-fix-shadowed-variable-warnings +1 -1
@@ -268,16 +268,16 @@ ch_read_element_status(scsi_changer *ch,
 static int
 ch_init_elem(scsi_changer *ch)
 {
-	int err;
+	int ch_err;

This isn't really the correct fix, is it? The driver stupidity is having a global (although static) variable called err which invites problems like this. How about this fix?

James

---

drivers/scsi/ch.c +6 -6

index 7aad154..b1be2d8 100644
@@ -113,7 +113,7 @@ static const struct {
 	unsigned char  asc;
 	unsigned char  ascq;
 	int	       errno;
-} err[] = {
+} ch_err_desc[] = {
 /* Just filled in what looks right. Hav'nt checked any standard paper for
    these errno assignments, so they may be wrong... */
 	{

@@ -155,11 +155,11 @@ static int ch_find_errno(struct scsi_sense_hdr *sshdr)
 	/* Check to see if additional sense information is available */
 	if (scsi_sense_valid(sshdr) &&
 	    sshdr->asc != 0) {
-		for (i = 0; err[i].errno != 0; i++) {
-			if (err[i].sense == sshdr->sense_key &&
-			    err[i].asc   == sshdr->asc &&
-			    err[i].ascq  == sshdr->ascq) {
-				errno = -err[i].errno;
+		for (i = 0; ch_err_desc[i].errno != 0; i++) {
+			if (ch_err_desc[i].sense == sshdr->sense_key &&
+			    ch_err_desc[i].asc   == sshdr->asc &&
+			    ch_err_desc[i].ascq  == sshdr->ascq) {
+				errno = -ch_err_desc[i].errno;
 				break;
 			}
 		}

Harvey Harrison

2008-03-30 18:43 UTC #2 lore raw

Quoted 22 lines · James Bottomley

Quoted 21 lines · On Fri, 2008-03-28 at 14:48 -0700, [email protected] wrote:
From: Harvey Harrison <[email protected]>
err shadows the array of errors in this driver, switch to ch_err drivers/scsi/ch.c:271:6: warning: symbol 'err' shadows an earlier one drivers/scsi/ch.c:116:3: originally declared here
cmd shadows the argument to this function, switch to ch_cmd drivers/scsi/ch.c:724:11: warning: symbol 'cmd' shadows an earlier one drivers/scsi/ch.c:596:20: originally declared here
Small code cleanup as well in if() statement.
[[email protected]: coding-style fixes]
Signed-off-by Harvey Harrison <[email protected]>
Cc James Bottomley <[email protected]>
Signed-off-by Andrew Morton <[email protected]>
---
 drivers/scsi/ch.c |   29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

Quoted 14 lines

drivers/scsi/ch.c ch_read_element_status(scsi_changer *ch, · line 271
Quoted 9 lines
diff -puN drivers/scsi/ch.c~scsi-chc-fix-shadowed-variable-warnings drivers/scsi/ch.c
drivers/scsi/ch.c renamed from drivers/scsi/ch.c~scsi-chc-fix-shadowed-variable-warnings +1 -1
@@ -268,16 +268,16 @@ ch_read_element_status(scsi_changer *ch,
 static int
 ch_init_elem(scsi_changer *ch)
 {
-	int err;
+	int ch_err;
This isn't really the correct fix, is it? The driver stupidity is having a global (although static) variable called err which invites problems like this. How about this fix?

If you like that better, sure.

Harvey

James Bottomley

maintainer

2008-03-30 18:56 UTC #3 lore raw

Quoted 38 lines · Harvey Harrison

Quoted 22 lines · James Bottomley
Quoted 21 lines · On Fri, 2008-03-28 at 14:48 -0700, [email protected] wrote:
From: Harvey Harrison <[email protected]>
err shadows the array of errors in this driver, switch to ch_err drivers/scsi/ch.c:271:6: warning: symbol 'err' shadows an earlier one drivers/scsi/ch.c:116:3: originally declared here
cmd shadows the argument to this function, switch to ch_cmd drivers/scsi/ch.c:724:11: warning: symbol 'cmd' shadows an earlier one drivers/scsi/ch.c:596:20: originally declared here
Small code cleanup as well in if() statement.
[[email protected]: coding-style fixes]
Signed-off-by Harvey Harrison <[email protected]>
Cc James Bottomley <[email protected]>
Signed-off-by Andrew Morton <[email protected]>
---
 drivers/scsi/ch.c |   29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)
Quoted 14 lines
drivers/scsi/ch.c ch_read_element_status(scsi_changer *ch, · line 271
Quoted 9 lines
diff -puN drivers/scsi/ch.c~scsi-chc-fix-shadowed-variable-warnings drivers/scsi/ch.c
drivers/scsi/ch.c renamed from drivers/scsi/ch.c~scsi-chc-fix-shadowed-variable-warnings +1 -1
@@ -268,16 +268,16 @@ ch_read_element_status(scsi_changer *ch,
 static int
 ch_init_elem(scsi_changer *ch)
 {
-	int err;
+	int ch_err;
This isn't really the correct fix, is it? The driver stupidity is having a global (although static) variable called err which invites problems like this. How about this fix?

Quoted 1 line

If you like that better, sure.

Er, well it's not really a question of like; it's more a question of the programming principle of reducing namespace pollution.

The more global something is (and you start with global headers, then arch headers, then globally exported symbols, then subsystem symbols, etc ...) the more polluting. So, the only thing that should be allowed to name something totally generic, like 'err' or 'ret' should be right at the lowest level (i.e. unexported symbols of local functions).

Your patch was wrong because you changed the name of the lowest level and left the actual polluting symbol in the next level up, thus inviting another patch or function addition to do it all over again. The correct way to fix something like this is to rename the more globally polluting symbol to make a clash far less likely.

James

Harvey Harrison

2008-03-30 18:59 UTC #4 lore raw

Quoted 17 lines · James Bottomley

Quoted 7 lines
Quoted 5 lines
This isn't really the correct fix, is it? The driver stupidity is having a global (although static) variable called err which invites problems like this. How about this fix?
If you like that better, sure.
Er, well it's not really a question of like; it's more a question of the programming principle of reducing namespace pollution.
The more global something is (and you start with global headers, then arch headers, then globally exported symbols, then subsystem symbols, etc ...) the more polluting. So, the only thing that should be allowed to name something totally generic, like 'err' or 'ret' should be right at the lowest level (i.e. unexported symbols of local functions).

Quoted 5 lines

Your patch was wrong because you changed the name of the lowest level and left the actual polluting symbol in the next level up, thus inviting another patch or function addition to do it all over again. The correct way to fix something like this is to rename the more globally polluting symbol to make a clash far less likely.

True, but being somewhat unfamiliar with scsi code, I chose the more localized version that was easier to verify.

Are you going to apply your above version, or did you want me to redo mine that way?

Harvey

James Bottomley

maintainer

2008-04-01 03:10 UTC #5 lore raw

Quoted 24 lines · Harvey Harrison

Quoted 17 lines · James Bottomley
Quoted 7 lines
Quoted 5 lines
This isn't really the correct fix, is it? The driver stupidity is having a global (although static) variable called err which invites problems like this. How about this fix?
If you like that better, sure.
Er, well it's not really a question of like; it's more a question of the programming principle of reducing namespace pollution.
The more global something is (and you start with global headers, then arch headers, then globally exported symbols, then subsystem symbols, etc ...) the more polluting. So, the only thing that should be allowed to name something totally generic, like 'err' or 'ret' should be right at the lowest level (i.e. unexported symbols of local functions).
Quoted 5 lines
Your patch was wrong because you changed the name of the lowest level and left the actual polluting symbol in the next level up, thus inviting another patch or function addition to do it all over again. The correct way to fix something like this is to rename the more globally polluting symbol to make a clash far less likely.

Quoted 5 lines

True, but being somewhat unfamiliar with scsi code, I chose the more localized version that was easier to verify.
Are you going to apply your above version, or did you want me to redo mine that way?

Sure ... I only spent a few seconds with an editor and checked that it compiled ... if there's a problem with my patch, by all means fix it up and resubmit it.

James

Harvey Harrison

2008-04-01 05:05 UTC #6 lore raw

PATCH 1/1 scsi: ch.c fix sparse shadowed variable warnings

Replace the global err array with ch_err. drivers/scsi/ch.c:271:6: warning: symbol 'err' shadows an earlier one drivers/scsi/ch.c:116:3: originally declared here

Replace the temporary cmd buffer with ch_err to avoid shadowing the cmd function parameter.
drivers/scsi/ch.c:724:11: warning: symbol 'cmd' shadows an earlier one drivers/scsi/ch.c:596:20: originally declared here

Signed-off-by Harvey Harrison <[email protected]>

---
James, incorporated your comments.

 drivers/scsi/ch.c |   33 +++++++++++++++++----------------
 1 files changed, 17 insertions(+), 16 deletions(-)

drivers/scsi/ch.c +17 ~~-16~~

index 7aad154..92d1cb1 100644
@@ -113,7 +113,7 @@ static const struct {
 	unsigned char  asc;
 	unsigned char  ascq;
 	int	       errno;
-} err[] = {
+} ch_err[] = {
 /* Just filled in what looks right. Hav'nt checked any standard paper for
    these errno assignments, so they may be wrong... */
 	{

@@ -155,11 +155,11 @@ static int ch_find_errno(struct scsi_sense_hdr *sshdr)
 	/* Check to see if additional sense information is available */
 	if (scsi_sense_valid(sshdr) &&
 	    sshdr->asc != 0) {
-		for (i = 0; err[i].errno != 0; i++) {
-			if (err[i].sense == sshdr->sense_key &&
-			    err[i].asc   == sshdr->asc &&
-			    err[i].ascq  == sshdr->ascq) {
-				errno = -err[i].errno;
+		for (i = 0; ch_err[i].errno != 0; i++) {
+			if (ch_err[i].sense == sshdr->sense_key &&
+			    ch_err[i].asc   == sshdr->asc &&
+			    ch_err[i].ascq  == sshdr->ascq) {
+				errno = -ch_err[i].errno;
 				break;
 			}
 		}

@@ -721,8 +721,8 @@ static long ch_ioctl(struct file *file,
 	case CHIOGELEM:
 	{
 		struct changer_get_element cge;
-		u_char  cmd[12];
-		u_char  *buffer;
+		u_char ch_cmd[12];
+		u_char *buffer;
 		unsigned int elem;
 		int     result,i;

@@ -739,17 +739,18 @@ static long ch_ioctl(struct file *file,
 		mutex_lock(&ch->lock);
 
 	voltag_retry:
-		memset(cmd,0,sizeof(cmd));
-		cmd[0] = READ_ELEMENT_STATUS;
-		cmd[1] = (ch->device->lun << 5) |
+		memset(ch_cmd, 0, sizeof(ch_cmd));
+		ch_cmd[0] = READ_ELEMENT_STATUS;
+		ch_cmd[1] = (ch->device->lun << 5) |
 			(ch->voltags ? 0x10 : 0) |
 			ch_elem_to_typecode(ch,elem);
-		cmd[2] = (elem >> 8) & 0xff;
-		cmd[3] = elem        & 0xff;
-		cmd[5] = 1;
-		cmd[9] = 255;
+		ch_cmd[2] = (elem >> 8) & 0xff;
+		ch_cmd[3] = elem        & 0xff;
+		ch_cmd[5] = 1;
+		ch_cmd[9] = 255;
 
-		if (0 == (result = ch_do_scsi(ch, cmd, buffer, 256, DMA_FROM_DEVICE))) {
+		result = ch_do_scsi(ch, ch_cmd, buffer, 256, DMA_FROM_DEVICE);
+		if (!result) {
 			cge.cge_status = buffer[18];
 			cge.cge_flags = 0;
 			if (buffer[18] & CESTATUS_EXCEPT) {

Signature · 1 line

1.5.5.rc1.135.g8527

James Bottomley

maintainer

2008-04-01 14:51 UTC #7 lore raw

Re: [PATCH] scsi: ch.c fix sparse shadowed variable warnings

Quoted 4 lines · Harvey Harrison

Replace the global err array with ch_err. drivers/scsi/ch.c:271:6: warning: symbol 'err' shadows an earlier one drivers/scsi/ch.c:116:3: originally declared here

Quoted 8 lines

Replace the temporary cmd buffer with ch_err to avoid shadowing the cmd function parameter.
drivers/scsi/ch.c:724:11: warning: symbol 'cmd' shadows an earlier one drivers/scsi/ch.c:596:20: originally declared here
Signed-off-by Harvey Harrison <[email protected]>
---
James, incorporated your comments.

Yes, that looks fine, thanks.

I have a challenge for you: It looks like this driver has three incorrect unconditional uses of GFP_DMA. It gets a 512 byte bufer and then only uses 256 bytes of it on several occasions and the routine with the shadowed cmd variable looks like it should be calling ch_read_element_status() rather than duplicating it.

James

Boaz Harrosh

2008-04-01 15:16 UTC #8 lore raw

Re: [PATCH] scsi: ch.c fix sparse shadowed variable warnings

Quoted 16 lines · James Bottomley

Quoted 4 lines · Harvey Harrison
Replace the global err array with ch_err. drivers/scsi/ch.c:271:6: warning: symbol 'err' shadows an earlier one drivers/scsi/ch.c:116:3: originally declared here
Quoted 8 lines
Replace the temporary cmd buffer with ch_err to avoid shadowing the cmd function parameter.
drivers/scsi/ch.c:724:11: warning: symbol 'cmd' shadows an earlier one drivers/scsi/ch.c:596:20: originally declared here
Signed-off-by Harvey Harrison <[email protected]>
---
James, incorporated your comments.
Yes, that looks fine, thanks.

Quoted 8 lines

I have a challenge for you: It looks like this driver has three incorrect unconditional uses of GFP_DMA. It gets a 512 byte bufer and then only uses 256 bytes of it on several occasions and the routine with the shadowed cmd variable looks like it should be calling ch_read_element_status() rather than duplicating it.
James

I have the GFP_DMA and allocation stuff covered for this driver.

Boaz