diff mbox

[1/1] tty: vt: Remove redundant null check before kfree.

Message ID 1353475147-28308-1-git-send-email-sachin.kamat@linaro.org
State Accepted
Headers show

Commit Message

Sachin Kamat Nov. 21, 2012, 5:19 a.m. UTC
kfree on a NULL pointer is a no-op.

Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/tty/vt/consolemap.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

Comments

Greg KH Nov. 21, 2012, 2:46 p.m. UTC | #1
On Wed, Nov 21, 2012 at 10:49:07AM +0530, Sachin Kamat wrote:
> kfree on a NULL pointer is a no-op.
> 
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> ---
>  drivers/tty/vt/consolemap.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
> index 2aaa0c2..248381b 100644
> --- a/drivers/tty/vt/consolemap.c
> +++ b/drivers/tty/vt/consolemap.c
> @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p)
>  		kfree(p->inverse_translations[i]);
>  		p->inverse_translations[i] = NULL;
>  	}
> -	if (p->inverse_trans_unicode) {
> -		kfree(p->inverse_trans_unicode);
> -		p->inverse_trans_unicode = NULL;
> -	}
> +	kfree(p->inverse_trans_unicode);
> +	p->inverse_trans_unicode = NULL;

kfree with NULL is a no-op, but the line after that just caused a kernel
crash if it was NULL, so I can't accept this type of thing.

Please be more careful.

What's with the patches@linaro.org email address?  What is that for?

thanks,

greg k-h
Sachin Kamat Nov. 21, 2012, 2:51 p.m. UTC | #2
On 21 November 2012 20:16, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Nov 21, 2012 at 10:49:07AM +0530, Sachin Kamat wrote:
>> kfree on a NULL pointer is a no-op.
>>
>> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
>> ---
>>  drivers/tty/vt/consolemap.c |    6 ++----
>>  1 files changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
>> index 2aaa0c2..248381b 100644
>> --- a/drivers/tty/vt/consolemap.c
>> +++ b/drivers/tty/vt/consolemap.c
>> @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p)
>>               kfree(p->inverse_translations[i]);
>>               p->inverse_translations[i] = NULL;
>>       }
>> -     if (p->inverse_trans_unicode) {
>> -             kfree(p->inverse_trans_unicode);
>> -             p->inverse_trans_unicode = NULL;
>> -     }
>> +     kfree(p->inverse_trans_unicode);
>> +     p->inverse_trans_unicode = NULL;
>
> kfree with NULL is a no-op, but the line after that just caused a kernel
> crash if it was NULL, so I can't accept this type of thing.
>
> Please be more careful.

My mistake. Apologies for the same.

Do we need to assign the pointer to NULL after freeing?

>
> What's with the patches@linaro.org email address?  What is that for?
That is a logging mechanism (done by patchwork) for all patches sent
by Linaro engineers.

>
> thanks,
>
> greg k-h
Greg KH Nov. 21, 2012, 3:16 p.m. UTC | #3
On Wed, Nov 21, 2012 at 08:21:40PM +0530, Sachin Kamat wrote:
> On 21 November 2012 20:16, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Nov 21, 2012 at 10:49:07AM +0530, Sachin Kamat wrote:
> >> kfree on a NULL pointer is a no-op.
> >>
> >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> >> ---
> >>  drivers/tty/vt/consolemap.c |    6 ++----
> >>  1 files changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
> >> index 2aaa0c2..248381b 100644
> >> --- a/drivers/tty/vt/consolemap.c
> >> +++ b/drivers/tty/vt/consolemap.c
> >> @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p)
> >>               kfree(p->inverse_translations[i]);
> >>               p->inverse_translations[i] = NULL;
> >>       }
> >> -     if (p->inverse_trans_unicode) {
> >> -             kfree(p->inverse_trans_unicode);
> >> -             p->inverse_trans_unicode = NULL;
> >> -     }
> >> +     kfree(p->inverse_trans_unicode);
> >> +     p->inverse_trans_unicode = NULL;
> >
> > kfree with NULL is a no-op, but the line after that just caused a kernel
> > crash if it was NULL, so I can't accept this type of thing.
> >
> > Please be more careful.
> 
> My mistake. Apologies for the same.
> 
> Do we need to assign the pointer to NULL after freeing?

