Re: [LTP] [PATCH v1] Rewrite sighold02.c using new LTP API

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
Can you please format the documentation comment so that it renders
nicely in asciidoc?

(just install asciidoc and perl/JSON the make in the top level directory
will render docparse/metadata.html)

>  /* _XOPEN_SOURCE disables NSIG */
>  #ifndef NSIG
> -# define NSIG _NSIG
> +#define NSIG _NSIG
>  #endif
>  
>  /* ensure NUMSIGS is defined */
>  #ifndef NUMSIGS
> -# define NUMSIGS NSIG
> +#define NUMSIGS NSIG
>  #endif
>  
> -char *TCID = "sighold02";
> -int TST_TOTAL = 2;
> -
> -static int pid;
> -static void do_child(void);
> -static void setup(void);
> -static void cleanup(void);
> -
>  static int sigs_catched;
>  static int sigs_map[NUMSIGS];
>  
> @@ -88,57 +50,13 @@ static int skip_sig(int sig)
>  	}
>  }
>  
> -int main(int ac, char **av)
> -{
> -	int sig;
> -	int lc;
> -
> -	tst_parse_opts(ac, av, NULL, NULL);
> -
> -#ifdef UCLINUX
> -	maybe_run_child(&do_child, "");
> -#endif
> -
> -	setup();
> -
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -		if ((pid = FORK_OR_VFORK()) < 0) {
> -			tst_brkm(TBROK | TERRNO, NULL, "fork() failed");
> -		} else if (pid > 0) {
> -			TST_SAFE_CHECKPOINT_WAIT(NULL, 0);
> -
> -			for (sig = 1; sig < NUMSIGS; sig++) {
> -				if (skip_sig(sig))
> -					continue;
> -				SAFE_KILL(NULL, pid, sig);
> -			}
> -
> -			TST_SAFE_CHECKPOINT_WAKE(NULL, 0);
> -			tst_record_childstatus(cleanup, pid);
> -		} else {
> -
> -#ifdef UCLINUX
> -			if (self_exec(av[0], "") < 0) {
> -				tst_brkm(TBROK | TERRNO, NULL,
> -					 "self_exec() failed");
> -			}
> -#else
> -			do_child();
> -#endif
> -		}
> -	}
> -
> -	cleanup();
> -	tst_exit();
> -}
> -
>  static void handle_sigs(int sig)
>  {
>  	sigs_map[sig] = 1;
>  	sigs_catched++;
>  }
>  
> -void do_child(void)
> +static void do_child(void)
>  {
>  	int cnt;
>  	int sig;
> @@ -148,55 +66,60 @@ void do_child(void)
>  		if (skip_sig(sig))
>  			continue;
>  
> -		if (signal(sig, handle_sigs) == SIG_ERR) {
> -			tst_resm(TBROK | TERRNO, "signal() %i(%s) failed",
> -				 sig, tst_strsig(sig));
> -		}
> +		SAFE_SIGNAL(sig, handle_sigs);
>  	}
>  
>  	/* all set up to catch signals, now hold them */

No need to keep comments commeting the obvious like this one.

>  	for (cnt = 0, sig = 1; sig < NUMSIGS; sig++) {
>  		if (skip_sig(sig))
>  			continue;
> +
>  		cnt++;

The cnt is actually unused.

> -		TEST(sighold(sig));
> -		if (TEST_RETURN != 0) {
> -			tst_resm(TBROK | TTERRNO, "sighold() %i(%s) failed",
> -				 sig, tst_strsig(sig));
> -		}
> +
> +		if (sighold(sig))
> +			tst_brk(TBROK, "sighold() %i(%s) failed", sig,
> +				tst_strsig(sig));

Please add | TERRNO to print errno in the case of the failure as well.

Also I woudl have simplified the message just to:

	tst_brk(TBROK | TERRNO, "sighold(%s %i)", tst_strsig(sig), sig);

>  	}
>  
> -	TST_SAFE_CHECKPOINT_WAKE_AND_WAIT(NULL, 0);
> +	TST_CHECKPOINT_WAKE_AND_WAIT(0);
>  
>  	if (!sigs_catched) {
> -		tst_resm(TPASS, "All signals were hold");
> -		tst_exit();
> +		tst_res(TPASS, "all signals were hold");
> +	} else {
> +		tst_res(TFAIL, "signal handler was executed");
> +
> +		for (sig = 1; sig < NUMSIGS; sig++)
> +			if (sigs_map[sig])
> +				tst_res(TINFO, "Signal %i(%s) catched", sig,
> +					tst_strsig(sig));
>  	}


We can get rid of the else and make the code nices by doing return in
the if as:

	if (!sigs_caught) {
		tst_res(TPASS, ...)
		return;
	}

	tst_res(TFAIL, ...);

	for (...)

> +}
>  
> -	tst_resm(TFAIL, "Signal handler was executed");
> +static void run(void)
> +{
> +	pid_t pid_child;
> +	int signal;
>  
> -	for (sig = 1; sig < NUMSIGS; sig++) {
> -		if (sigs_map[sig]) {
> -			tst_resm(TINFO, "Signal %i(%s) catched",
> -			         sig, tst_strsig(sig));
> -		}
> +	pid_child = SAFE_FORK();
> +	if (!pid_child) {
> +		do_child();
> +		return;
>  	}
>  
> -	tst_exit();
> -}
> -
> -static void setup(void)
> -{
> -	tst_sig(FORK, DEF_HANDLER, NULL);
> +	TST_CHECKPOINT_WAIT(0);
>  
> -	tst_tmpdir();
> +	for (signal = 1; signal < NUMSIGS; signal++) {
> +		if (skip_sig(signal))
> +			continue;
>  
> -	TST_CHECKPOINT_INIT(tst_rmdir);
> +		SAFE_KILL(pid_child, signal);
> +	}
>  
> -	TEST_PAUSE;
> +	TST_CHECKPOINT_WAKE(0);
>  }
>  
> -static void cleanup(void)
> -{
> -	tst_rmdir();
> -}
> +static struct tst_test test = {
> +	.test_all = run,
> +	.forks_child = 1,
> +	.needs_checkpoints = 1,
> +};
> -- 
> 2.34.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp