[LTP] [PATCH v2 03/13] hugeshmctl01: Convert to LTP synchronisation primitives

[Cyril Hrubis]

Hi!
>  static void stat_setup(void);
>  static void stat_cleanup(void);
>  static void set_setup(void);
> @@ -141,19 +139,10 @@ void *set_shmat(void)
>   */
>  static void stat_setup(void)
>  {
> -	int i, rval;
> +	int i;
>  	void *test;
>  	pid_t pid;
> -	sigset_t newmask, oldmask;
> -	struct sigaction sa;
> -
> -	memset (&sa, '\0', sizeof(sa));
> -	sa.sa_handler = sighandler;
> -	sa.sa_flags = 0;
> -	TEST(sigaction(SIGUSR1, &sa, NULL));
> -	if (TEST_RETURN == -1)
> -		tst_brk(TBROK | TRERRNO,
> -				"SIGSEGV signal setup failed");
> +
>  	/*
>  	 * The first time through, let the children attach the memory.
>  	 * The second time through, attach the memory first and let
> @@ -168,21 +157,6 @@ static void stat_setup(void)
>  		set_shared = set_shmat();
>  	}
>  
> -	/*
> -	 * Block SIGUSR1 before children pause for a signal
> -	 * Doing so to avoid the risk that the parent cleans up
> -	 * children by calling stat_cleanup() before children call
> -	 * call pause() so that children sleep forever(this is a
> -	 * side effect of the arbitrary usleep time below).
> -	 * In FIRST, children call shmat. If children sleep forever,
> -	 * those attached shm can't be released so some other shm
> -	 * tests will fail a lot.
> -	 */
> -	sigemptyset(&newmask);
> -	sigaddset(&newmask, SIGUSR1);
> -	if (sigprocmask(SIG_BLOCK, &newmask, &oldmask) < 0)
> -		tst_brk(TBROK | TERRNO, "block SIGUSR1 error");
> -
>  	for (i = 0; i < N_ATTACH; i++) {
>  		switch (pid = SAFE_FORK()) {
>  		case 0:
> @@ -191,39 +165,24 @@ static void stat_setup(void)
>  			/* do an assignement for fun */
>  			*(int *)test = i;
>  
> -			/*
> -			 * sigsuspend until we get a signal from stat_cleanup()
> -			 * use sigsuspend instead of pause to avoid children
> -			 * infinite sleep without getting SIGUSR1 from parent
> -			 */
> -			rval = sigsuspend(&oldmask);
> -			if (rval != -1)
> -				tst_brk(TBROK | TERRNO, "sigsuspend");
> -
> -			/*
> -			 * don't have to block SIGUSR1 any more,
> -			 * recover the mask
> -			 */
> -			if (sigprocmask(SIG_SETMASK, &oldmask, NULL) < 0)
> -				tst_brk(TBROK | TERRNO,
> -					 "child sigprocmask");
> +			/* If last child then wake the parent */
> +			if (i == N_ATTACH - 1)
> +				TST_CHECKPOINT_WAKE(0);

As far as I can tell the assumption that the children are executed in
any order does not hold at all. So the parent should wait for _each_ of
them separately.

I guess that we have to suspend the parent process with
TST_CHECKPOINT_WAIT() after each fork() and wait till the children wakes
up the parent before we attempt to fork next child.

> +			TST_CHECKPOINT_WAIT(1);
>  
>  			/* now we're back - detach the memory and exit */
>  			if (shmdt(test) == -1)
>  				tst_brk(TBROK | TERRNO,
>  					 "shmdt in stat_setup()");
> +
> +			/* Child process quits here */

Please avoid adding obvious comments like this one. The original code is
not a good example and should be cleaned up.

>  			exit(0);
> -		default:
> -			/* save the child's pid for cleanup later */
> -			pid_arr[i] = pid;
>  		}
>  	}
>  
> -	/* parent doesn't have to block SIGUSR1, recover the mask */
> -	if (sigprocmask(SIG_SETMASK, &oldmask, NULL) < 0)
> -		tst_brk(TBROK, "parent sigprocmask");
> -
> -	usleep(250000);
> +	/* Wait for all the children to attach to shared memory */
> +	TST_CHECKPOINT_WAIT(0);
>  }
>  
>  /*
> @@ -283,11 +242,8 @@ fail:
>   */
>  static void stat_cleanup(void)
>  {
> -	int i;
> -
>  	/* wake up the childern so they can detach the memory and exit */
> -	for (i = 0; i < N_ATTACH; i++)
> -		SAFE_KILL(pid_arr[i], SIGUSR1);
> +	TST_CHECKPOINT_WAKE2(1, N_ATTACH);

We should still SAFE_WAIT() each of the processses here otherwise we
cannot guarantee that the memory was detached and that the child really
exitted.

>  	/* remove the parent's shared memory the second time through */
>  	if (stat_time == SECOND)
> @@ -296,12 +252,6 @@ static void stat_cleanup(void)
>  	stat_time++;
>  }
>  
> -static void sighandler(int sig)
> -{
> -	if (sig != SIGUSR1)
> -		tst_res(TFAIL, "received unexpected signal %d", sig);
> -}
> -
>  /*
>   * set_setup() - set up for the IPC_SET command with shmctl()
>   */
> @@ -387,4 +337,5 @@ static struct tst_test test = {
>  	.setup = setup,
>  	.cleanup = cleanup,
>  	.test_all = test_hugeshmctl,
> +	.needs_checkpoints = 1,
>  };
> -- 
> 2.14.2
> 

-- 
Cyril Hrubis
chrubis@suse.cz