It depends if it is checked later on, is it?

> > What's with the patches@linaro.org email address?  What is that for?
> That is a logging mechanism (done by patchwork) for all patches sent
> by Linaro engineers.

So you can track it internally?  Why?

greg k-h
Sachin Kamat Nov. 21, 2012, 3:29 p.m. UTC | #4
On 21 November 2012 20:46, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Nov 21, 2012 at 08:21:40PM +0530, Sachin Kamat wrote:
>> On 21 November 2012 20:16, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Wed, Nov 21, 2012 at 10:49:07AM +0530, Sachin Kamat wrote:
>> >> kfree on a NULL pointer is a no-op.
>> >>
>> >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
>> >> ---
>> >>  drivers/tty/vt/consolemap.c |    6 ++----
>> >>  1 files changed, 2 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
>> >> index 2aaa0c2..248381b 100644
>> >> --- a/drivers/tty/vt/consolemap.c
>> >> +++ b/drivers/tty/vt/consolemap.c
>> >> @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p)
>> >>               kfree(p->inverse_translations[i]);
>> >>               p->inverse_translations[i] = NULL;
>> >>       }
>> >> -     if (p->inverse_trans_unicode) {
>> >> -             kfree(p->inverse_trans_unicode);
>> >> -             p->inverse_trans_unicode = NULL;
>> >> -     }
>> >> +     kfree(p->inverse_trans_unicode);
>> >> +     p->inverse_trans_unicode = NULL;
>> >
>> > kfree with NULL is a no-op, but the line after that just caused a kernel
>> > crash if it was NULL, so I can't accept this type of thing.
>> >
>> > Please be more careful.
>>
>> My mistake. Apologies for the same.
>>
>> Do we need to assign the pointer to NULL after freeing?
>
> It depends if it is checked later on, is it?

I will re-send this patch after deleting the assignment?

>
>> > What's with the patches@linaro.org email address?  What is that for?
>> That is a logging mechanism (done by patchwork) for all patches sent
>> by Linaro engineers.
>
> So you can track it internally?  Why?

Yes. For tracking the status.
>
> greg k-h
Greg KH Nov. 21, 2012, 4:54 p.m. UTC | #5
On Wed, Nov 21, 2012 at 08:59:08PM +0530, Sachin Kamat wrote:
> On 21 November 2012 20:46, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Nov 21, 2012 at 08:21:40PM +0530, Sachin Kamat wrote:
> >> On 21 November 2012 20:16, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> > On Wed, Nov 21, 2012 at 10:49:07AM +0530, Sachin Kamat wrote:
> >> >> kfree on a NULL pointer is a no-op.
> >> >>
> >> >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> >> >> ---
> >> >>  drivers/tty/vt/consolemap.c |    6 ++----
> >> >>  1 files changed, 2 insertions(+), 4 deletions(-)
> >> >>
> >> >> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
> >> >> index 2aaa0c2..248381b 100644
> >> >> --- a/drivers/tty/vt/consolemap.c
> >> >> +++ b/drivers/tty/vt/consolemap.c
> >> >> @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p)
> >> >>               kfree(p->inverse_translations[i]);
> >> >>               p->inverse_translations[i] = NULL;
> >> >>       }
> >> >> -     if (p->inverse_trans_unicode) {
> >> >> -             kfree(p->inverse_trans_unicode);
> >> >> -             p->inverse_trans_unicode = NULL;
> >> >> -     }
> >> >> +     kfree(p->inverse_trans_unicode);
> >> >> +     p->inverse_trans_unicode = NULL;
> >> >
> >> > kfree with NULL is a no-op, but the line after that just caused a kernel
> >> > crash if it was NULL, so I can't accept this type of thing.
> >> >
> >> > Please be more careful.
> >>
> >> My mistake. Apologies for the same.
> >>
> >> Do we need to assign the pointer to NULL after freeing?
> >
> > It depends if it is checked later on, is it?
> 
> I will re-send this patch after deleting the assignment?

You tell me, please determine if it really is correct or not, before
sending a patch.  You did test this, right?

