Re: [PATCH 2/4] signaltest: properly test return value from system()

[John Kacur <jkacur@redhat.com>]

On Tue, 6 Oct 2015, Henrik Austad wrote:

> signaltest writes to several /proc/sys/kernel attributes without testing
> if the system() call succeeds or not. We expect the writes from echo to
> suceed (i.e. yield '0' in return), so we test explicitly for that.
> 
> Signed-off-by: Henrik Austad <haustad@cisco.com>
> Cc: Clark Williams <williams@redhat.com>
> Cc: John Kacur <jkacur@redhat.com>
> ---
>  src/signaltest/signaltest.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/src/signaltest/signaltest.c b/src/signaltest/signaltest.c
> index b80969b..8074b68 100644
> --- a/src/signaltest/signaltest.c
> +++ b/src/signaltest/signaltest.c
> @@ -65,6 +65,11 @@ struct thread_stat {
>  	int threadstarted;
>  	int tid;
>  };
> +#define SYSTEM_W(x)				\
> +	if (system((x)) != 0) {			\
> +		fprintf(stderr, "Trouble running %s\n", (x));	\
> +		return NULL;					\
> +	}							\
>  
>  static int shutdown;
>  static int tracelimit = 0;
> @@ -102,16 +107,16 @@ void *signalthread(void *param)
>  	int first = 1;
>  
>  	if (tracelimit) {
> -		system("echo 1 > /proc/sys/kernel/trace_all_cpus");
> -		system("echo 1 > /proc/sys/kernel/trace_enabled");
> -		system("echo 1 > /proc/sys/kernel/trace_freerunning");
> -		system("echo 0 > /proc/sys/kernel/trace_print_at_crash");
> -		system("echo 1 > /proc/sys/kernel/trace_user_triggered");
> -		system("echo -1 > /proc/sys/kernel/trace_user_trigger_irq");
> -		system("echo 0 > /proc/sys/kernel/trace_verbose");
> -		system("echo 0 > /proc/sys/kernel/preempt_thresh");
> -		system("echo 0 > /proc/sys/kernel/wakeup_timing");
> -		system("echo 0 > /proc/sys/kernel/preempt_max_latency");
> +		SYSTEM_W("echo 1 > /proc/sys/kernel/trace_all_cpus");
> +		SYSTEM_W("echo 1 > /proc/sys/kernel/trace_enabled");
> +		SYSTEM_W("echo 1 > /proc/sys/kernel/trace_freerunning");
> +		SYSTEM_W("echo 0 > /proc/sys/kernel/trace_print_at_crash");
> +		SYSTEM_W("echo 1 > /proc/sys/kernel/trace_user_triggered");
> +		SYSTEM_W("echo -1 > /proc/sys/kernel/trace_user_trigger_irq");
> +		SYSTEM_W("echo 0 > /proc/sys/kernel/trace_verbose");
> +		SYSTEM_W("echo 0 > /proc/sys/kernel/preempt_thresh");
> +		SYSTEM_W("echo 0 > /proc/sys/kernel/wakeup_timing");
> +		SYSTEM_W("echo 0 > /proc/sys/kernel/preempt_max_latency");
>  	}
>  
>  	stat->tid = gettid();
> -- 
> 1.9.1
> 

I really don't like patches like this that just exist to shut-up the 
compiler's overzealous warnings. Are you actually having a problem with 
this program that you need to debug? If you are, is the message "Trouble 
running echo num to some /proc" file really useful? Have you carefully 
read the system man page to see what kind of errors can be returned and 
why? I'm going to pass on this one for now.
Sorry
John