Re: [PATCH] rteval: Avoid passing None cpulist to measurement

[John Kacur <jkacur@redhat.com>]

On Thu, 6 Jun 2024, tglozar@redhat.com wrote:

> From: Tomas Glozar <tglozar@redhat.com>
> 
> When rteval is called via the command line, cpulists for both
> measurements and loads default to an empty string and are further
> processed by parse_cpulist_from_config. However, this is not true when
> rteval is used as a module: in that case, neither the default
> command-line value is used nor parse_cpulist_from_config is run, leading
> to None being set to the config property of measurements which is
> explicitely passed down to the corresponding cyclictest config property.
> 
> The issue was introduced in 64ce7848dfab ("rteval: Add relative cpulists
> for measurements"), where the check for None was removed from
> the cyclictest module, assuming the value was processed by
> parse_cpulist_from_config.
> 
> Avoid this by calling parse_cpulist_from_config with an empty string to
> get the default cpulist to pass to cyclictest module if cpulist is empty
> in the measurement config.

In this context "avoid" is a weasel word. I "avoid" a sunburn by staying 
out of the sun.

> 
> Note: this is a workaround, in the future, parse_cpulist_from_config
> will be called properly in one place only instead of being scattered
> throughout the code of rteval. That will also enable relative cpulists
> to be used when calling rteval as a module.

I don't know if it is a workaround, this is what the code did before it 
was changed in 64ce7848dfab, leave out any predictions of how we'll handle 
this in the futre.

> 
> Fixes: 64ce7848dfab ("rteval: Add relative cpulists for measurements")
> Signed-off-by: Tomas Glozar <tglozar@redhat.com>
> ---
>  rteval/modules/measurement/__init__.py | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/rteval/modules/measurement/__init__.py b/rteval/modules/measurement/__init__.py
> index 11bd7b0..fda618d 100644
> --- a/rteval/modules/measurement/__init__.py
> +++ b/rteval/modules/measurement/__init__.py
> @@ -148,6 +148,13 @@ measurement profiles, based on their characteristics"""
>          modcfg = self.__cfg.GetSection("measurement")
>          cpulist = modcfg.cpulist
>          run_on_isolcpus = modcfg.run_on_isolcpus
> +        if cpulist is None:
> +            # Avoid passing None cpulist down to measurement module which might
> +            # not expect it
> +            # Note: None can appear only when using rteval as module, not when
> +            # being called from command line; in the latter, cpulists are all
> +            # processed by parse_cpulist_from_config already by this point

Five lines of comment for one line of code? How about
# Initialize the cpulist if it wasn't already set from the command line
> +            cpulist = cpulist_utils.collapse_cpulist(parse_cpulist_from_config("", run_on_isolcpus))
>  
>          for (modname, modtype) in modcfg:
>              if isinstance(modtype, str) and modtype.lower() == 'module':  # Only 'module' will be supported (ds)
> -- 
> 2.45.0

John