[PATCH] infiniband-diags: ibstat fix strncpy coverity check and -l bug

[PATCH] infiniband-diags: ibstat fix strncpy coverity check and -l bug

[Ira Weiny]

Signed-off-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 src/ibstat.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/src/ibstat.c b/src/ibstat.c
index 665bb0a..1ef27f2 100644
--- a/src/ibstat.c
+++ b/src/ibstat.c
@@ -314,7 +314,8 @@ int main(int argc, char *argv[])
 		if (i >= n)
 			IBPANIC("'%s' IB device can't be found", argv[0]);
 
-		strncpy(names[i], argv[0], sizeof names[i]);
+		strncpy(names[0], argv[0], sizeof(names[0])-1);
+		names[0][sizeof(names[0])-1] = '\0';
 		n = 1;
 	}
 
@@ -324,12 +325,6 @@ int main(int argc, char *argv[])
 		return 0;
 	}
 
-	if (!list_only && argc) {
-		if (ca_stat(argv[0], dev_port, short_format) < 0)
-			IBPANIC("stat of IB device '%s' failed", argv[0]);
-		return 0;
-	}
-
 	for (i = 0; i < n; i++) {
 		if (list_only)
 			printf("%s\n", names[i]);
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] infiniband-diags: ibstat fix strncpy coverity check and -l bug

[Hal Rosenstock]

On 6/12/2013 8:03 PM, Ira Weiny wrote:

This seems like 2 different patches to me.

> Signed-off-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  src/ibstat.c |    9 ++-------
>  1 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/src/ibstat.c b/src/ibstat.c
> index 665bb0a..1ef27f2 100644
> --- a/src/ibstat.c
> +++ b/src/ibstat.c
> @@ -314,7 +314,8 @@ int main(int argc, char *argv[])
>  		if (i >= n)
>  			IBPANIC("'%s' IB device can't be found", argv[0]);
>  
> -		strncpy(names[i], argv[0], sizeof names[i]);
> +		strncpy(names[0], argv[0], sizeof(names[0])-1);
> +		names[0][sizeof(names[0])-1] = '\0';
>  		n = 1;
>  	}
>  
> @@ -324,12 +325,6 @@ int main(int argc, char *argv[])
>  		return 0;
>  	}
>  
> -	if (!list_only && argc) {

I think I see the issue. Rather than eliminate this if clause, I think
the line above should be:

	if (!list_only && argc > 1) {

so that this properly handles the case when the port option is supplied.

-- Hal

> -		if (ca_stat(argv[0], dev_port, short_format) < 0)
> -			IBPANIC("stat of IB device '%s' failed", argv[0]);
> -		return 0;
> -	}
> -
>  	for (i = 0; i < n; i++) {
>  		if (list_only)
>  			printf("%s\n", names[i]);

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] infiniband-diags: ibstat fix strncpy coverity check and -l bug

[Ira Weiny]

On Wed, 12 Jun 2013 21:38:58 -0400
Hal Rosenstock <hal-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:

> On 6/12/2013 8:03 PM, Ira Weiny wrote:
> 
> This seems like 2 different patches to me.

Not really...

> 
> > Signed-off-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > ---
> >  src/ibstat.c |    9 ++-------
> >  1 files changed, 2 insertions(+), 7 deletions(-)
> > 
> > @@ -324,12 +325,6 @@ int main(int argc, char *argv[])
> >  		return 0;
> >  	}
> >  
> > -	if (!list_only && argc) {
> 
> I think I see the issue. Rather than eliminate this if clause, I think
> the line above should be:
> 
> 	if (!list_only && argc > 1) {
> 
> so that this properly handles the case when the port option is supplied.

V2 on the way.  I think we should still get rid of that if clause but your right I broke the "port" parameter.

Ira

> 
> -- Hal
> 
> > -		if (ca_stat(argv[0], dev_port, short_format) < 0)
> > -			IBPANIC("stat of IB device '%s' failed", argv[0]);
> > -		return 0;
> > -	}
> > -
> >  	for (i = 0; i < n; i++) {
> >  		if (list_only)
> >  			printf("%s\n", names[i]);
> 


-- 
Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] infiniband-diags: ibstat fix strncpy coverity check and -l bug

[Ira Weiny]

Changes since V1:
	Fix port parameter usage.

Signed-off-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 src/ibstat.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/src/ibstat.c b/src/ibstat.c
index 665bb0a..37f2361 100644
--- a/src/ibstat.c
+++ b/src/ibstat.c
@@ -314,7 +314,8 @@ int main(int argc, char *argv[])
 		if (i >= n)
 			IBPANIC("'%s' IB device can't be found", argv[0]);
 
-		strncpy(names[i], argv[0], sizeof names[i]);
+		strncpy(names[0], argv[0], sizeof(names[0])-1);
+		names[0][sizeof(names[0])-1] = '\0';
 		n = 1;
 	}
 
