[v3,2/2] semop04:Refactor with new API

Signed-off-by: Wei Gao <wegao@suse.com>
---
 testcases/kernel/syscalls/ipc/semop/semop04.c | 163 ++++++++----------
 1 file changed, 68 insertions(+), 95 deletions(-)

Hi Wei,

...
> -int verbose = 0;
> -int loops = 100;
> -int errors = 0;
> +static char *verbose;

We usually prefer to not introduce verbose option in refactored tests.
It'd be nice to have output, which is not too verbose, but not just TPASS
message (something in between).

Kind regards,
Petr

On Wed, Mar 15, 2023 at 01:36:41PM +0100, Petr Vorel wrote:
> Hi Wei,
> 
> ...
> > -int verbose = 0;
> > -int loops = 100;
> > -int errors = 0;
> > +static char *verbose;
> 
> We usually prefer to not introduce verbose option in refactored tests.
> It'd be nice to have output, which is not too verbose, but not just TPASS
> message (something in between).
I just need double confirm with your idea:
Remove this option and let current verbose info print out by default?
> 
> Kind regards,
> Petr

Hi Wei,

> +// SPDX-License-Identifier: GPL-2.0
Below is "or (at your option) any later version."
=> 
// SPDX-License-Identifier: GPL-2.0-or-later

>  /*
> - *
> - *   Copyright (c) International Business Machines  Corp., 2001
> - *
> - *   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
> - *   the Free Software Foundation; either version 2 of the License, or
> - *   (at your option) any later version.
> - *
> - *   This program 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 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
> + * Copyright (c) International Business Machines  Corp., 2001
> + * Copyright (C) 2023 Linux Test Project, Inc.
Actually LTP started to work on the file since 2003, i.e.
* Copyright (C) 2003-2023 Linux Test Project, Inc.
>   */

> -/*
> - *  FILE        : sem01.c
> - *  DESCRIPTION : Creates a semaphore and two processes.  The processes
> - *                each go through a loop where they semdown, delay for a
> - *                random amount of time, and semup, so they will almost
> - *                always be fighting for control of the semaphore.
> - *  HISTORY:
> - *    01/15/2001 Paul Larson (plars@us.ibm.com)
> - *      -written
> - *    11/09/2001 Manoj Iyer (manjo@ausin.ibm.com)
Probably you should keep these two names (although you can clean formatting a
bit), e.g.:
* Author: 2001 Paul Larson <plars@us.ibm.com>
* Modified: 2001 Manoj Iyer <manjo@ausin.ibm.com>

> - *    Modified.
> - *    - Removed compiler warnings.
> - *      added exit to the end of function main()

...
> -int semup(int semid)
> +static void semup(int semid)
>  {
>  	struct sembuf semops;
> +
>  	semops.sem_num = 0;
> +
nit: I'd remove this blank line (readability)
>  	semops.sem_op = 1;
> +
>  	semops.sem_flg = SEM_UNDO;
> -	if (semop(semid, &semops, 1) == -1) {
> -		perror("semup");
> -		errors++;
> -		return 1;
> -	}
> -	return 0;
> +
> +	SAFE_SEMOP(semid, &semops, 1);
>  }

> -int semdown(int semid)
> +static void semdown(int semid)
>  {
>  	struct sembuf semops;
> +
>  	semops.sem_num = 0;
> +
nit: also here remove blank line.
>  	semops.sem_op = -1;
> +
>  	semops.sem_flg = SEM_UNDO;
> -	if (semop(semid, &semops, 1) == -1) {
> -		perror("semdown");
> -		errors++;
> -		return 1;
> -	}
> -	return 0;
> +
> +	SAFE_SEMOP(semid, &semops, 1);
>  }

> -void delayloop()
> +static void delayloop(void)
>  {
>  	int delay;
> +
>  	delay = 1 + ((100.0 * rand()) / RAND_MAX);
> +
>  	if (verbose)
> -		printf("in delay function for %d microseconds\n", delay);
> +		tst_res(TINFO, "in delay function for %d microseconds", delay);
I'd remove this message.

>  	usleep(delay);
>  }

>  void mainloop(int semid)
>  {
>  	int i;
> +
>  	for (i = 0; i < loops; i++) {
> -		if (semdown(semid)) {
> -			printf("semdown failed\n");
> -		}
> +		semdown(semid);


>  		if (verbose)
> -			printf("sem is down\n");
> +			tst_res(TINFO, "Sem is down");
Actually this message is IMHO useless. semdown() uses SAFE_SEMOP() => we will
see where it failed.

>  		delayloop();
> -		if (semup(semid)) {
> -			printf("semup failed\n");
> -		}
> +		semup(semid);
>  		if (verbose)
> -			printf("sem is up\n");
> +			tst_res(TINFO, "Sem is up");
The same applies there => I'd remove verbose and these messages.
>  	}
>  }

> -int main(int argc, char *argv[])
> +static void run(void)
>  {
> -	int semid, opt;
> +	int semid;
>  	union semun semunion;
> -	extern char *optarg;
>  	pid_t pid;
>  	int chstat;

> -	while ((opt = getopt(argc, argv, "l:vh")) != EOF) {
> -		switch ((char)opt) {
> -		case 'l':
> -			loops = atoi(optarg);
> -			break;
> -		case 'v':
> -			verbose = 1;
> -			break;
> -		case 'h':
> -		default:
> -			printf("Usage: -l loops [-v]\n");
> -			exit(1);
> -		}
> -	}
> -
>  	/* set up the semaphore */
> -	if ((semid = semget((key_t) 9142, 1, 0666 | IPC_CREAT)) < 0) {
> -		printf("error in semget()\n");
> -		exit(-1);
> -	}
> +	semid = SAFE_SEMGET((key_t) 9142, 1, 0666 | IPC_CREAT);
> +	if (semid < 0)
> +		tst_brk(TBROK, "Error in semget id=%i", semid);
All safe messages handles error checking and call tst_brk() on error => this if
check is unreachable (useless) => remove it.
> +
>  	semunion.val = 1;
> -	if (semctl(semid, 0, SETVAL, semunion) == -1) {
> -		printf("error in semctl\n");
> -	}

> -	if ((pid = fork()) < 0) {
> -		printf("fork error\n");
> -		exit(-1);
> -	}
> +	SAFE_SEMCTL(semid, 0, SETVAL, semunion);
> +
> +	pid = SAFE_FORK();
> +	if (pid < 0)
> +		tst_brk(TBROK, "Fork failed pid %i", pid);
Also this if check is useless => remove it.
> +
>  	if (pid) {
>  		/* parent */
>  		srand(pid);
> @@ -147,18 +109,29 @@ int main(int argc, char *argv[])
>  			printf("child exited with status\n");
>  			exit(-1);
>  		}
NOTE: waitpid(), WIFEXITED(), printf() and exit(-1) here should be replaced by tst_reap_children(), see
https://github.com/linux-test-project/ltp/wiki/C-Test-API#18-doing-the-test-in-the-child-process
91e6a95b7 ("clone08: convert to new LTP API")

> +		SAFE_SEMCTL(semid, 0, IPC_RMID, semunion);
> +		tst_res(TPASS, "Semaphore up/down check success");
>  	} else {
>  		/* child */
>  		mainloop(semid);
>  	}
> -	exit(0);
>  }
> +
> +static void setup(void)
> +{
> +	if (opt_loops_str)
> +		loops = SAFE_STRTOL(opt_loops_str, 1, INT_MAX);
> +}
I wonder if we need to specify loops, we have -i which can do more runs.

Kind regards,
Petr