[patch 11/17] scsi: ch.c fix shadowed variable warnings

[patch 11/17] scsi: ch.c fix shadowed variable warnings

[akpm]

From: Harvey Harrison <harvey.harrison@gmail.com>

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.

[akpm@linux-foundation.org: coding-style fixes]
Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 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
--- a/drivers/scsi/ch.c~scsi-chc-fix-shadowed-variable-warnings
+++ a/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) {
_

Re: [patch 11/17] scsi: ch.c fix shadowed variable warnings

[James Bottomley]

On Fri, 2008-03-28 at 14:48 -0700, akpm@linux-foundation.org wrote:
> From: Harvey Harrison <harvey.harrison@gmail.com>
> 
> 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.
> 
> [akpm@linux-foundation.org: coding-style fixes]
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  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
> --- a/drivers/scsi/ch.c~scsi-chc-fix-shadowed-variable-warnings
> +++ a/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;

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

---

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 7aad154..b1be2d8 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -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;
 			}
 		}

Re: [patch 11/17] scsi: ch.c fix shadowed variable warnings

[Harvey Harrison]

On Sun, 2008-03-30 at 12:21 -0500, James Bottomley wrote:
> On Fri, 2008-03-28 at 14:48 -0700, akpm@linux-foundation.org wrote:
> > From: Harvey Harrison <harvey.harrison@gmail.com>
> > 
> > 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.
> > 
> > [akpm@linux-foundation.org: coding-style fixes]
> > Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> > Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > ---
> > 
> >  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
> > --- a/drivers/scsi/ch.c~scsi-chc-fix-shadowed-variable-warnings
> > +++ a/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;
> 
> 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

Re: [patch 11/17] scsi: ch.c fix shadowed variable warnings

[James Bottomley]

On Sun, 2008-03-30 at 11:43 -0700, Harvey Harrison wrote:
> On Sun, 2008-03-30 at 12:21 -0500, James Bottomley wrote:
> > On Fri, 2008-03-28 at 14:48 -0700, akpm@linux-foundation.org wrote:
> > > From: Harvey Harrison <harvey.harrison@gmail.com>
> > > 
> > > 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.
> > > 
> > > [akpm@linux-foundation.org: coding-style fixes]
> > > Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> > > Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > > ---
> > > 
> > >  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
> > > --- a/drivers/scsi/ch.c~scsi-chc-fix-shadowed-variable-warnings
> > > +++ a/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;
> > 
> > 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).

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

Re: [patch 11/17] scsi: ch.c fix shadowed variable warnings

[Harvey Harrison]

On Sun, 2008-03-30 at 13:55 -0500, James Bottomley wrote:
> > > 
> > > 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).
> 
> 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

Re: [patch 11/17] scsi: ch.c fix shadowed variable warnings

[James Bottomley]

On Sun, 2008-03-30 at 11:59 -0700, Harvey Harrison wrote:
> On Sun, 2008-03-30 at 13:55 -0500, James Bottomley wrote:
> > > > 
> > > > 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).
> > 
> > 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?

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

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

[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

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 <harvey.harrison@gmail.com>
---
James, incorporated your comments.

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

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 7aad154..92d1cb1 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -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) {
-- 
1.5.5.rc1.135.g8527

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

[James Bottomley]

On Mon, 2008-03-31 at 22:05 -0700, Harvey Harrison wrote:
> 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 <harvey.harrison@gmail.com>
> ---
> 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

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

[Boaz Harrosh]

On Tue, Apr 01 2008 at 17:51 +0300, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> On Mon, 2008-03-31 at 22:05 -0700, Harvey Harrison wrote:
>> 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 <harvey.harrison@gmail.com>
>> ---
>> 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
> 
I have the GFP_DMA and allocation stuff covered for this driver.

Boaz