On Fri, Oct 24, 2008 at 02:01:58PM +0100, Mel Gorman wrote:
> On Thu, Oct 23, 2008 at 03:54:59PM +0100, Andy Whitcroft wrote:
> > gethugepagesizes has two modes: a counting mode which tells us exactly
> > how many entries we would need; and a lookup mode which fills in a
> > supplied buffer.
> >
> > The counting mode is defined as triggering when we pass in both a zero
> > element count and a NULL buffer pointer. Currently we enter this mode when
> > the element count is zero which causes it to trigger at the edge case where
> > the caller (pointlessly) passes in a buffer with an element count of zero.
> > Fix behaviour to match the documentation.
> >
> > Signed-off-by: Andy Whitcroft <[EMAIL PROTECTED]>
> > ---
> > hugeutils.c | 19 ++++++++++---------
> > 1 files changed, 10 insertions(+), 9 deletions(-)
> >
> > diff --git a/hugeutils.c b/hugeutils.c
> > index ae60f39..eb0b7b2 100644
> > --- a/hugeutils.c
> > +++ b/hugeutils.c
> > @@ -581,11 +581,11 @@ int gethugepagesizes(long pagesizes[], int n_elem)
> > if (default_size < 0)
> > return 0;
> >
> > - if (n_elem && pagesizes)
> > + if (pagesizes && (nr_sizes == n_elem))
> > + return nr_sizes;
>
> Ok, this is a little confusing because nr_sizes == 0 - can't we just go
> pagesizes && n_elem == 0 ?
well its consistent with the other version of this check in this
function which must use nr_sizes.
> Otherwise, the behaviour appears to match the documentation. If you
> supply a buffer with zero elements, then 0 pagesizes are put in the
> buffer.
>
> > + if (pagesizes)
> > pagesizes[nr_sizes] = default_size;
>
> I'm not seeing why you have to check pagesizes here. Earlier in the
> function, we already check it for EINVAL and the check above would exit
> if n_elem was 0 so we know the array is valid at this point, right?
If we are in counting mode, pagesizes is null and we cannot write to it,
but we must continue to accumulate nr_sizes.
> > nr_sizes++;
> > - if (n_elem && (nr_sizes == n_elem))
> > - return nr_sizes;
> >
> > /*
> > * Scan sysfs to look for other sizes.
> > @@ -599,8 +599,7 @@ int gethugepagesizes(long pagesizes[], int n_elem)
> > } else
> > return -1;
> > }
> > - while ((ent = readdir(sysfs)) &&
> > - ((n_elem == 0) || (nr_sizes < n_elem))) {
> > + while ((ent = readdir(sysfs))) {
> > long size;
> >
> > if (strncmp(ent->d_name, "hugepages-", 10))
> > @@ -613,7 +612,9 @@ int gethugepagesizes(long pagesizes[], int n_elem)
> >
> > if (size < 0 || size == default_size)
> > continue;
> > - if (n_elem && pagesizes)
> > + if (pagesizes && (nr_sizes == n_elem))
> > + return nr_sizes;
> > + if (pagesizes)
> > pagesizes[nr_sizes] = size;
>
> Can we really reach here with pagesizes == NULL?
Yes, when we call this n_elem 0, pagesizes = NULL we are in counting
mode and iterate over all entries just counting nr_sizes.
> > nr_sizes++;
> > }
> > @@ -636,10 +637,10 @@ int getpagesizes(long pagesizes[], int n_elem)
> > ret = gethugepagesizes(pagesizes, n_elem);
> > } else {
> > /* Install the base page size. */
> > - if (n_elem && pagesizes)
> > + if (pagesizes && n_elem == 0)
> > + return 0;
>
> Here the test is easier to follow.
And I am trying to be consistant with here for that clarity.
> > + if (pagesizes)
> > pagesizes[0] = sysconf(_SC_PAGESIZE);
> > - if (n_elem == 1)
> > - return 1;
> >
> > ret = gethugepagesizes(pagesizes + 1, n_elem - 1);
> > }
-apw
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Libhugetlbfs-devel mailing list
Libhugetlbfs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libhugetlbfs-devel
