* [PATCH -tip][SCSI] scsi: scsi_transport_iscsi.c initialize the variable before it get used
@ 2009-05-26 15:19 Jaswinder Singh Rajput
2009-05-26 15:28 ` James Bottomley
2009-05-26 21:36 ` Mike Christie
0 siblings, 2 replies; 6+ messages in thread
From: Jaswinder Singh Rajput @ 2009-05-26 15:19 UTC (permalink / raw)
To: James Bottomley, linux-scsi, Ingo Molnar, x86 maintainers, LKML
I am watching this problem from long time in -tip.
Subject: [PATCH] scsi: scsi_transport_iscsi.c initialize the variable before it get used
In some cases, err will be used uninitialized.
Also fixed compilation warning :
CC drivers/scsi/scsi_transport_iscsi.o
drivers/scsi/scsi_transport_iscsi.c: In function ‘iscsi_add_session’:
drivers/scsi/scsi_transport_iscsi.c:678: warning: ‘err’ may be used uninitialized in this function
Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@gmail.com>
---
drivers/scsi/scsi_transport_iscsi.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 0947954..e78cde2 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -675,7 +675,7 @@ int iscsi_add_session(struct iscsi_cls_session *session, unsigned int target_id)
struct iscsi_cls_host *ihost;
unsigned long flags;
unsigned int id = target_id;
- int err;
+ int err = 0;
ihost = shost->shost_data;
session->sid = atomic_add_return(1, &iscsi_session_nr);
--
1.6.0.6
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH -tip][SCSI] scsi: scsi_transport_iscsi.c initialize the variable before it get used
2009-05-26 15:19 [PATCH -tip][SCSI] scsi: scsi_transport_iscsi.c initialize the variable before it get used Jaswinder Singh Rajput
@ 2009-05-26 15:28 ` James Bottomley
2009-05-26 16:00 ` Jaswinder Singh Rajput
2009-05-26 21:36 ` Mike Christie
1 sibling, 1 reply; 6+ messages in thread
From: James Bottomley @ 2009-05-26 15:28 UTC (permalink / raw)
To: Jaswinder Singh Rajput; +Cc: linux-scsi, Ingo Molnar, x86 maintainers, LKML
On Tue, 2009-05-26 at 20:49 +0530, Jaswinder Singh Rajput wrote:
> I am watching this problem from long time in -tip.
>
> Subject: [PATCH] scsi: scsi_transport_iscsi.c initialize the variable before it get used
>
> In some cases, err will be used uninitialized.
What cases? A simple theoretical analysis shows that this variable is
always set.
> CC drivers/scsi/scsi_transport_iscsi.o
> drivers/scsi/scsi_transport_iscsi.c: In function ‘iscsi_add_session’:
> drivers/scsi/scsi_transport_iscsi.c:678: warning: ‘err’ may be used
> uninitialized in this function
My version of gcc (4.3.3) correctly sees that the variable cannot be
uninitialised ... what version are you using?
If it's a popular version, we can always do the uninitialised_var()
thing, but if it's just a non-standard compiler, I'd rather not mess up
the source code to please gcc.
James
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -tip][SCSI] scsi: scsi_transport_iscsi.c initialize the variable before it get used
2009-05-26 15:28 ` James Bottomley
@ 2009-05-26 16:00 ` Jaswinder Singh Rajput
2009-05-26 16:06 ` James Bottomley
0 siblings, 1 reply; 6+ messages in thread
From: Jaswinder Singh Rajput @ 2009-05-26 16:00 UTC (permalink / raw)
To: James Bottomley; +Cc: linux-scsi, Ingo Molnar, x86 maintainers, LKML
Hello James,
On Tue, 2009-05-26 at 10:28 -0500, James Bottomley wrote:
> On Tue, 2009-05-26 at 20:49 +0530, Jaswinder Singh Rajput wrote:
> > I am watching this problem from long time in -tip.
> >
> > Subject: [PATCH] scsi: scsi_transport_iscsi.c initialize the variable before it get used
> >
> > In some cases, err will be used uninitialized.
>
> What cases? A simple theoretical analysis shows that this variable is
> always set.
>
Ok lets takes your tree:
http://git.kernel.org/?p=linux/kernel/git/jejb/scsi-misc-2.6.git;a=blob;f=drivers/scsi/scsi_transport_iscsi.c;hb=10eb0f013c63c71c82ede77945a5f390c10cfda6
672 int iscsi_add_session(struct iscsi_cls_session *session, unsigned int target_id)
673 {
674 struct Scsi_Host *shost = iscsi_session_to_shost(session);
675 struct iscsi_cls_host *ihost;
676 unsigned long flags;
677 unsigned int id = target_id;
678 int err;
679
680 ihost = shost->shost_data;
681 session->sid = atomic_add_return(1, &iscsi_session_nr);
682
683 if (id == ISCSI_MAX_TARGET) {
684 for (id = 0; id < ISCSI_MAX_TARGET; id++) {
685 err = device_for_each_child(&shost->shost_gendev, &id,
686 iscsi_get_next_target_id);
687 if (!err)
688 break;
689 }
690
691 if (id == ISCSI_MAX_TARGET) {
692 iscsi_cls_session_printk(KERN_ERR, session,
693 "Too many iscsi targets. Max "
694 "number of targets is %d.\n",
695 ISCSI_MAX_TARGET - 1);
696 goto release_host;
697 }
698 }
699 session->target_id = id;
700
701 dev_set_name(&session->dev, "session%u", session->sid);
702 err = device_add(&session->dev);
703 if (err) {
704 iscsi_cls_session_printk(KERN_ERR, session,
705 "could not register session's dev\n");
706 goto release_host;
707 }
708 transport_register_device(&session->dev);
709
710 spin_lock_irqsave(&sesslock, flags);
711 list_add(&session->sess_list, &sesslist);
712 spin_unlock_irqrestore(&sesslock, flags);
713
714 iscsi_session_event(session, ISCSI_KEVENT_CREATE_SESSION);
715 return 0;
716
717 release_host:
718 scsi_host_put(shost);
719 return err;
720 }
You are right but compiler is confused and giving warning like this :
1. do not go inside :
684 for (id = 0; id < ISCSI_MAX_TARGET; id++) {
2. and then choose path :
696 goto release_host;
then we return err uninitialized ;-)
> > CC drivers/scsi/scsi_transport_iscsi.o
> > drivers/scsi/scsi_transport_iscsi.c: In function ‘iscsi_add_session’:
> > drivers/scsi/scsi_transport_iscsi.c:678: warning: ‘err’ may be used
> > uninitialized in this function
>
> My version of gcc (4.3.3) correctly sees that the variable cannot be
> uninitialised ... what version are you using?
>
> If it's a popular version, we can always do the uninitialised_var()
> thing, but if it's just a non-standard compiler, I'd rather not mess up
> the source code to please gcc.
>
gcc version 4.3.2 20081105 (Red Hat 4.3.2-7) (GCC)
--
JSR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -tip][SCSI] scsi: scsi_transport_iscsi.c initialize the variable before it get used
2009-05-26 16:00 ` Jaswinder Singh Rajput
@ 2009-05-26 16:06 ` James Bottomley
2009-05-26 16:21 ` Jaswinder Singh Rajput
0 siblings, 1 reply; 6+ messages in thread
From: James Bottomley @ 2009-05-26 16:06 UTC (permalink / raw)
To: Jaswinder Singh Rajput; +Cc: linux-scsi, Ingo Molnar, x86 maintainers, LKML
On Tue, 2009-05-26 at 21:30 +0530, Jaswinder Singh Rajput wrote:
> Hello James,
>
> On Tue, 2009-05-26 at 10:28 -0500, James Bottomley wrote:
> > On Tue, 2009-05-26 at 20:49 +0530, Jaswinder Singh Rajput wrote:
> > > I am watching this problem from long time in -tip.
> > >
> > > Subject: [PATCH] scsi: scsi_transport_iscsi.c initialize the variable before it get used
> > >
> > > In some cases, err will be used uninitialized.
> >
> > What cases? A simple theoretical analysis shows that this variable is
> > always set.
> >
>
> Ok lets takes your tree:
>
> http://git.kernel.org/?p=linux/kernel/git/jejb/scsi-misc-2.6.git;a=blob;f=drivers/scsi/scsi_transport_iscsi.c;hb=10eb0f013c63c71c82ede77945a5f390c10cfda6
>
>
> 672 int iscsi_add_session(struct iscsi_cls_session *session, unsigned int target_id)
> 673 {
> 674 struct Scsi_Host *shost = iscsi_session_to_shost(session);
> 675 struct iscsi_cls_host *ihost;
> 676 unsigned long flags;
> 677 unsigned int id = target_id;
> 678 int err;
> 679
> 680 ihost = shost->shost_data;
> 681 session->sid = atomic_add_return(1, &iscsi_session_nr);
> 682
> 683 if (id == ISCSI_MAX_TARGET) {
> 684 for (id = 0; id < ISCSI_MAX_TARGET; id++) {
> 685 err = device_for_each_child(&shost->shost_gendev, &id,
> 686 iscsi_get_next_target_id);
> 687 if (!err)
> 688 break;
> 689 }
> 690
> 691 if (id == ISCSI_MAX_TARGET) {
> 692 iscsi_cls_session_printk(KERN_ERR, session,
> 693 "Too many iscsi targets. Max "
> 694 "number of targets is %d.\n",
> 695 ISCSI_MAX_TARGET - 1);
> 696 goto release_host;
> 697 }
> 698 }
> 699 session->target_id = id;
> 700
> 701 dev_set_name(&session->dev, "session%u", session->sid);
> 702 err = device_add(&session->dev);
> 703 if (err) {
> 704 iscsi_cls_session_printk(KERN_ERR, session,
> 705 "could not register session's dev\n");
> 706 goto release_host;
> 707 }
> 708 transport_register_device(&session->dev);
> 709
> 710 spin_lock_irqsave(&sesslock, flags);
> 711 list_add(&session->sess_list, &sesslist);
> 712 spin_unlock_irqrestore(&sesslock, flags);
> 713
> 714 iscsi_session_event(session, ISCSI_KEVENT_CREATE_SESSION);
> 715 return 0;
> 716
> 717 release_host:
> 718 scsi_host_put(shost);
> 719 return err;
> 720 }
>
> You are right but compiler is confused and giving warning like this :
>
> 1. do not go inside :
> 684 for (id = 0; id < ISCSI_MAX_TARGET; id++) {
How? id is an unsigned int and ISCSI_MAX_TARGET is defined to be -1 in
include/scsi/scsi_transport_iscsi.h (might be better if it were
UINT_MAX, but -1 is just as good).
James
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -tip][SCSI] scsi: scsi_transport_iscsi.c initialize the variable before it get used
2009-05-26 16:06 ` James Bottomley
@ 2009-05-26 16:21 ` Jaswinder Singh Rajput
0 siblings, 0 replies; 6+ messages in thread
From: Jaswinder Singh Rajput @ 2009-05-26 16:21 UTC (permalink / raw)
To: James Bottomley; +Cc: linux-scsi, Ingo Molnar, x86 maintainers, LKML
On Tue, 2009-05-26 at 16:06 +0000, James Bottomley wrote:
> On Tue, 2009-05-26 at 21:30 +0530, Jaswinder Singh Rajput wrote:
> > Hello James,
> >
> > On Tue, 2009-05-26 at 10:28 -0500, James Bottomley wrote:
> > > On Tue, 2009-05-26 at 20:49 +0530, Jaswinder Singh Rajput wrote:
> > > > I am watching this problem from long time in -tip.
> > > >
> > > > Subject: [PATCH] scsi: scsi_transport_iscsi.c initialize the variable before it get used
> > > >
> > > > In some cases, err will be used uninitialized.
> > >
> > > What cases? A simple theoretical analysis shows that this variable is
> > > always set.
> > >
> >
> > Ok lets takes your tree:
> >
> > http://git.kernel.org/?p=linux/kernel/git/jejb/scsi-misc-2.6.git;a=blob;f=drivers/scsi/scsi_transport_iscsi.c;hb=10eb0f013c63c71c82ede77945a5f390c10cfda6
> >
> >
> > 672 int iscsi_add_session(struct iscsi_cls_session *session, unsigned int target_id)
> > 673 {
> > 674 struct Scsi_Host *shost = iscsi_session_to_shost(session);
> > 675 struct iscsi_cls_host *ihost;
> > 676 unsigned long flags;
> > 677 unsigned int id = target_id;
> > 678 int err;
> > 679
> > 680 ihost = shost->shost_data;
> > 681 session->sid = atomic_add_return(1, &iscsi_session_nr);
> > 682
> > 683 if (id == ISCSI_MAX_TARGET) {
> > 684 for (id = 0; id < ISCSI_MAX_TARGET; id++) {
> > 685 err = device_for_each_child(&shost->shost_gendev, &id,
> > 686 iscsi_get_next_target_id);
> > 687 if (!err)
> > 688 break;
> > 689 }
> > 690
> > 691 if (id == ISCSI_MAX_TARGET) {
> > 692 iscsi_cls_session_printk(KERN_ERR, session,
> > 693 "Too many iscsi targets. Max "
> > 694 "number of targets is %d.\n",
> > 695 ISCSI_MAX_TARGET - 1);
> > 696 goto release_host;
> > 697 }
> > 698 }
> > 699 session->target_id = id;
> > 700
> > 701 dev_set_name(&session->dev, "session%u", session->sid);
> > 702 err = device_add(&session->dev);
> > 703 if (err) {
> > 704 iscsi_cls_session_printk(KERN_ERR, session,
> > 705 "could not register session's dev\n");
> > 706 goto release_host;
> > 707 }
> > 708 transport_register_device(&session->dev);
> > 709
> > 710 spin_lock_irqsave(&sesslock, flags);
> > 711 list_add(&session->sess_list, &sesslist);
> > 712 spin_unlock_irqrestore(&sesslock, flags);
> > 713
> > 714 iscsi_session_event(session, ISCSI_KEVENT_CREATE_SESSION);
> > 715 return 0;
> > 716
> > 717 release_host:
> > 718 scsi_host_put(shost);
> > 719 return err;
> > 720 }
> >
> > You are right but compiler is confused and giving warning like this :
> >
> > 1. do not go inside :
> > 684 for (id = 0; id < ISCSI_MAX_TARGET; id++) {
>
> How? id is an unsigned int and ISCSI_MAX_TARGET is defined to be -1 in
> include/scsi/scsi_transport_iscsi.h (might be better if it were
> UINT_MAX, but -1 is just as good).
>
Yes, you are right, I was telling you how compiler was confused in 4.3.2
but I am glad to hear that this problem is fixed in 4.3.3 as you told
me :-)
So now its upto you. 4.3.2 is not soo old, So either initialize the
variable or stay with stray warning in the kernel ;-)
Thanks,
--
JSR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -tip][SCSI] scsi: scsi_transport_iscsi.c initialize the variable before it get used
2009-05-26 15:19 [PATCH -tip][SCSI] scsi: scsi_transport_iscsi.c initialize the variable before it get used Jaswinder Singh Rajput
2009-05-26 15:28 ` James Bottomley
@ 2009-05-26 21:36 ` Mike Christie
1 sibling, 0 replies; 6+ messages in thread
From: Mike Christie @ 2009-05-26 21:36 UTC (permalink / raw)
To: Jaswinder Singh Rajput
Cc: James Bottomley, linux-scsi, Ingo Molnar, x86 maintainers, LKML
[-- Attachment #1: Type: text/plain, Size: 916 bytes --]
Jaswinder Singh Rajput wrote:
> I am watching this problem from long time in -tip.
>
> Subject: [PATCH] scsi: scsi_transport_iscsi.c initialize the variable before it get used
>
> In some cases, err will be used uninitialized.
>
As you guys already discussed it will not get used uninitialized.
> Also fixed compilation warning :
>
> CC drivers/scsi/scsi_transport_iscsi.o
> drivers/scsi/scsi_transport_iscsi.c: In function ‘iscsi_add_session’:
> drivers/scsi/scsi_transport_iscsi.c:678: warning: ‘err’ may be used uninitialized in this function
>
I did the attached patch to silence the compile warning. I goofed and
forgot to send it before, because I had change my compiler and was not
seeing the error message. I was planning on sending the attached with
some other changes for the next feature window. I am going to fix up the
patch to use UINT_MAX like James mentioned in the thread.
[-- Attachment #2: 0001-iscsi-class-quiet-iscsi_add_session-compilation-war.patch --]
[-- Type: text/x-patch, Size: 1221 bytes --]
>From 32ffa5248bf11f4df76ebf3c4a023958a9131f9c Mon Sep 17 00:00:00 2001
From: Mike Christie <michaelc@cs.wisc.edu>
Date: Mon, 18 May 2009 11:54:37 -0500
Subject: [PATCH] iscsi class: quiet iscsi_add_session compilation warning.
If ISCSI_MAX_TARGET was zero then iscsi_add_session could return
success when it had failed. This will currently not happen because
ISCSI_MAX_TARGET is a macro and not a variable. But gcc likes to
complain about it and people keep reporting it. This patch
just initializes err to be -EINVAL so if we ever defined
ISCSI_MAX_TARGET incorrectly we would fail gracefully.
---
drivers/scsi/scsi_transport_iscsi.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 0a2ce7b..f508567 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -675,7 +675,7 @@ int iscsi_add_session(struct iscsi_cls_session *session, unsigned int target_id)
struct iscsi_cls_host *ihost;
unsigned long flags;
unsigned int id = target_id;
- int err;
+ int err = -EINVAL;
ihost = shost->shost_data;
session->sid = atomic_add_return(1, &iscsi_session_nr);
--
1.6.0.6
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-05-26 22:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-05-26 15:19 [PATCH -tip][SCSI] scsi: scsi_transport_iscsi.c initialize the variable before it get used Jaswinder Singh Rajput
2009-05-26 15:28 ` James Bottomley
2009-05-26 16:00 ` Jaswinder Singh Rajput
2009-05-26 16:06 ` James Bottomley
2009-05-26 16:21 ` Jaswinder Singh Rajput
2009-05-26 21:36 ` Mike Christie
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).