Re: [PATCH 1/6] cyclictest: Centralize clock option parsing

On Tue, Apr 07, 2026 at 10:27:39AM +0200, Florian Bezdeka wrote:
> On Tue, 2026-04-07 at 08:48 +0200, Thomas Weißschuh (Schneider Electric)
> wrote:
> > Currently the lookup of the clockid from the commandline options is done
> > in multiple places. This makes the addition of symbolic clock names and
> > new clocks cumbersome.
> > 
> > Move all of the clock option parsing into a new helper function.
> > 
> > As a side-effect fix an off-by-one error in the validation of the
> > clock command line argument.
> > 
> > The new variable can not be named 'clock' as that would conflict with
> > the libc function of the same name.
> > 
> > Signed-off-by: Thomas Weißschuh (Schneider Electric) <[email protected]>
> > ---
> >  src/cyclictest/cyclictest.c | 35 ++++++++++++++++++-----------------
> >  1 file changed, 18 insertions(+), 17 deletions(-)
> > 
> > diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> > index 960c90560668..592cf9d9387d 100644
> > --- a/src/cyclictest/cyclictest.c
> > +++ b/src/cyclictest/cyclictest.c
> > @@ -1022,7 +1022,7 @@ static int priority;
> >  static int policy = SCHED_OTHER;	/* default policy if not specified */
> >  static int num_threads = 1;
> >  static int max_cycles;
> > -static int clocksel = 0;
> > +static clockid_t used_clock;
> >  static int quiet;
> >  static int interval = DEFAULT_INTERVAL;
> >  static int distance = -1;
> > @@ -1031,10 +1031,17 @@ static struct bitmask *main_affinity_mask = NULL;
> >  static int smp = 0;
> >  static int setaffinity = AFFINITY_UNSPECIFIED;
> >  
> > -static int clocksources[] = {
> > -	CLOCK_MONOTONIC,
> > -	CLOCK_REALTIME,
> > -};
> > +static int handleclock(const char *clockarg)
> > +{
> > +	if (strcmp(clockarg, "0") == 0)
> > +		used_clock = CLOCK_MONOTONIC;
> > +	else if (strcmp(clockarg, "1") == 0)
> > +		used_clock = CLOCK_REALTIME;
> > +	else
> > +		return 1;
> > +
> > +	return 0;
> > +}
> 
> What happens to "unexpected" cmdline parameters now? used_clocks will
> not be set and ...

Then handleclock() will return 1 ...

> >  
> >  static void handlepolicy(char *polname)
> >  {
> > @@ -1195,7 +1202,7 @@ static void process_options(int argc, char *argv[], int max_cpus)
> >  			tracelimit = atoi(optarg); break;
> >  		case 'c':
> >  		case OPT_CLOCK:
> > -			clocksel = atoi(optarg); break;
> > +			error |= handleclock(optarg); break;

... which will set the error flag here.

> >  		case OPT_DEFAULT_SYSTEM:
> >  			power_management = 1; break;
> >  		case 'd':
> > @@ -1377,9 +1384,6 @@ static void process_options(int argc, char *argv[], int max_cpus)
> >  			      "on this processor\n");
> >  	}
> >  
> > -	if (clocksel < 0 || clocksel > ARRAY_SIZE(clocksources))
> > -		error = 1;
> > -
> 
> ... the validation check has been removed. As unused_clock is static
> (and with that initialized to zero) all invalid parameter values will
> silently be "migrated" to CLOCK_REALTIME. Right?

I don't think so, see above.

> I would vote for an error/warn message to avoid that a fat fingered
> cmdline argument invalidates your measurements.

With this patch in its current form, the usage guide will be printed
in case of invalid clock ids. I also just noticed that the behavior
*before* this patch is to silently treat non-numeric arguments as
CLOCK_REALTIME.

> >  	if (oscope_reduction < 1)
> >  		error = 1;

(...)