[PATCH 2/4] drivers/scsi/fcoe/fcoe.c: add missing test

[PATCH 2/4] drivers/scsi/fcoe/fcoe.c: add missing test

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

The initializations of both fcoe_nport_scsi_transport and
fcoe_vport_scsi_transport can fail, so test both of them.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r@
identifier x,y,f!={PTR_ERR,ERR_PTR,ERR_CAST};
statement S;
@@

x = f(...);
(
if (\(x == NULL\|IS_ERR(x)\)) S
|
*if (\(y == NULL\|IS_ERR(y)\))
 { ... when != x
   return ...; }
)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 drivers/scsi/fcoe/fcoe.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index ba710e3..921b636 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1096,7 +1096,7 @@ static int __init fcoe_if_init(void)
 	fcoe_vport_scsi_transport =
 		fc_attach_transport(&fcoe_vport_fc_functions);
 
-	if (!fcoe_nport_scsi_transport) {
+	if (!fcoe_nport_scsi_transport || !fcoe_vport_scsi_transport) {
 		printk(KERN_ERR "fcoe: Failed to attach to the FC transport\n");
 		return -ENODEV;
 	}

Re: [PATCH 2/4] drivers/scsi/fcoe/fcoe.c: add missing test

[Jesper Juhl]

On Mon, 22 Aug 2011, Julia Lawall wrote:

> From: Julia Lawall <julia@diku.dk>
> 
> The initializations of both fcoe_nport_scsi_transport and
> fcoe_vport_scsi_transport can fail, so test both of them.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @r@
> identifier x,y,f!={PTR_ERR,ERR_PTR,ERR_CAST};
> statement S;
> @@
> 
> x = f(...);
> (
> if (\(x == NULL\|IS_ERR(x)\)) S
> |
> *if (\(y == NULL\|IS_ERR(y)\))
>  { ... when != x
>    return ...; }
> )
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>
> 
> ---
>  drivers/scsi/fcoe/fcoe.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> index ba710e3..921b636 100644
> --- a/drivers/scsi/fcoe/fcoe.c
> +++ b/drivers/scsi/fcoe/fcoe.c
> @@ -1096,7 +1096,7 @@ static int __init fcoe_if_init(void)
>  	fcoe_vport_scsi_transport =
>  		fc_attach_transport(&fcoe_vport_fc_functions);
>  
> -	if (!fcoe_nport_scsi_transport) {
> +	if (!fcoe_nport_scsi_transport || !fcoe_vport_scsi_transport) {
>  		printk(KERN_ERR "fcoe: Failed to attach to the FC transport\n");
>  		return -ENODEV;
>  	}
> 

I only took a quick look, so I may have overlooked something, so bear with 
me.

fc_attach_transport() allocates memory with kzalloc. If either call fails 
the other may have succeeded and we'll leak the memory allocated to one of 
them.

Shouldn't we be kfree()'ing the two variables before the 'return -ENODEV'?

-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH 2/4] drivers/scsi/fcoe/fcoe.c: add missing test

[Julia Lawall]

On Mon, 22 Aug 2011, Jesper Juhl wrote:

> On Mon, 22 Aug 2011, Julia Lawall wrote:
> 
> > From: Julia Lawall <julia@diku.dk>
> > 
> > The initializations of both fcoe_nport_scsi_transport and
> > fcoe_vport_scsi_transport can fail, so test both of them.
> > 
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> > 
> > // <smpl>
> > @r@
> > identifier x,y,f!={PTR_ERR,ERR_PTR,ERR_CAST};
> > statement S;
> > @@
> > 
> > x = f(...);
> > (
> > if (\(x == NULL\|IS_ERR(x)\)) S
> > |
> > *if (\(y == NULL\|IS_ERR(y)\))
> >  { ... when != x
> >    return ...; }
> > )
> > // </smpl>
> > 
> > Signed-off-by: Julia Lawall <julia@diku.dk>
> > 
> > ---
> >  drivers/scsi/fcoe/fcoe.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> > index ba710e3..921b636 100644
> > --- a/drivers/scsi/fcoe/fcoe.c
> > +++ b/drivers/scsi/fcoe/fcoe.c
> > @@ -1096,7 +1096,7 @@ static int __init fcoe_if_init(void)
> >  	fcoe_vport_scsi_transport =
> >  		fc_attach_transport(&fcoe_vport_fc_functions);
> >  
> > -	if (!fcoe_nport_scsi_transport) {
> > +	if (!fcoe_nport_scsi_transport || !fcoe_vport_scsi_transport) {
> >  		printk(KERN_ERR "fcoe: Failed to attach to the FC transport\n");
> >  		return -ENODEV;
> >  	}
> > 
> 
> I only took a quick look, so I may have overlooked something, so bear with 
> me.
> 
> fc_attach_transport() allocates memory with kzalloc. If either call fails 
> the other may have succeeded and we'll leak the memory allocated to one of 
> them.
> 
> Shouldn't we be kfree()'ing the two variables before the 'return -ENODEV'?

It seems reasonable.

julia

Re: [PATCH 2/4] drivers/scsi/fcoe/fcoe.c: add missing test

[Julia Lawall]

>From nobody Mon Aug 22 15:57:42 CEST 2011
From: Julia Lawall <julia@diku.dk>
To: Robert Love <robert.w.love@intel.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>,devel@open-fcoe.org,linux-scsi@vger.kernel.org,linux-kernel@vger.kernel.org
Subject: [PATCH 2/4] drivers/scsi/fcoe/fcoe.c: add missing test

From: Julia Lawall <julia@diku.dk>

The initializations of both fcoe_nport_scsi_transport and
fcoe_vport_scsi_transport can fail, so test both of them.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r@
identifier x,y,f!={PTR_ERR,ERR_PTR,ERR_CAST};
statement S;
@@

x = f(...);
(
if (\(x == NULL\|IS_ERR(x)\)) S
|
*if (\(y == NULL\|IS_ERR(y)\))
 { ... when != x
   return ...; }
)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 drivers/scsi/fcoe/fcoe.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index ba710e3..9f2e2f4 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1093,11 +1093,20 @@ static int __init fcoe_if_init(void)
 	/* attach to scsi transport */
 	fcoe_nport_scsi_transport =
 		fc_attach_transport(&fcoe_nport_fc_functions);
+
+	if (!fcoe_nport_scsi_transport) {
+		printk(KERN_ERR
+		       "fcoe_nport: Failed to attach to the FC transport\n");
+		return -ENODEV;
+	}
+
 	fcoe_vport_scsi_transport =
 		fc_attach_transport(&fcoe_vport_fc_functions);
 
-	if (!fcoe_nport_scsi_transport) {
-		printk(KERN_ERR "fcoe: Failed to attach to the FC transport\n");
+	if (!fcoe_vport_scsi_transport) {
+		printk(KERN_ERR
+		       "fcoe_vport: Failed to attach to the FC transport\n");
+		fc_release_transport(fcoe_nport_scsi_transport);
 		return -ENODEV;
 	}
 

RE: [Open-FCoE] [PATCH 2/4] drivers/scsi/fcoe/fcoe.c: add missing test

[Zou, Yi]

> From nobody Mon Aug 22 15:57:42 CEST 2011
> From: Julia Lawall <julia@diku.dk>
> To: Robert Love <robert.w.love@intel.com>
> Cc: "James E.J. Bottomley" <JBottomley@parallels.com>,devel@open-
> fcoe.org,linux-scsi@vger.kernel.org,linux-kernel@vger.kernel.org
> Subject: [PATCH 2/4] drivers/scsi/fcoe/fcoe.c: add missing test
> 
> From: Julia Lawall <julia@diku.dk>
> 
> The initializations of both fcoe_nport_scsi_transport and
> fcoe_vport_scsi_transport can fail, so test both of them.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @r@
> identifier x,y,f!={PTR_ERR,ERR_PTR,ERR_CAST};
> statement S;
> @@
> 
> x = f(...);
> (
> if (\(x == NULL\|IS_ERR(x)\)) S
> |
> *if (\(y == NULL\|IS_ERR(y)\))
>  { ... when != x
>    return ...; }
> )
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>
> 
> ---
>  drivers/scsi/fcoe/fcoe.c |   13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> index ba710e3..9f2e2f4 100644
> --- a/drivers/scsi/fcoe/fcoe.c
> +++ b/drivers/scsi/fcoe/fcoe.c
> @@ -1093,11 +1093,20 @@ static int __init fcoe_if_init(void)
>  	/* attach to scsi transport */
>  	fcoe_nport_scsi_transport =
>  		fc_attach_transport(&fcoe_nport_fc_functions);
> +
> +	if (!fcoe_nport_scsi_transport) {
> +		printk(KERN_ERR
> +		       "fcoe_nport: Failed to attach to the FC transport\n");
> +		return -ENODEV;
> +	}
> +
>  	fcoe_vport_scsi_transport =
>  		fc_attach_transport(&fcoe_vport_fc_functions);
> 
> -	if (!fcoe_nport_scsi_transport) {
> -		printk(KERN_ERR "fcoe: Failed to attach to the FC
> transport\n");
> +	if (!fcoe_vport_scsi_transport) {
> +		printk(KERN_ERR
> +		       "fcoe_vport: Failed to attach to the FC transport\n");
> +		fc_release_transport(fcoe_nport_scsi_transport);
>  		return -ENODEV;
>  	}
> 

I understand we were using -ENODEV before, but -ENOMEM is preferred here 
as that's the only err from fc_attach_transport.

Thanks,
yi

RE: [Open-FCoE] [PATCH 2/4] drivers/scsi/fcoe/fcoe.c: add missing test

[Julia Lawall]

On Tue, 23 Aug 2011, Zou, Yi wrote:

> > From nobody Mon Aug 22 15:57:42 CEST 2011
> > From: Julia Lawall <julia@diku.dk>
> > To: Robert Love <robert.w.love@intel.com>
> > Cc: "James E.J. Bottomley" <JBottomley@parallels.com>,devel@open-
> > fcoe.org,linux-scsi@vger.kernel.org,linux-kernel@vger.kernel.org
> > Subject: [PATCH 2/4] drivers/scsi/fcoe/fcoe.c: add missing test
> > 
> > From: Julia Lawall <julia@diku.dk>
> > 
> > The initializations of both fcoe_nport_scsi_transport and
> > fcoe_vport_scsi_transport can fail, so test both of them.
> > 
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> > 
> > // <smpl>
> > @r@
> > identifier x,y,f!={PTR_ERR,ERR_PTR,ERR_CAST};
> > statement S;
> > @@
> > 
> > x = f(...);
> > (
> > if (\(x == NULL\|IS_ERR(x)\)) S
> > |
> > *if (\(y == NULL\|IS_ERR(y)\))
> >  { ... when != x
> >    return ...; }
> > )
> > // </smpl>
> > 
> > Signed-off-by: Julia Lawall <julia@diku.dk>
> > 
> > ---
> >  drivers/scsi/fcoe/fcoe.c |   13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> > index ba710e3..9f2e2f4 100644
> > --- a/drivers/scsi/fcoe/fcoe.c
> > +++ b/drivers/scsi/fcoe/fcoe.c
> > @@ -1093,11 +1093,20 @@ static int __init fcoe_if_init(void)
> >  	/* attach to scsi transport */
> >  	fcoe_nport_scsi_transport =
> >  		fc_attach_transport(&fcoe_nport_fc_functions);
> > +
> > +	if (!fcoe_nport_scsi_transport) {
> > +		printk(KERN_ERR
> > +		       "fcoe_nport: Failed to attach to the FC transport\n");
> > +		return -ENODEV;
> > +	}
> > +
> >  	fcoe_vport_scsi_transport =
> >  		fc_attach_transport(&fcoe_vport_fc_functions);
> > 
> > -	if (!fcoe_nport_scsi_transport) {
> > -		printk(KERN_ERR "fcoe: Failed to attach to the FC
> > transport\n");
> > +	if (!fcoe_vport_scsi_transport) {
> > +		printk(KERN_ERR
> > +		       "fcoe_vport: Failed to attach to the FC transport\n");
> > +		fc_release_transport(fcoe_nport_scsi_transport);
> >  		return -ENODEV;
> >  	}
> > 
> 
> I understand we were using -ENODEV before, but -ENOMEM is preferred here 
> as that's the only err from fc_attach_transport.

OK, I'll fix it.  Thanks.

julia

RE: [Open-FCoE] [PATCH 2/4] drivers/scsi/fcoe/fcoe.c: add missing test

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

The initializations of both fcoe_nport_scsi_transport and
fcoe_vport_scsi_transport can fail, so test both of them.

Convert the return value to -ENOMEM to better reflect the reason for the 
error.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r@
identifier x,y,f!={PTR_ERR,ERR_PTR,ERR_CAST};
statement S;
@@

x = f(...);
(
if (\(x == NULL\|IS_ERR(x)\)) S
|
*if (\(y == NULL\|IS_ERR(y)\))
 { ... when != x
   return ...; }
)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 drivers/scsi/fcoe/fcoe.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index ba710e3..2a279eb 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1093,12 +1093,21 @@ static int __init fcoe_if_init(void)
 	/* attach to scsi transport */
 	fcoe_nport_scsi_transport =
 		fc_attach_transport(&fcoe_nport_fc_functions);
+
+	if (!fcoe_nport_scsi_transport) {
+		printk(KERN_ERR
+		       "fcoe_nport: Failed to attach to the FC transport\n");
+		return -ENOMEM;
+	}
+
 	fcoe_vport_scsi_transport =
 		fc_attach_transport(&fcoe_vport_fc_functions);
 
-	if (!fcoe_nport_scsi_transport) {
-		printk(KERN_ERR "fcoe: Failed to attach to the FC transport\n");
-		return -ENODEV;
+	if (!fcoe_vport_scsi_transport) {
+		printk(KERN_ERR
+		       "fcoe_vport: Failed to attach to the FC transport\n");
+		fc_release_transport(fcoe_nport_scsi_transport);
+		return -ENOMEM;
 	}
 
 	return 0;