greg k-h
Jiri Slaby Nov. 21, 2012, 10:43 p.m. UTC | #6
On 11/21/2012 03:46 PM, Greg KH wrote:
> On Wed, Nov 21, 2012 at 10:49:07AM +0530, Sachin Kamat wrote:
>> --- a/drivers/tty/vt/consolemap.c
>> +++ b/drivers/tty/vt/consolemap.c
>> @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p)
>>  		kfree(p->inverse_translations[i]);
>>  		p->inverse_translations[i] = NULL;
>>  	}
>> -	if (p->inverse_trans_unicode) {
>> -		kfree(p->inverse_trans_unicode);
>> -		p->inverse_trans_unicode = NULL;
>> -	}
>> +	kfree(p->inverse_trans_unicode);
>> +	p->inverse_trans_unicode = NULL;
> 
> kfree with NULL is a no-op, but the line after that just caused a kernel
> crash if it was NULL, so I can't accept this type of thing.

Greg, I'm not sure -- what do you mean here? The change actually looks
fine to me... We do not dereference p->inverse_trans_unicode there.

thanks,
Greg KH Nov. 21, 2012, 11 p.m. UTC | #7
On Wed, Nov 21, 2012 at 11:43:34PM +0100, Jiri Slaby wrote:
> On 11/21/2012 03:46 PM, Greg KH wrote:
> > On Wed, Nov 21, 2012 at 10:49:07AM +0530, Sachin Kamat wrote:
> >> --- a/drivers/tty/vt/consolemap.c
> >> +++ b/drivers/tty/vt/consolemap.c
> >> @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p)
> >>  		kfree(p->inverse_translations[i]);
> >>  		p->inverse_translations[i] = NULL;
> >>  	}
> >> -	if (p->inverse_trans_unicode) {
> >> -		kfree(p->inverse_trans_unicode);
> >> -		p->inverse_trans_unicode = NULL;
> >> -	}
> >> +	kfree(p->inverse_trans_unicode);
> >> +	p->inverse_trans_unicode = NULL;
> > 
> > kfree with NULL is a no-op, but the line after that just caused a kernel
> > crash if it was NULL, so I can't accept this type of thing.
> 
> Greg, I'm not sure -- what do you mean here? The change actually looks
> fine to me... We do not dereference p->inverse_trans_unicode there.

If we never dereference it, why is it being set to NULL?

greg k-h
Jiri Slaby Nov. 21, 2012, 11:17 p.m. UTC | #8
On 11/22/2012 12:00 AM, Greg KH wrote:
> On Wed, Nov 21, 2012 at 11:43:34PM +0100, Jiri Slaby wrote:
>> On 11/21/2012 03:46 PM, Greg KH wrote:
>>> On Wed, Nov 21, 2012 at 10:49:07AM +0530, Sachin Kamat wrote:
>>>> --- a/drivers/tty/vt/consolemap.c
>>>> +++ b/drivers/tty/vt/consolemap.c
>>>> @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p)
>>>>  		kfree(p->inverse_translations[i]);
>>>>  		p->inverse_translations[i] = NULL;
>>>>  	}
>>>> -	if (p->inverse_trans_unicode) {
>>>> -		kfree(p->inverse_trans_unicode);
>>>> -		p->inverse_trans_unicode = NULL;
>>>> -	}
>>>> +	kfree(p->inverse_trans_unicode);
>>>> +	p->inverse_trans_unicode = NULL;
>>>
>>> kfree with NULL is a no-op, but the line after that just caused a kernel
>>> crash if it was NULL, so I can't accept this type of thing.
>>
>> Greg, I'm not sure -- what do you mean here? The change actually looks
>> fine to me... We do not dereference p->inverse_trans_unicode there.
> 
> If we never dereference it, why is it being set to NULL?

