Re: [RFC 5/7] rt-tests: cyclictest: Move signal handler to avoid function declaration

[Punit Agrawal <punitagrawal@gmail.com>]

Hi John,

John Kacur <jkacur@redhat.com> writes:

> On Thu, 14 Oct 2021, Punit Agrawal wrote:
>
>> From: Punit Agrawal <punit1.agrawal@toshiba.co.jp>
>> 
>> Signed-off-by: Punit Agrawal <punit1.agrawal@toshiba.co.jp>
>> ---
>>  src/cyclictest/cyclictest.c | 78 ++++++++++++++++++-------------------
>>  1 file changed, 37 insertions(+), 41 deletions(-)
>> 
>> diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
>> index 9c67a3ce3034..48a782fe167d 100644
>> --- a/src/cyclictest/cyclictest.c
>> +++ b/src/cyclictest/cyclictest.c
>> @@ -220,10 +220,6 @@ static char jsonfile[MAX_PATH];
>>  static struct thread_param **parameters;
>>  static struct thread_stat **statistics;
>>  
>> -static void print_stat(FILE *fp, struct thread_param *par, int index, int verbose, int quiet);
>> -static void rstat_print_stat(struct thread_param *par, int index, int verbose, int quiet);
>> -static void rstat_setup(void);
>> -
>>  static int latency_target_fd = -1;
>>  static int32_t latency_target_value = 0;
>>  
>> @@ -1311,43 +1307,6 @@ static int check_timer(void)
>>  	return (ts.tv_sec != 0 || ts.tv_nsec != 1);
>>  }
>>  
>> -static void sighand(int sig)
>> -{
>> -	if (sig == SIGUSR1) {
>> -		int i;
>> -		int oldquiet = quiet;
>> -
>> -		quiet = 0;
>> -		fprintf(stderr, "#---------------------------\n");
>> -		fprintf(stderr, "# cyclictest current status:\n");
>> -		for (i = 0; i < num_threads; i++)
>> -			print_stat(stderr, parameters[i], i, 0, 0);
>> -		fprintf(stderr, "#---------------------------\n");
>> -		quiet = oldquiet;
>> -		return;
>> -	} else if (sig == SIGUSR2) {
>> -		int i;
>> -		int oldquiet = quiet;
>> -
>> -		if (rstat_fd == -1) {
>> -			fprintf(stderr, "ERROR: rstat_fd not valid\n");
>> -			return;
>> -		}
>> -		rstat_ftruncate(rstat_fd, 0);
>> -		quiet = 0;
>> -		dprintf(rstat_fd, "#---------------------------\n");
>> -		dprintf(rstat_fd, "# cyclictest current status:\n");
>> -		for (i = 0; i < num_threads; i++)
>> -			rstat_print_stat(parameters[i], i, 0, 0);
>> -		dprintf(rstat_fd, "#---------------------------\n");
>> -		quiet = oldquiet;
>> -		return;
>> -	}
>> -	shutdown = 1;
>> -	if (refresh_on_max)
>> -		pthread_cond_signal(&refresh_on_max_cond);
>> -}
>> -
>>  static void print_tids(struct thread_param *par[], int nthreads)
>>  {
>>  	int i;
>> @@ -1566,6 +1525,43 @@ static void rstat_print_stat(struct thread_param *par, int index, int verbose, i
>>  }
>>  
>>  
>> +static void sighand(int sig)
>> +{
>> +	if (sig == SIGUSR1) {
>> +		int i;
>> +		int oldquiet = quiet;
>> +
>> +		quiet = 0;
>> +		fprintf(stderr, "#---------------------------\n");
>> +		fprintf(stderr, "# cyclictest current status:\n");
>> +		for (i = 0; i < num_threads; i++)
>> +			print_stat(stderr, parameters[i], i, 0, 0);
>> +		fprintf(stderr, "#---------------------------\n");
>> +		quiet = oldquiet;
>> +		return;
>> +	} else if (sig == SIGUSR2) {
>> +		int i;
>> +		int oldquiet = quiet;
>> +
>> +		if (rstat_fd == -1) {
>> +			fprintf(stderr, "ERROR: rstat_fd not valid\n");
>> +			return;
>> +		}
>> +		rstat_ftruncate(rstat_fd, 0);
>> +		quiet = 0;
>> +		dprintf(rstat_fd, "#---------------------------\n");
>> +		dprintf(rstat_fd, "# cyclictest current status:\n");
>> +		for (i = 0; i < num_threads; i++)
>> +			rstat_print_stat(parameters[i], i, 0, 0);
>> +		dprintf(rstat_fd, "#---------------------------\n");
>> +		quiet = oldquiet;
>> +		return;
>> +	}
>> +	shutdown = 1;
>> +	if (refresh_on_max)
>> +		pthread_cond_signal(&refresh_on_max_cond);
>> +}
>> +
>>  /*
>>   * thread that creates a named fifo and hands out run stats when someone
>>   * reads from the fifo.
>> -- 
>> 2.32.0
>> 
>> 
>
> NACK: What's wrong with function declarations? They allow the freedeom to 
> put your function where you wish. This patch is unnecessary churn, sorry.

I wasn't implying there's anything wrong with a function
declaration. They seemed unnecessary and can be easily avoided. But if
there's a reason to prefer the current placement of the signal handler
(I couldn't see any obvious pattern), I'll drop the patch.

Thanks for comments on the other patches as well. I'll update the set
once you've had a chance to go through the rest.

Punit