Re: [LTP] [PATCH v2 1/2] mlock/mlock02.c: cleanup

[Jan Stancek <jstancek@redhat.com>]

----- Original Message -----
> From: "Zeng Linggang" <zenglg.jy@cn.fujitsu.com>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: "ltp-list" <ltp-list@lists.sourceforge.net>
> Sent: Wednesday, 19 February, 2014 10:38:36 AM
> Subject: [PATCH v2 1/2] mlock/mlock02.c: cleanup
> 
> * Delete some useless commtents.
> * Move the test body form main() to mlock_verify().
> * Some cleanup.
> 
> Signed-off-by: Zeng Linggang <zenglg.jy@cn.fujitsu.com>

Hi,

I have trouble compiling this patch (comments inline):
$ make
make -C "/usr/src/ltp/testcases/kernel/include" -f "/usr/src/ltp/testcases/kernel/include/Makefile" all
make[1]: Entering directory `/usr/src/ltp/testcases/kernel/include'
make[1]: Nothing to be done for `all'.
make[1]: Leaving directory `/usr/src/ltp/testcases/kernel/include'
gcc -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -W -D_FORTIFY_SOURCE=2 -I/usr/src/ltp/testcases/kernel/include -I../../../../include -I../../../../include   -L../../../../lib  mlock02.c   -lltp -o mlock02
mlock02.c: In function ‘mlock_verify’:
mlock02.c:90: error: void value not ignored as it ought to be
make: *** [mlock02] Error 1

> ---
>  testcases/kernel/syscalls/mlock/mlock02.c | 143
>  +++++++++++++-----------------
>  1 file changed, 63 insertions(+), 80 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/mlock/mlock02.c
> b/testcases/kernel/syscalls/mlock/mlock02.c
> index edd9e45..1d1c853 100644
> --- a/testcases/kernel/syscalls/mlock/mlock02.c
> +++ b/testcases/kernel/syscalls/mlock/mlock02.c
> @@ -1,6 +1,6 @@
>  /*
> - *
>   *   Copyright (c) International Business Machines  Corp., 2002
> + *	06/2002 Written by Paul Larson
>   *
>   *   This program is free software;  you can redistribute it and/or modify
>   *   it under the terms of the GNU General Public License as published by
> @@ -13,70 +13,48 @@
>   *   the GNU General Public License for more details.
>   *
>   *   You should have received a copy of the GNU General Public License
> - *   along with this program;  if not, write to the Free Software
> - *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301 USA
> + *   along with this program;  if not, write to the Free Software
> Foundation,
> + *   Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>   */
> -
>  /*
> - * NAME
> - * 	mlock02.c
> - *
> - * DESCRIPTION
> - * 	Test to see the proper errors are returned by mlock
> - *$
>   * ALGORITHM
>   * 	test 1:
>   *		Call mlock with a NULL address.  ENOMEM should be returned
> - *
> - * USAGE:  <for command-line>
> - *         -c n    Run n copies concurrently
> - *         -e      Turn on errno logging
> - *         -f      Turn off functional testing
> - *         -h      Show this help screen
> - *         -i n    Execute test n times
> - *         -I x    Execute test for x seconds
> - *         -p      Pause for SIGUSR1 before starting
> - *         -P x    Pause for x seconds between iterations
> - *         -t      Turn on syscall timing
> - *
> - * HISTORY
> - *	06/2002 Written by Paul Larson
> - *
> - * RESTRICTIONS
> - * 	None
>   */
> +
>  #include <errno.h>
>  #include <unistd.h>
>  #include <sys/mman.h>
>  #include "test.h"
>  #include "usctest.h"
>  
> -void setup();
> -void setup1();
> -void cleanup();
> -
>  char *TCID = "mlock02";
> -int TST_TOTAL = 1;
> -
> -int exp_enos[] = { ENOMEM, 0 };
>  
> -void *addr1;
> +#if !defined(UCLINUX)
>  
>  struct test_case_t {
>  	void **addr;
>  	int len;
>  	int error;
>  	void (*setupfunc) ();

should list parameters or void

> -} TC[] = {
> -	/* mlock should return ENOMEM when some or all of the address
> -	 * range pointed to by addr and len are not valid mapped pages
> -	 * in the address space of the process
> -	 */
> -	{
> -	&addr1, 1024, ENOMEM, setup1}
>  };
>  
> -#if !defined(UCLINUX)
> +static void *addr1;
> +static void setup(void);
> +#ifdef __ia64__
> +static void setup1(const struct test_case_t *);

Why different prototype for ia64?
I don't think this parameter should be const, it's likely setupfunc()
will want to modify the struct.

> +#else
> +static void setup1(void);
> +#endif
> +static void cleanup(void);
> +static void mlock_verify(const struct test_case_t *);
> +
> +static struct test_case_t TC[] = {
> +	{&addr1, 1024, ENOMEM, setup1},
> +};
> +
> +int TST_TOTAL = ARRAY_SIZE(TC);
> +static int exp_enos[] = { ENOMEM, 0 };
>  
>  int main(int ac, char **av)
>  {
> @@ -91,62 +69,67 @@ int main(int ac, char **av)
>  	TEST_EXP_ENOS(exp_enos);
>  
>  	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -
>  		tst_count = 0;
> -
> -		for (i = 0; i < TST_TOTAL; i++) {
> -
> -			if (TC[i].setupfunc != NULL)
> -				TC[i].setupfunc();
> -
> -			TEST(mlock(*(TC[i].addr), TC[i].len));
> -
> -			if (TEST_RETURN == -1) {
> -				if (TEST_ERRNO != TC[i].error)
> -					tst_brkm(TFAIL | TTERRNO, cleanup,
> -						 "mlock didn't fail as expected; "
> -						 "expected - %d : %s",
> -						 TC[i].error,
> -						 strerror(TC[i].error));
> -				else
> -					tst_resm(TPASS | TTERRNO,
> -						 "mlock failed as expected");
> -			} else
> -				tst_brkm(TFAIL, cleanup,
> -					 "mlock succeeded unexpectedly");
> -		}
> +		for (i = 0; i < TST_TOTAL; i++)
> +			mlock_verify(&TC[i]);
>  	}
>  
>  	cleanup();
> -
>  	tst_exit();
>  }
>  
> -#else
> -
> -int main()
> +static void setup(void)
>  {
> -	tst_brkm(TCONF, NULL, "test is not available on uClinux");
> -}
> -
> -#endif /* if !defined(UCLINUX) */
> +	tst_sig(NOFORK, DEF_HANDLER, cleanup);
>  
> -void setup()
> -{
>  	TEST_PAUSE;
>  }
>  
> -void setup1()
> +static void mlock_verify(const struct test_case_t *test)
>  {
> +	if (test->setupfunc(test) == -1)

setupfunc returns void

> +		return;
> +
> +	TEST(mlock(*(test->addr), test->len));
> +
> +	if (TEST_RETURN != -1) {
> +		tst_resm(TFAIL, "mlock succeeded unexpectedly");
> +		return;
> +	}
> +
> +	if (TEST_ERRNO != test->error) {
> +		tst_resm(TFAIL | TTERRNO,
> +			 "mlock didn't fail as expected; expected - %d : %s",
> +			 test->error, strerror(test->error));
> +	} else {
> +		tst_resm(TPASS | TTERRNO, "mlock failed as expected");
> +	}
> +}
> +
>  #ifdef __ia64__
> -	TC[0].len = getpagesize() + 1;
> +static void setup1(const struct test_case_t *test)
> +{
> +	test->len = getpagesize() + 1;

Not relevant to this cleanup patch: Any idea why ia64 needs this setup? 

> +}
>  #else
> +static void setup1(void)
> +{
>  	addr1 = NULL;

If you had same prototype you could also do:
  *(test->addr) = NULL;
which is not needed for this single testcase, but I'm assuming part 2
of the patchset will modify addr1, so resetting value will be needed.

> -#endif
>  }
> +#endif
>  
> -void cleanup()
> +static void cleanup(void)
>  {
>  	TEST_CLEANUP;
> +}
> +
> +#else
>  
> +int TST_TOTAL = 1;
> +
> +int main(void)
> +{
> +	tst_brkm(TCONF, NULL, "test is not available on uClinux");
>  }
> +
> +#endif /* if !defined(UCLINUX) */
> --
> 1.8.4.2
> 
> 

Here is patch which made it compile-able for me:

diff --git a/testcases/kernel/syscalls/mlock/mlock02.c b/testcases/kernel/syscalls/mlock/mlock02.c
index 1d1c853..cfa9176 100644
--- a/testcases/kernel/syscalls/mlock/mlock02.c
+++ b/testcases/kernel/syscalls/mlock/mlock02.c
@@ -36,18 +36,14 @@ struct test_case_t {
 	void **addr;
 	int len;
 	int error;
-	void (*setupfunc) ();
+	int (*setupfunc) (struct test_case_t *);
 };
 
 static void *addr1;
 static void setup(void);
-#ifdef __ia64__
-static void setup1(const struct test_case_t *);
-#else
-static void setup1(void);
-#endif
+static int setup1(struct test_case_t *);
 static void cleanup(void);
-static void mlock_verify(const struct test_case_t *);
+static void mlock_verify(struct test_case_t *);
 
 static struct test_case_t TC[] = {
 	{&addr1, 1024, ENOMEM, setup1},
@@ -85,7 +81,7 @@ static void setup(void)
 	TEST_PAUSE;
 }
 
-static void mlock_verify(const struct test_case_t *test)
+static void mlock_verify(struct test_case_t *test)
 {
 	if (test->setupfunc(test) == -1)
 		return;
@@ -106,17 +102,14 @@ static void mlock_verify(const struct test_case_t *test)
 	}
 }
 
-#ifdef __ia64__
-static void setup1(const struct test_case_t *test)
+static int setup1(struct test_case_t *test)
 {
+#ifdef __ia64__
 	test->len = getpagesize() + 1;
-}
-#else
-static void setup1(void)
-{
-	addr1 = NULL;
-}
 #endif
+	*(test->addr) = NULL;
+	return 0;
+}
 
 static void cleanup(void)
 {

Regards,
Jan

------------------------------------------------------------------------------
Managing the Performance of Cloud-Based Applications
Take advantage of what the Cloud has to offer - Avoid Common Pitfalls.
Read the Whitepaper.
http://pubads.g.doubleclick.net/gampad/clk?id=121054471&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list