[PATCH] siw: Fix potential NULL pointer in siw_connect().

[PATCH] siw: Fix potential NULL pointer in siw_connect().

[Bernard Metzler]

Fixes: 6c52fdc244b5 ("rdma/siw: connection management")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com>
---
 drivers/infiniband/sw/siw/siw_cm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 9ce8a1b925d2..fc97571a640b 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1515,7 +1515,7 @@ int siw_connect(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 		}
 	}
 error:
-	siw_dbg_qp(qp, "failed: %d\n", rv);
+	siw_dbg(id->device, "failed: %d\n", rv);
 
 	if (cep) {
 		siw_socket_disassoc(s);
@@ -1540,7 +1540,8 @@ int siw_connect(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 	} else if (s) {
 		sock_release(s);
 	}
-	siw_qp_put(qp);
+	if (qp)
+		siw_qp_put(qp);
 
 	return rv;
 }
-- 
2.17.2

Re: [PATCH] siw: Fix potential NULL pointer in siw_connect().

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2195 bytes --]

On Mon, 2019-08-19 at 16:02 +0200, Bernard Metzler wrote:
> Fixes: 6c52fdc244b5 ("rdma/siw: connection management")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com>
> ---
>  drivers/infiniband/sw/siw/siw_cm.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/siw/siw_cm.c
> b/drivers/infiniband/sw/siw/siw_cm.c
> index 9ce8a1b925d2..fc97571a640b 100644
> --- a/drivers/infiniband/sw/siw/siw_cm.c
> +++ b/drivers/infiniband/sw/siw/siw_cm.c
> @@ -1515,7 +1515,7 @@ int siw_connect(struct iw_cm_id *id, struct
> iw_cm_conn_param *params)
>  		}
>  	}
>  error:
> -	siw_dbg_qp(qp, "failed: %d\n", rv);
> +	siw_dbg(id->device, "failed: %d\n", rv);
>  
>  	if (cep) {
>  		siw_socket_disassoc(s);
> @@ -1540,7 +1540,8 @@ int siw_connect(struct iw_cm_id *id, struct
> iw_cm_conn_param *params)
>  	} else if (s) {
>  		sock_release(s);
>  	}
> -	siw_qp_put(qp);
> +	if (qp)
> +		siw_qp_put(qp);
>  
>  	return rv;
>  }

Hi Bernard,

We try to avoid empty commit messages.  I know in this case the subject
really carries the important info, but it looks better when displaying
the commit if there is something in the message body.

Also, please preface the subject with RDMA/siw: as we like to have the
RDMA subsystem preface the individual subcomponent element in the
subject line (some people still use IB, which is what used to be
preferred, but RDMA is preferred today).

I fixed both of those things up and applied this to for-rc, thanks.

Please take a look (I pushed it out to my wip/dl-for-rc branch) so you
can see what I mean about how to make both a simple subject line and a
decent commit message.  Also, no final punctuation on the subject line,
and try to keep the subject length <= 50 chars total.  If you have to go
over to have a decent subject, then so be it, but we strive for that 50
char limit to make a subject stay on one line when displayed using git
log --oneline.

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Re: [PATCH] siw: Fix potential NULL pointer in siw_connect().

[Bernard Metzler]

-----"Doug Ledford" <dledford@redhat.com> wrote: -----

