Menu
▾
▴
Re: [LTP] [PATCH v2 1/5] hugemmap01: coding style cleanups
|
From: Caspar Z. <ca...@ca...> - 2012-02-02 08:51:07
|
On 01/30/2012 11:30 AM, Wanlong Gao wrote:
> cleanup the coding style
>
> Signed-off-by: Wanlong Gao <gao...@cn...>
> ---
> testcases/kernel/mem/hugetlb/hugemmap/hugemmap01.c | 158 +++++++++-----------
> 1 files changed, 71 insertions(+), 87 deletions(-)
>
> diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap01.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap01.c
> index 874f736..209265e 100644
> --- a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap01.c
> +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap01.c
> @@ -33,29 +33,17 @@
> * Pause for SIGUSR1 if option specified.
> * Create temporary directory.
> *
> - * Test:
> - * Loop if the proper options are given.
> - * Execute system call
> - * Check return code, if system call failed (return=-1)
> - * Log the errno and Issue a FAIL message.
> - * Cleanup:
> - * Print timing stats if options given
> - * Delete the temporary directory created.
> - *
> - * Usage: <for command-line>
> - * hugemmap01 [-c n] [-f] [-i n] [-I x] [-P x] [-t]
> - * where, -c n : Run n copies concurrently.
> - * -f : Turn off functionality Testing.
> - * -i n : Execute test n times.
> - * -I x : Execute test for x seconds.
> - * -P x : Pause for x seconds between iterations.
> - * -t : Turn on syscall timing.
> + * Test:
> + * Loop if the proper options are given.
> + * Execute system call
> + * Check return code, if system call failed (return=-1)
> + * Log the errno and Issue a FAIL message.
> + * Cleanup:
> + * Print timing stats if options given
> + * Delete the temporary directory created.
> *
> * HISTORY
> - * 04/2004 Written by Robbie Williamson
> - *
> - * RESTRICTIONS:
> - * None.
> + * 04/2004 Written by Robbie Williamson
> */
>
> #include <stdio.h>
> @@ -76,62 +64,53 @@
>
> #define BUFFER_SIZE 256
This one is unused, even if it is used, you can replace it with BUFSIZ
>
> -char* TEMPFILE="mmapfile";
> +static char *TEMPFILE = "mmapfile";
>
> -char *TCID="hugemmap01"; /* Test program identifier. */
> -int TST_TOTAL=1; /* Total number of test cases. */
> -long *addr; /* addr of memory mapped region */
> -int fildes; /* file descriptor for tempfile */
> -char *Hopt; /* location of hugetlbfs */
> -int beforetest=0; /* Amount of free huge pages before testing */
> -int aftertest=0; /* Amount of free huge pages after testing */
> -int hugepagesmapped=0; /* Amount of huge pages mapped after testing */
> +char *TCID = "hugemmap01";
> +int TST_TOTAL = 1;
> +static long *addr; /* addr of memory mapped region */
> +static int fildes; /* file descriptor for tempfile */
> +static char *Hopt; /* location of hugetlbfs */
> +static int beforetest; /* Amount of free huge pages before testing */
> +static int aftertest; /* Amount of free huge pages after testing */
> +static int hugepagesmapped; /* Amount of huge pages mapped after testing */
I think these comments are not necessary either
>
> -void setup(); /* Main setup function of test */
> -void cleanup(); /* cleanup function for the test */
> -
> -void help()
> -{
> - printf(" -H /.. Location of hugetlbfs, i.e. -H /var/hugetlbfs \n");
> -}
> +static void setup(void);
> +static void cleanup(void);
> +static void help(void);
>
> -int
> -main(int ac, char **av)
> +int main(int ac, char **av)
> {
> - int lc; /* loop counter */
> - char *msg; /* message returned from parse_opts */
> - int Hflag=0; /* binary flag: opt or not */
> - int page_sz=0;
> + int lc;
> + char *msg;
> + int Hflag = 0;
> + int page_sz = 0;
>
> - option_t options[] = {
> - { "H:", &Hflag, &Hopt }, /* Required for location of hugetlbfs */
> - { NULL, NULL, NULL } /* NULL required to end array */
> - };
> + option_t options[] = {
> + { "H:", &Hflag, &Hopt },
> + { NULL, NULL, NULL }
> + };
>
> - /* Parse standard options given to run the test. */
> msg = parse_opts(ac, av, options, &help);
> - if (msg != (char *) NULL) {
> - tst_brkm(TBROK, NULL, "OPTION PARSING ERROR - %s, use -help", msg);
> - tst_exit();
> - }
> + if (msg != NULL)
> + tst_brkm(TBROK, NULL, "OPTION PARSING ERROR - %s,"
> + "use -help", msg);
>
> - if (Hflag == 0) {
> - tst_brkm(TBROK, NULL, "-H option is REQUIRED for this test, use -h for options help");
> - tst_exit();
> - }
> + if (!Hflag)
> + tst_brkm(TBROK, NULL, "-H option is REQUIRED for this test, "
> + "use -h for options help");
>
> setup();
>
> for (lc = 0; TEST_LOOPING(lc); lc++) {
>
> - /* Creat a temporary file used for mapping */
> - if ((fildes = open(TEMPFILE, O_RDWR | O_CREAT, 0666)) < 0) {
> - tst_brkm(TFAIL, cleanup,
> - "open() on %s Failed, errno=%d : %s",
> - TEMPFILE, errno, strerror(errno));
> - }
> + /* Creat a temporary file used for mapping */
> + fildes = open(TEMPFILE, O_RDWR | O_CREAT, 0666);
> + if (fildes < 0)
> + tst_brkm(TFAIL|TERRNO, cleanup, "open %s failed",
> + TEMPFILE);
>
> - Tst_count=0;
> + Tst_count = 0;
>
> /* Note the number of free huge pages BEFORE testing */
> beforetest = get_no_of_free_hugepages();
> @@ -139,11 +118,8 @@ main(int ac, char **av)
> /* Note the size of huge page size BEFORE testing */
> page_sz = hugepages_size();
>
> - /*
> - * Call mmap
> - */
> errno = 0;
> - addr = mmap(NULL, page_sz*1024, PROT_READ | PROT_WRITE,
> + addr = mmap(NULL, page_sz*1024, PROT_READ | PROT_WRITE,
> MAP_SHARED, fildes, 0);
> TEST_ERRNO = errno;
errno = 0 and TEST_ERRNO = errno should be useless here.
Also I saw the original code:
150 /* Check for the return value of mmap() */
151 if (addr == MAP_FAILED) {
152 tst_resm(TFAIL, "mmap() Failed on %s, errno=%d : %s",
153 TEMPFILE, errno, strerror(errno));
154 continue;
the tst_resm line should be updated by using TFAIL|TERRNO as well.
>
> @@ -155,19 +131,24 @@ main(int ac, char **av)
> } else {
> tst_resm(TPASS, "call succeeded");
> /* force to allocate page and change HugePages_Free */
> - *(int*)addr = 0;
> + *(int *)addr = 0;
> }
>
> - /* Make sure the number of free huge pages AFTER testing decreased */
> + /*
> + * Make sure the number of free huge pages
> + * AFTER testing decreased
> + */
> aftertest = get_no_of_free_hugepages();
> hugepagesmapped = beforetest - aftertest;
> if (hugepagesmapped < 1) {
> - tst_resm(TWARN,"Number of HUGEPAGES_FREE stayed the same. Okay if");
> - tst_resm(TWARN,"multiple copies running due to test collision.");
> + tst_resm(TWARN, "Number of HUGEPAGES_FREE stayed the"
> + " same. Okay if");
> + tst_resm(TWARN, "multiple copies running due to test"
> + " collision.");
This looks a little complicated. one tst_resm and mulitple quoted lines
are enough.
> }
> /* Clean up things in case we are looping */
> /* Unmap the mapped memory */
> - if (munmap(addr, page_sz*1024) != 0) {
> + if (munmap(addr, page_sz*1024) != 0) {
> tst_brkm(TFAIL, NULL, "munmap() fails to unmap the "
> "memory, errno=%d", errno);
> }
brackets are unnecessary here. Also you'd better use
`tst_brkm(TFAIL|TERRNO, NULL, "munmap")` here.
> @@ -181,21 +162,20 @@ main(int ac, char **av)
> /*
> * setup() - performs all ONE TIME setup for this test.
> *
> - * Get system page size, allocate and initialize the string dummy.
> - * Initialize addr such that it is more than one page below the break
> - * address of the process, and initialize one page region from addr
> - * with char 'A'.
> - * Creat a temporary directory and a file under it.
> - * Write some known data into file and get the size of the file.
> + * Get system page size, allocate and initialize the string dummy.
> + * Initialize addr such that it is more than one page below the break
> + * address of the process, and initialize one page region from addr
> + * with char 'A'.
> + * Creat a temporary directory and a file under it.
> + * Write some known data into file and get the size of the file.
> */
> -void
> -setup()
> +static void setup(void)
> {
> char mypid[40];
>
> - sprintf(mypid,"/%d",getpid());
> - TEMPFILE=strcat(mypid,TEMPFILE);
> - TEMPFILE=strcat(Hopt,TEMPFILE);
> + sprintf(mypid, "/%d", getpid());
> + TEMPFILE = strcat(mypid, TEMPFILE);
> + TEMPFILE = strcat(Hopt, TEMPFILE);
>
> tst_sig(FORK, DEF_HANDLER, cleanup);
>
> @@ -205,11 +185,10 @@ setup()
>
> /*
> * cleanup() - performs all ONE TIME cleanup for this test at
> - * completion or premature exit.
> - * Remove the temporary directory created.
> + * completion or premature exit.
> + * Remove the temporary directory created.
> */
> -void
> -cleanup()
> +static void cleanup(void)
> {
> /*
> * print timing stats if that option was specified.
> @@ -219,3 +198,8 @@ cleanup()
> unlink(TEMPFILE);
>
> }
> +
> +static void help(void)
> +{
> + printf(" -H /.. Location of hugetlbfs, i.e. -H /var/hugetlbfs\n");
> +}
|