Re: [Patch] mm/sparse.c: Check the return value of sparse_index_alloc().

2007-11-19 Thread WANG Cong
On Mon, Nov 19, 2007 at 01:17:02PM -0800, Dave Hansen wrote:
>On Thu, 2007-11-15 at 21:54 +0800, WANG Cong wrote:
>> Since sparse_index_alloc() can return NULL on memory allocation failure,
>> we must deal with the failure condition when calling it.
>> 
>> Signed-off-by: WANG Cong <[EMAIL PROTECTED]>
>> Cc: Christoph Lameter <[EMAIL PROTECTED]>
>> Cc: Rik van Riel <[EMAIL PROTECTED]>
>> 
>> ---
>> 
>> diff --git a/Makefile b/Makefile
>> diff --git a/mm/sparse.c b/mm/sparse.c
>> index e06f514..d245e59 100644
>> --- a/mm/sparse.c
>> +++ b/mm/sparse.c
>> @@ -83,6 +83,8 @@ static int __meminit sparse_index_init(unsigned long 
>> section_nr, int nid)
>>  return -EEXIST;
>> 
>>  section = sparse_index_alloc(nid);
>> +if (!section)
>> +return -ENOMEM;
>>  /*
>>   * This lock keeps two different sections from
>>   * reallocating for the same index
>
>Oddly enough, sparse_add_one_section() doesn't seem to like to check
>its allocations.  The usemap is checked, but not freed on error.  If you
>want to fix this up, I think it needs a little more love than just two
>lines.  

Er, right. I missed this point.

>
>Do you want to try to add some actual error handling to
>sparse_add_one_section()?

Yes, I will have a try. And memory_present() also doesn't check it.
More patches around this will come up soon. Since Andrew has included
the above patch, so I won't remake it with others together.

Andrew, is this OK for you?

Thanks.



-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [Patch] mm/sparse.c: Check the return value of sparse_index_alloc().

2007-11-19 Thread Dave Hansen
On Thu, 2007-11-15 at 21:54 +0800, WANG Cong wrote:
> Since sparse_index_alloc() can return NULL on memory allocation failure,
> we must deal with the failure condition when calling it.
> 
> Signed-off-by: WANG Cong <[EMAIL PROTECTED]>
> Cc: Christoph Lameter <[EMAIL PROTECTED]>
> Cc: Rik van Riel <[EMAIL PROTECTED]>
> 
> ---
> 
> diff --git a/Makefile b/Makefile
> diff --git a/mm/sparse.c b/mm/sparse.c
> index e06f514..d245e59 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -83,6 +83,8 @@ static int __meminit sparse_index_init(unsigned long 
> section_nr, int nid)
>   return -EEXIST;
> 
>   section = sparse_index_alloc(nid);
> + if (!section)
> + return -ENOMEM;
>   /*
>* This lock keeps two different sections from
>* reallocating for the same index

Oddly enough, sparse_add_one_section() doesn't seem to like to check
its allocations.  The usemap is checked, but not freed on error.  If you
want to fix this up, I think it needs a little more love than just two
lines.  

Do you want to try to add some actual error handling to
sparse_add_one_section()?

-- Dave

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [Patch] mm/sparse.c: Check the return value of sparse_index_alloc().

2007-11-19 Thread Dave Hansen
On Thu, 2007-11-15 at 21:54 +0800, WANG Cong wrote:
 Since sparse_index_alloc() can return NULL on memory allocation failure,
 we must deal with the failure condition when calling it.
 
 Signed-off-by: WANG Cong [EMAIL PROTECTED]
 Cc: Christoph Lameter [EMAIL PROTECTED]
 Cc: Rik van Riel [EMAIL PROTECTED]
 
 ---
 
 diff --git a/Makefile b/Makefile
 diff --git a/mm/sparse.c b/mm/sparse.c
 index e06f514..d245e59 100644
 --- a/mm/sparse.c
 +++ b/mm/sparse.c
 @@ -83,6 +83,8 @@ static int __meminit sparse_index_init(unsigned long 
 section_nr, int nid)
   return -EEXIST;
 
   section = sparse_index_alloc(nid);
 + if (!section)
 + return -ENOMEM;
   /*
* This lock keeps two different sections from
* reallocating for the same index

Oddly enough, sparse_add_one_section() doesn't seem to like to check
its allocations.  The usemap is checked, but not freed on error.  If you
want to fix this up, I think it needs a little more love than just two
lines.  

Do you want to try to add some actual error handling to
sparse_add_one_section()?

-- Dave

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [Patch] mm/sparse.c: Check the return value of sparse_index_alloc().

2007-11-19 Thread WANG Cong
On Mon, Nov 19, 2007 at 01:17:02PM -0800, Dave Hansen wrote:
On Thu, 2007-11-15 at 21:54 +0800, WANG Cong wrote:
 Since sparse_index_alloc() can return NULL on memory allocation failure,
 we must deal with the failure condition when calling it.
 
 Signed-off-by: WANG Cong [EMAIL PROTECTED]
 Cc: Christoph Lameter [EMAIL PROTECTED]
 Cc: Rik van Riel [EMAIL PROTECTED]
 
 ---
 
 diff --git a/Makefile b/Makefile
 diff --git a/mm/sparse.c b/mm/sparse.c
 index e06f514..d245e59 100644
 --- a/mm/sparse.c
 +++ b/mm/sparse.c
 @@ -83,6 +83,8 @@ static int __meminit sparse_index_init(unsigned long 
 section_nr, int nid)
  return -EEXIST;
 
  section = sparse_index_alloc(nid);
 +if (!section)
 +return -ENOMEM;
  /*
   * This lock keeps two different sections from
   * reallocating for the same index

Oddly enough, sparse_add_one_section() doesn't seem to like to check
its allocations.  The usemap is checked, but not freed on error.  If you
want to fix this up, I think it needs a little more love than just two
lines.  

Er, right. I missed this point.


Do you want to try to add some actual error handling to
sparse_add_one_section()?

Yes, I will have a try. And memory_present() also doesn't check it.
More patches around this will come up soon. Since Andrew has included
the above patch, so I won't remake it with others together.

Andrew, is this OK for you?

Thanks.



-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [Patch] mm/sparse.c: Check the return value of sparse_index_alloc().

2007-11-16 Thread Andrew Morton
On Thu, 15 Nov 2007 21:54:28 +0800
WANG Cong <[EMAIL PROTECTED]> wrote:

> 
> Since sparse_index_alloc() can return NULL on memory allocation failure,
> we must deal with the failure condition when calling it.
> 
> Signed-off-by: WANG Cong <[EMAIL PROTECTED]>
> Cc: Christoph Lameter <[EMAIL PROTECTED]>
> Cc: Rik van Riel <[EMAIL PROTECTED]>
> 
> ---
> 
> diff --git a/Makefile b/Makefile
> diff --git a/mm/sparse.c b/mm/sparse.c
> index e06f514..d245e59 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -83,6 +83,8 @@ static int __meminit sparse_index_init(unsigned long 
> section_nr, int nid)
>   return -EEXIST;
>  
>   section = sparse_index_alloc(nid);
> + if (!section)
> + return -ENOMEM;
>   /*
>* This lock keeps two different sections from
>* reallocating for the same index

Sure, but both callers of sparse_index_init() ignore its return value anyway.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [Patch] mm/sparse.c: Check the return value of sparse_index_alloc().

2007-11-16 Thread Andrew Morton
On Thu, 15 Nov 2007 21:54:28 +0800
WANG Cong [EMAIL PROTECTED] wrote:

 
 Since sparse_index_alloc() can return NULL on memory allocation failure,
 we must deal with the failure condition when calling it.
 
 Signed-off-by: WANG Cong [EMAIL PROTECTED]
 Cc: Christoph Lameter [EMAIL PROTECTED]
 Cc: Rik van Riel [EMAIL PROTECTED]
 
 ---
 
 diff --git a/Makefile b/Makefile
 diff --git a/mm/sparse.c b/mm/sparse.c
 index e06f514..d245e59 100644
 --- a/mm/sparse.c
 +++ b/mm/sparse.c
 @@ -83,6 +83,8 @@ static int __meminit sparse_index_init(unsigned long 
 section_nr, int nid)
   return -EEXIST;
  
   section = sparse_index_alloc(nid);
 + if (!section)
 + return -ENOMEM;
   /*
* This lock keeps two different sections from
* reallocating for the same index

Sure, but both callers of sparse_index_init() ignore its return value anyway.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/