[PATCH] rteval: Avoid passing None cpulist to measurement

[PATCH] rteval: Avoid passing None cpulist to measurement

[tglozar]

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.

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.

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
+            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

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

[John Kacur]

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