>To: "Bernard Metzler" <bmt@zurich.ibm.com>,
>linux-rdma@vger.kernel.org
>From: "Doug Ledford" <dledford@redhat.com>
>Date: 08/20/2019 06:05PM
>Cc: dan.carpenter@oracle.com, jgg@ziepe.ca
>Subject: [EXTERNAL] Re: [PATCH] siw: Fix potential NULL pointer in
>siw_connect().
>
>On Mon, 2019-08-19 at 16:02 +0200, Bernard Metzler wrote:
>> Fixes: 6c52fdc244b5 ("rdma/siw: connection management")
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>> Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com>
>> ---
>>  drivers/infiniband/sw/siw/siw_cm.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>> b/drivers/infiniband/sw/siw/siw_cm.c
>> index 9ce8a1b925d2..fc97571a640b 100644
>> --- a/drivers/infiniband/sw/siw/siw_cm.c
>> +++ b/drivers/infiniband/sw/siw/siw_cm.c
>> @@ -1515,7 +1515,7 @@ int siw_connect(struct iw_cm_id *id, struct
>> iw_cm_conn_param *params)
>>  		}
>>  	}
>>  error:
>> -	siw_dbg_qp(qp, "failed: %d\n", rv);
>> +	siw_dbg(id->device, "failed: %d\n", rv);
>>  
>>  	if (cep) {
>>  		siw_socket_disassoc(s);
>> @@ -1540,7 +1540,8 @@ int siw_connect(struct iw_cm_id *id, struct
>> iw_cm_conn_param *params)
>>  	} else if (s) {
>>  		sock_release(s);
>>  	}
>> -	siw_qp_put(qp);
>> +	if (qp)
>> +		siw_qp_put(qp);
>>  
>>  	return rv;
>>  }
>
>Hi Bernard,
>
>We try to avoid empty commit messages.  I know in this case the
>subject
>really carries the important info, but it looks better when
>displaying
>the commit if there is something in the message body.
>
>Also, please preface the subject with RDMA/siw: as we like to have
>the
>RDMA subsystem preface the individual subcomponent element in the
>subject line (some people still use IB, which is what used to be
>preferred, but RDMA is preferred today).
>
>I fixed both of those things up and applied this to for-rc, thanks.
>
>Please take a look (I pushed it out to my wip/dl-for-rc branch) so
>you
>can see what I mean about how to make both a simple subject line and
>a
>decent commit message.  Also, no final punctuation on the subject
>line,
>and try to keep the subject length <= 50 chars total.  If you have to
>go
>over to have a decent subject, then so be it, but we strive for that
>50
>char limit to make a subject stay on one line when displayed using
>git
>log --oneline.
>

Hi Doug,

I'll print that email and put it at the wall, right above the screen.
Thanks so much for making it clear even to people like me!

Best regards,
Bernard

Re: [PATCH] siw: Fix potential NULL pointer in siw_connect().

[Dan Carpenter]

On Tue, Aug 20, 2019 at 12:05:33PM -0400, Doug Ledford wrote:
> Please take a look (I pushed it out to my wip/dl-for-rc branch) so you
> can see what I mean about how to make both a simple subject line and a
> decent commit message.  Also, no final punctuation on the subject line,
> and try to keep the subject length <= 50 chars total.  If you have to go
> over to have a decent subject, then so be it, but we strive for that 50
> char limit to make a subject stay on one line when displayed using git
> log --oneline.

50 is really small.  If it were based on git log --oneline output the
limit would be 67 characters.  If you look at actual kernel git commits
then the average subject is 52.4 characters and probably the upper bound
is 60+ or so.

I was surprised how well I had done personally at generating subjects
when I looked at my own git log.

My shortest subject was commit 0746556beab1 ("bna: off by one").  That
was from 10 years ago and is not up to my current standards.  My longest
was commit 49d3d6c37a32 ("drivers/misc/sgi-gru/grukdump.c: unlocking
should be conditional in gru_dump_context()"), but originally I used
"gru:" as the patch prefix and Andrew changed it.  :P

regards,
dan carpenter

Re: [PATCH] siw: Fix potential NULL pointer in siw_connect().

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1917 bytes --]

On Wed, 2019-08-21 at 15:56 +0300, Dan Carpenter wrote:
> On Tue, Aug 20, 2019 at 12:05:33PM -0400, Doug Ledford wrote:
> > Please take a look (I pushed it out to my wip/dl-for-rc branch) so
> > you
> > can see what I mean about how to make both a simple subject line and
> > a
> > decent commit message.  Also, no final punctuation on the subject
> > line,
> > and try to keep the subject length <= 50 chars total.  If you have
> > to go
> > over to have a decent subject, then so be it, but we strive for that
> > 50
> > char limit to make a subject stay on one line when displayed using
> > git
> > log --oneline.
> 
> 50 is really small.

50 is the vim syntax highlighting suggested limit.  You can go over,
which is why I indicated it was a soft limit, but there you are.  It
leaves room for the displayed hash length to grow as well.

>   If it were based on git log --oneline output the
> limit would be 67 characters.

Only if you don't include room for the hash size to grow and other
possible things, like a tag.

>   If you look at actual kernel git commits
> then the average subject is 52.4 characters and probably the upper
> bound
> is 60+ or so.

Yep, probably largely due to that very same vim syntax highlighting :-)

