This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2] malloc/tst-valloc.c: Improve test coverage and use test-skeleton.c.
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: Will Newton <will dot newton at linaro dot org>
- Cc: libc-alpha at sourceware dot org, patches at linaro dot org
- Date: Wed, 02 Oct 2013 12:28:49 -0400
- Subject: Re: [PATCH v2] malloc/tst-valloc.c: Improve test coverage and use test-skeleton.c.
- Authentication-results: sourceware.org; auth=none
- References: <523042CC dot 5010305 at linaro dot org>
Will,
I'm am *very* excited to see people putting in more testing
into glibc. I really value your effort and time here. It will
make the project better. Thank you.
Having said that I'm looking for just a little more descriptions
on each of the tests here. Some comments explaining why the test
is testing what it tests go a long way to helping a QE, or any
other junior developer looking at a test failure. While you and
I have the experience to know what's wrong just a single line
comment per test sequence would help immensely.
What do you say? I know you've checked this in already, but
please consider adding some comments.
My review below (at least one cut-and-paste error):
> 2013-08-12 Will Newton <will.newton@linaro.org>
>
> * malloc/tst-valloc.c: Rewrite to use test-skeleton.c and
> improve test coverage.
> ---
> malloc/tst-valloc.c | 100 +++++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 84 insertions(+), 16 deletions(-)
>
> Changes in v2:
> - Check errno for -pagesize failure case
>
> diff --git a/malloc/tst-valloc.c b/malloc/tst-valloc.c
> index 643a0dd..3648f2f 100644
> --- a/malloc/tst-valloc.c
> +++ b/malloc/tst-valloc.c
> @@ -1,23 +1,91 @@
> -/* Test case by Stephen Tweedie <sct@redhat.com>. */
> -#include <unistd.h>
> -#include <stdio.h>
> +/* Copyright (C) 2013 Free Software Foundation, Inc.
Complete rewrite so 2013 is OK.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +#include <errno.h>
> #include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <unistd.h>
>
> -int
> -main (void)
> +static int errors = 0;
> +
> +static void
> +merror (const char *msg)
> +{
> + ++errors;
> + printf ("Error: %s\n", msg);
> +}
> +
> +static int
> +do_test (void)
> {
> - char *p;
> - int pagesize = getpagesize ();
> - int i;
> + void *p;
> + unsigned long pagesize = getpagesize();
> + unsigned long ptrval;
> + int save;
> +
> + errno = 0;
> +
> + p = valloc (-1);
> +
> + save = errno;
>
> - p = valloc (pagesize);
> - i = (long int) p;
> + if (p != NULL)
> + merror ("valloc (-1) succeeded.");
>
> - if ((i & (pagesize-1)) != 0)
> - {
> - fprintf (stderr, "Alignment problem: valloc returns %p\n", p);
> - exit (1);
> - }
> + if (p == NULL && save != ENOMEM)
> + merror ("valloc (-1) errno is not set correctly");
OK, modulo comments describing test, and free (p).
> - return 0;
> + errno = 0;
> +
> + p = valloc (-pagesize);
Why test with -pagesize?
> +
> + save = errno;
> +
> + if (p != NULL)
> + merror ("valloc (-pagesize) succeeded.");
> +
> + if (p == NULL && save != ENOMEM)
> + merror ("valloc (-pagesize) errno is not set correctly");
> +
OK, modulo comments describing test, and free (p).
> + p = valloc (0);
> +
> + if (p == NULL)
> + merror ("valloc (0) failed.");
> +
> + free (p);
> +
OK, modulo comments describing test.
> + p = valloc (32);
> +
> + if (p == NULL)
> + merror ("valloc (32) failed.");
> +
> + ptrval = (unsigned long)p;
> +
> + if (p == NULL)
> + merror ("valloc (32) failed.");
Duplicate check? Cut and paste error?
> +
> + if ((ptrval & (pagesize - 1)) != 0)
> + merror ("returned pointer is not page aligned.");
OK, modulo comments describing test.
> +
> + free (p);
> +
> + return errors != 0;
> }
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
>
Cheers,
Carlos.