@@ -324,16 +325,10 @@ int main(int argc, char *argv[])
 		return 0;
 	}
 
-	if (!list_only && argc) {
-		if (ca_stat(argv[0], dev_port, short_format) < 0)
-			IBPANIC("stat of IB device '%s' failed", argv[0]);
-		return 0;
-	}
-
 	for (i = 0; i < n; i++) {
 		if (list_only)
 			printf("%s\n", names[i]);
-		else if (ca_stat(names[i], -1, short_format) < 0)
+		else if (ca_stat(names[i], dev_port, short_format) < 0)
 			IBPANIC("stat of IB device '%s' failed", names[i]);
 	}
 
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] infiniband-diags: ibstat fix strncpy coverity check and -l bug

[Hal Rosenstock]

On 6/13/2013 12:15 AM, Ira Weiny wrote:
> 
> Changes since V1:
> 	Fix port parameter usage.
> 
> Signed-off-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  src/ibstat.c |   11 +++--------
>  1 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/src/ibstat.c b/src/ibstat.c
> index 665bb0a..37f2361 100644
> --- a/src/ibstat.c
> +++ b/src/ibstat.c
> @@ -314,7 +314,8 @@ int main(int argc, char *argv[])
>  		if (i >= n)
>  			IBPANIC("'%s' IB device can't be found", argv[0]);
>  
> -		strncpy(names[i], argv[0], sizeof names[i]);
> +		strncpy(names[0], argv[0], sizeof(names[0])-1);
> +		names[0][sizeof(names[0])-1] = '\0';
>  		n = 1;
>  	}
>  
> @@ -324,16 +325,10 @@ int main(int argc, char *argv[])
>  		return 0;
>  	}
>  
> -	if (!list_only && argc) {
> -		if (ca_stat(argv[0], dev_port, short_format) < 0)
> -			IBPANIC("stat of IB device '%s' failed", argv[0]);
> -		return 0;
> -	}
> -
>  	for (i = 0; i < n; i++) {
>  		if (list_only)
>  			printf("%s\n", names[i]);
> -		else if (ca_stat(names[i], -1, short_format) < 0)
> +		else if (ca_stat(names[i], dev_port, short_format) < 0)

I don't think this works when ib device is not first one and port is
also supplied.

-- Hal

>  			IBPANIC("stat of IB device '%s' failed", names[i]);
>  	}
>  

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] infiniband-diags: ibstat fix strncpy coverity check and -l bug

[Weiny, Ira]