I'm not disputing the assignment. Let it there. But I do not see any
issues with the patch. What problem do you see exactly?
Greg KH Nov. 22, 2012, 12:38 a.m. UTC | #9
On Thu, Nov 22, 2012 at 12:17:00AM +0100, Jiri Slaby wrote:
> On 11/22/2012 12:00 AM, Greg KH wrote:
> > On Wed, Nov 21, 2012 at 11:43:34PM +0100, Jiri Slaby wrote:
> >> On 11/21/2012 03:46 PM, Greg KH wrote:
> >>> On Wed, Nov 21, 2012 at 10:49:07AM +0530, Sachin Kamat wrote:
> >>>> --- a/drivers/tty/vt/consolemap.c
> >>>> +++ b/drivers/tty/vt/consolemap.c
> >>>> @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p)
> >>>>  		kfree(p->inverse_translations[i]);
> >>>>  		p->inverse_translations[i] = NULL;
> >>>>  	}
> >>>> -	if (p->inverse_trans_unicode) {
> >>>> -		kfree(p->inverse_trans_unicode);
> >>>> -		p->inverse_trans_unicode = NULL;
> >>>> -	}
> >>>> +	kfree(p->inverse_trans_unicode);
> >>>> +	p->inverse_trans_unicode = NULL;
> >>>
> >>> kfree with NULL is a no-op, but the line after that just caused a kernel
> >>> crash if it was NULL, so I can't accept this type of thing.
> >>
> >> Greg, I'm not sure -- what do you mean here? The change actually looks
> >> fine to me... We do not dereference p->inverse_trans_unicode there.
> > 
> > If we never dereference it, why is it being set to NULL?
> 
> I'm not disputing the assignment. Let it there. But I do not see any
> issues with the patch. What problem do you see exactly?

{sigh}  Nothing.  I don't know what I was thinking, my apologies
Sachin, there's nothing wrong with this patch.

I blame my fried brain, after reviewing all of those __dev* removal
patches :)

Sachin, can you resend these patches so I can apply them?

thanks,

greg k-h
Sachin Kamat Nov. 22, 2012, 5:19 a.m. UTC | #10
On 22 November 2012 06:08, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Nov 22, 2012 at 12:17:00AM +0100, Jiri Slaby wrote:
>> On 11/22/2012 12:00 AM, Greg KH wrote:
>> > On Wed, Nov 21, 2012 at 11:43:34PM +0100, Jiri Slaby wrote:
>> >> On 11/21/2012 03:46 PM, Greg KH wrote:
>> >>> On Wed, Nov 21, 2012 at 10:49:07AM +0530, Sachin Kamat wrote:
>> >>>> --- a/drivers/tty/vt/consolemap.c
>> >>>> +++ b/drivers/tty/vt/consolemap.c
>> >>>> @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p)
>> >>>>                  kfree(p->inverse_translations[i]);
>> >>>>                  p->inverse_translations[i] = NULL;
>> >>>>          }
>> >>>> -        if (p->inverse_trans_unicode) {
>> >>>> -                kfree(p->inverse_trans_unicode);
>> >>>> -                p->inverse_trans_unicode = NULL;
>> >>>> -        }
>> >>>> +        kfree(p->inverse_trans_unicode);
>> >>>> +        p->inverse_trans_unicode = NULL;
>> >>>
>> >>> kfree with NULL is a no-op, but the line after that just caused a kernel
>> >>> crash if it was NULL, so I can't accept this type of thing.
>> >>
>> >> Greg, I'm not sure -- what do you mean here? The change actually looks
>> >> fine to me... We do not dereference p->inverse_trans_unicode there.
>> >
>> > If we never dereference it, why is it being set to NULL?
>>
>> I'm not disputing the assignment. Let it there. But I do not see any
>> issues with the patch. What problem do you see exactly?
>
> {sigh}  Nothing.  I don't know what I was thinking, my apologies
> Sachin, there's nothing wrong with this patch.
>
> I blame my fried brain, after reviewing all of those __dev* removal
> patches :)
>
> Sachin, can you resend these patches so I can apply them?

No problem Greg. I will send them again.

>
> thanks,
>
> greg k-h
diff mbox

Patch

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 2aaa0c2..248381b 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -410,10 +410,8 @@  static void con_release_unimap(struct uni_pagedir *p)
 		kfree(p->inverse_translations[i]);
 		p->inverse_translations[i] = NULL;
 	}
-	if (p->inverse_trans_unicode) {
-		kfree(p->inverse_trans_unicode);
-		p->inverse_trans_unicode = NULL;
-	}
+	kfree(p->inverse_trans_unicode);
+	p->inverse_trans_unicode = NULL;
 }
 
 /* Caller must hold the console lock */