> I was surprised how well I had done personally at generating subjects
> when I looked at my own git log.
> 
> My shortest subject was commit 0746556beab1 ("bna: off by one").  That
> was from 10 years ago and is not up to my current standards.  My
> longest
> was commit 49d3d6c37a32 ("drivers/misc/sgi-gru/grukdump.c: unlocking
> should be conditional in gru_dump_context()"), but originally I used
> "gru:" as the patch prefix and Andrew changed it.  :P
> 
> regards,
> dan carpenter

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] siw: Fix potential NULL pointer in siw_connect().

[Jason Gunthorpe]

On Wed, Aug 21, 2019 at 09:39:50AM -0400, Doug Ledford wrote:
> On Wed, 2019-08-21 at 15:56 +0300, Dan Carpenter wrote:
> > On Tue, Aug 20, 2019 at 12:05:33PM -0400, Doug Ledford wrote:
> > > Please take a look (I pushed it out to my wip/dl-for-rc branch) so
> > > you
> > > can see what I mean about how to make both a simple subject line and
> > > a
> > > decent commit message.  Also, no final punctuation on the subject
> > > line,
> > > and try to keep the subject length <= 50 chars total.  If you have
> > > to go
> > > over to have a decent subject, then so be it, but we strive for that
> > > 50
> > > char limit to make a subject stay on one line when displayed using
> > > git
> > > log --oneline.
> > 
> > 50 is really small.
> 
> 50 is the vim syntax highlighting suggested limit.  You can go over,
> which is why I indicated it was a soft limit, but there you are.  It
> leaves room for the displayed hash length to grow as well.

I use 75 for all text in the commit message, as per
Documentation/process/submitting-patches.rst

People using 'git log --oneline' should have terminals wider than 80
:)

The bigger question is if the first character after the subject tag
should be uppper case or lower case <hum>

Jason

Re: [PATCH] siw: Fix potential NULL pointer in siw_connect().

[Leon Romanovsky]

On Wed, Aug 21, 2019 at 11:12:25AM -0300, Jason Gunthorpe wrote:
> On Wed, Aug 21, 2019 at 09:39:50AM -0400, Doug Ledford wrote:
> > On Wed, 2019-08-21 at 15:56 +0300, Dan Carpenter wrote:
> > > On Tue, Aug 20, 2019 at 12:05:33PM -0400, Doug Ledford wrote:
> > > > Please take a look (I pushed it out to my wip/dl-for-rc branch) so
> > > > you
> > > > can see what I mean about how to make both a simple subject line and
> > > > a
> > > > decent commit message.  Also, no final punctuation on the subject
> > > > line,
> > > > and try to keep the subject length <= 50 chars total.  If you have
> > > > to go
> > > > over to have a decent subject, then so be it, but we strive for that
> > > > 50
> > > > char limit to make a subject stay on one line when displayed using
> > > > git
> > > > log --oneline.
> > >
> > > 50 is really small.
> >
> > 50 is the vim syntax highlighting suggested limit.  You can go over,
> > which is why I indicated it was a soft limit, but there you are.  It
> > leaves room for the displayed hash length to grow as well.
>
> I use 75 for all text in the commit message, as per
> Documentation/process/submitting-patches.rst

checkpatch.pl will give warning if you have subject line above 75 chars.

>
> People using 'git log --oneline' should have terminals wider than 80
> :)

I like small terminals :(, it fits nicely with tiled WM on
my laptop screen.

>
> The bigger question is if the first character after the subject tag
> should be uppper case or lower case <hum>

It doesn't matter as long as person submitting patches is consistent.

Thanks

>
> Jason

Re: [PATCH] siw: Fix potential NULL pointer in siw_connect().

[Gal Pressman]

On 21/08/2019 17:12, Jason Gunthorpe wrote:
> On Wed, Aug 21, 2019 at 09:39:50AM -0400, Doug Ledford wrote:
>> On Wed, 2019-08-21 at 15:56 +0300, Dan Carpenter wrote:
>>> On Tue, Aug 20, 2019 at 12:05:33PM -0400, Doug Ledford wrote:
>>>> Please take a look (I pushed it out to my wip/dl-for-rc branch) so
>>>> you
>>>> can see what I mean about how to make both a simple subject line and
>>>> a
>>>> decent commit message.  Also, no final punctuation on the subject
>>>> line,
>>>> and try to keep the subject length <= 50 chars total.  If you have
>>>> to go
>>>> over to have a decent subject, then so be it, but we strive for that
>>>> 50
>>>> char limit to make a subject stay on one line when displayed using
>>>> git
>>>> log --oneline.
>>>
>>> 50 is really small.
>>
>> 50 is the vim syntax highlighting suggested limit.  You can go over,
>> which is why I indicated it was a soft limit, but there you are.  It
>> leaves room for the displayed hash length to grow as well.
> 
> I use 75 for all text in the commit message, as per
> Documentation/process/submitting-patches.rst
> 
> People using 'git log --oneline' should have terminals wider than 80
> :)
> 
> The bigger question is if the first character after the subject tag
> should be uppper case or lower case <hum>

