kfree on a NULL pointer is a no-op.
Signed-off-by: Sachin Kamat <[email protected]>
---
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;
}
/* Caller must hold the console lock */
--
1.7.4.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 <[email protected]>
> ---
> 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 [email protected] email address? What is that for?
thanks,
greg k-h
On 21 November 2012 20:16, Greg KH <[email protected]> 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 <[email protected]>
>> ---
>> 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 [email protected] 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
--
With warm regards,
Sachin
On Wed, Nov 21, 2012 at 08:21:40PM +0530, Sachin Kamat wrote:
> On 21 November 2012 20:16, Greg KH <[email protected]> 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 <[email protected]>
> >> ---
> >> 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 [email protected] 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
On 21 November 2012 20:46, Greg KH <[email protected]> wrote:
> On Wed, Nov 21, 2012 at 08:21:40PM +0530, Sachin Kamat wrote:
>> On 21 November 2012 20:16, Greg KH <[email protected]> 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 <[email protected]>
>> >> ---
>> >> 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 [email protected] 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
--
With warm regards,
Sachin
On Wed, Nov 21, 2012 at 08:59:08PM +0530, Sachin Kamat wrote:
> On 21 November 2012 20:46, Greg KH <[email protected]> wrote:
> > On Wed, Nov 21, 2012 at 08:21:40PM +0530, Sachin Kamat wrote:
> >> On 21 November 2012 20:16, Greg KH <[email protected]> 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 <[email protected]>
> >> >> ---
> >> >> 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
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?
--
js
suse labs
On 22 November 2012 06:08, Greg KH <[email protected]> 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
--
With warm regards,
Sachin
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,
--
js
suse labs
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
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