> -----Original Message-----
> From: Hal Rosenstock [mailto:hal-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org]
> Subject: Re: [PATCH] infiniband-diags: ibstat fix strncpy coverity check and -l
> bug
> 
> On 6/13/2013 12:15 AM, Ira Weiny wrote:
> >
> > Changes since V1:
> > 	Fix port parameter usage.
> >
> > Signed-off-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > ---
> >  src/ibstat.c |   11 +++--------
> >  1 files changed, 3 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/ibstat.c b/src/ibstat.c index 665bb0a..37f2361 100644
> > --- a/src/ibstat.c
> > +++ b/src/ibstat.c
> > @@ -314,7 +314,8 @@ int main(int argc, char *argv[])
> >  		if (i >= n)
> >  			IBPANIC("'%s' IB device can't be found", argv[0]);
> >
> > -		strncpy(names[i], argv[0], sizeof names[i]);
> > +		strncpy(names[0], argv[0], sizeof(names[0])-1);
> > +		names[0][sizeof(names[0])-1] = '\0';
> >  		n = 1;
> >  	}
> >
> > @@ -324,16 +325,10 @@ int main(int argc, char *argv[])
> >  		return 0;
> >  	}
> >
> > -	if (!list_only && argc) {
> > -		if (ca_stat(argv[0], dev_port, short_format) < 0)
> > -			IBPANIC("stat of IB device '%s' failed", argv[0]);
> > -		return 0;
> > -	}
> > -
> >  	for (i = 0; i < n; i++) {
> >  		if (list_only)
> >  			printf("%s\n", names[i]);
> > -		else if (ca_stat(names[i], -1, short_format) < 0)
> > +		else if (ca_stat(names[i], dev_port, short_format) < 0)
> 
> I don't think this works when ib device is not first one and port is also
> supplied.

That is what the change in the first hunk does.  It places argv[0] in name[0] and sets n=1.

This works fine for me:

11:17:42 > ./ibstat -l         
mlx4_0
mlx4_1

11:16:33 > ./ibstat mlx4_1 1
CA: 'mlx4_1'
Port 1:
State: Down
Physical state: Polling
Rate: 10
Base lid: 0
LMC: 0
SM lid: 0
Capability mask: 0x02510868
Port GUID: 0x0002c9030003ced7
Link layer: InfiniBand

Or are you speaking of some other use case?

Ira

> 
> -- Hal
> 
> >  			IBPANIC("stat of IB device '%s' failed", names[i]);
> >  	}
> >

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] infiniband-diags: ibstat fix strncpy coverity check and -l bug

[Hal Rosenstock]

On 6/13/2013 2:20 PM, Weiny, Ira wrote:
>> -----Original Message-----
>> From: Hal Rosenstock [mailto:hal-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org]
>> Subject: Re: [PATCH] infiniband-diags: ibstat fix strncpy coverity check and -l
>> bug
>>
>> On 6/13/2013 12:15 AM, Ira Weiny wrote:
>>>
>>> Changes since V1:
>>> 	Fix port parameter usage.
>>>
>>> Signed-off-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>> ---
>>>  src/ibstat.c |   11 +++--------
>>>  1 files changed, 3 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/ibstat.c b/src/ibstat.c index 665bb0a..37f2361 100644
>>> --- a/src/ibstat.c
>>> +++ b/src/ibstat.c
>>> @@ -314,7 +314,8 @@ int main(int argc, char *argv[])
>>>  		if (i >= n)
>>>  			IBPANIC("'%s' IB device can't be found", argv[0]);
>>>
>>> -		strncpy(names[i], argv[0], sizeof names[i]);
>>> +		strncpy(names[0], argv[0], sizeof(names[0])-1);
>>> +		names[0][sizeof(names[0])-1] = '\0';
>>>  		n = 1;
>>>  	}
>>>
>>> @@ -324,16 +325,10 @@ int main(int argc, char *argv[])
>>>  		return 0;
>>>  	}
>>>
>>> -	if (!list_only && argc) {
>>> -		if (ca_stat(argv[0], dev_port, short_format) < 0)
>>> -			IBPANIC("stat of IB device '%s' failed", argv[0]);
>>> -		return 0;
>>> -	}
>>> -
>>>  	for (i = 0; i < n; i++) {
>>>  		if (list_only)
>>>  			printf("%s\n", names[i]);
>>> -		else if (ca_stat(names[i], -1, short_format) < 0)
>>> +		else if (ca_stat(names[i], dev_port, short_format) < 0)
>>
>> I don't think this works when ib device is not first one and port is also
>> supplied.
> 
> That is what the change in the first hunk does.  It places argv[0] in name[0] and sets n=1.
> 
> This works fine for me:
> 
> 11:17:42 > ./ibstat -l         
> mlx4_0
> mlx4_1
> 
> 11:16:33 > ./ibstat mlx4_1 1
> CA: 'mlx4_1'
> Port 1:
> State: Down
> Physical state: Polling
> Rate: 10
> Base lid: 0
> LMC: 0
> SM lid: 0
> Capability mask: 0x02510868
> Port GUID: 0x0002c9030003ced7
> Link layer: InfiniBand
> 
> Or are you speaking of some other use case?

No; I missed looking carefully at the first hunk.

Reviewed-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

-- Hal

> 
> Ira
> 
>>
>> -- Hal
>>
>>>  			IBPANIC("stat of IB device '%s' failed", names[i]);
>>>  	}
>>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html