I was thinking about that lately as well, it seems like git patches (which are
pretty similar to the kernel) use lower-case letter [1].

RDMA subsystem mostly sticks to capital letter though:
$ git log --oneline -- drivers/infiniband/ | egrep ": [a-z]" | wc -l
1364

$ git log --oneline -- drivers/infiniband/ | egrep ": [A-Z]" | wc -l
8069

Things look different when checking the entire tree:
$ git log --oneline | egrep ": [a-z]" | wc -l
514596

$ git log --oneline | egrep ": [A-Z]" | wc -l
356939

[1] https://github.com/git/git/blob/master/Documentation/SubmittingPatches#L118

Re: [PATCH] siw: Fix potential NULL pointer in siw_connect().

[Dan Carpenter]

On Wed, Aug 21, 2019 at 11:12:25AM -0300, Jason Gunthorpe wrote:
> On Wed, Aug 21, 2019 at 09:39:50AM -0400, Doug Ledford wrote:
> > On Wed, 2019-08-21 at 15:56 +0300, Dan Carpenter wrote:
> > > On Tue, Aug 20, 2019 at 12:05:33PM -0400, Doug Ledford wrote:
> > > > Please take a look (I pushed it out to my wip/dl-for-rc branch) so
> > > > you
> > > > can see what I mean about how to make both a simple subject line and
> > > > a
> > > > decent commit message.  Also, no final punctuation on the subject
> > > > line,
> > > > and try to keep the subject length <= 50 chars total.  If you have
> > > > to go
> > > > over to have a decent subject, then so be it, but we strive for that
> > > > 50
> > > > char limit to make a subject stay on one line when displayed using
> > > > git
> > > > log --oneline.
> > > 
> > > 50 is really small.
> > 
> > 50 is the vim syntax highlighting suggested limit.  You can go over,
> > which is why I indicated it was a soft limit, but there you are.  It
> > leaves room for the displayed hash length to grow as well.
> 
> I use 75 for all text in the commit message, as per
> Documentation/process/submitting-patches.rst
> 

My limit is 57 characters for the subject (because otherwise mutt
introduces a newline).  I sometimes go over but I'm annoyed when forced
to do that.

72 characters for the commit message because that's my limit for emails.

> People using 'git log --oneline' should have terminals wider than 80
> :)
> 
> The bigger question is if the first character after the subject tag
> should be uppper case or lower case <hum>

I feel like more and more people are moving to upper case.  There are
some people who insist on upper case and no one who insists on lower
case so it's easier to just make everything upper case.

regards,
dan carpenter

Re: [PATCH] siw: Fix potential NULL pointer in siw_connect().

[Jason Gunthorpe]

On Thu, Aug 22, 2019 at 10:10:23AM +0300, Dan Carpenter wrote:

> > People using 'git log --oneline' should have terminals wider than 80
> > :)
> > 
> > The bigger question is if the first character after the subject tag
> > should be uppper case or lower case <hum>
> 
> I feel like more and more people are moving to upper case.  There are
> some people who insist on upper case and no one who insists on lower
> case so it's easier to just make everything upper case.

I've noticed Andrew lower cases everything going through his
tree. I've always used upper case for no prarticular reason

Jason

Re: [PATCH] siw: Fix potential NULL pointer in siw_connect().

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 933 bytes --]

On Thu, 2019-08-22 at 09:02 -0300, Jason Gunthorpe wrote:
> On Thu, Aug 22, 2019 at 10:10:23AM +0300, Dan Carpenter wrote:
> 
> > > People using 'git log --oneline' should have terminals wider than
> > > 80
> > > :)
> > > 
> > > The bigger question is if the first character after the subject
> > > tag
> > > should be uppper case or lower case <hum>
> > 
> > I feel like more and more people are moving to upper case.  There
> > are
> > some people who insist on upper case and no one who insists on lower
> > case so it's easier to just make everything upper case.
> 
> I've noticed Andrew lower cases everything going through his
> tree. I've always used upper case for no prarticular reason

I prefer uppercase for the subsystem, and lowercase for the component.

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]