[PATCH 0/3] Fix and improvements to xen-analysis.py - Pt.2

[PATCH 0/3] Fix and improvements to xen-analysis.py - Pt.2

[Luca Fancellu]

This serie includes one improvement suggested by Andrew Cooper, and two bug fix
for the xen-analysis.py tool.

Luca Fancellu (3):
  xen/misra: xen-analysis.py: Improve the cppcheck version check
  xen/misra: xen-analysis.py: Fix latent bug
  xen/misra: xen-analysis.py: Fix cppcheck report relative paths

 xen/scripts/xen_analysis/cppcheck_analysis.py | 13 +++------
 .../xen_analysis/cppcheck_report_utils.py     | 27 +++++++++++++++----
 2 files changed, 26 insertions(+), 14 deletions(-)

-- 
2.34.1

[PATCH 1/3] xen/misra: xen-analysis.py: Improve the cppcheck version check

[Luca Fancellu]

Use tuple comparison to check the cppcheck version.

Take the occasion to harden the regex, escaping the dots so that we
check for them instead of generic characters.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
 xen/scripts/xen_analysis/cppcheck_analysis.py | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/xen/scripts/xen_analysis/cppcheck_analysis.py b/xen/scripts/xen_analysis/cppcheck_analysis.py
index c8abbe0fca79..8dc45e653b79 100644
--- a/xen/scripts/xen_analysis/cppcheck_analysis.py
+++ b/xen/scripts/xen_analysis/cppcheck_analysis.py
@@ -157,7 +157,7 @@ def generate_cppcheck_deps():
             "Error occured retrieving cppcheck version:\n{}\n\n{}"
         )
 
-    version_regex = re.search('^Cppcheck (\d+).(\d+)(?:.\d+)?$',
+    version_regex = re.search('^Cppcheck (\d+)\.(\d+)(?:\.\d+)?$',
                               invoke_cppcheck, flags=re.M)
     # Currently, only cppcheck version >= 2.7 is supported, but version 2.8 is
     # known to be broken, please refer to docs/misra/cppcheck.txt
@@ -166,15 +166,10 @@ def generate_cppcheck_deps():
             "Can't find cppcheck version or version not identified: "
             "{}".format(invoke_cppcheck)
         )
-    major = int(version_regex.group(1))
-    minor = int(version_regex.group(2))
-    if major < 2 or (major == 2 and minor < 7):
+    version = (int(version_regex.group(1)), int(version_regex.group(2)))
+    if version < (2, 7) or version == (2, 8):
         raise CppcheckDepsPhaseError(
-            "Cppcheck version < 2.7 is not supported"
-        )
-    if major == 2 and minor == 8:
-        raise CppcheckDepsPhaseError(
-            "Cppcheck version 2.8 is known to be broken, see the documentation"
+            "Cppcheck version < 2.7 or 2.8 are not supported"
         )
 
     # If misra option is selected, append misra addon and generate cppcheck
-- 
2.34.1

[PATCH 2/3] xen/misra: xen-analysis.py: Fix latent bug

[Luca Fancellu]

Currenly there is a latent bug that is not triggered because
the function cppcheck_merge_txt_fragments is called with the
parameter strip_paths having a list of only one element.

The bug is that the split function should not be in the
loop for strip_paths, but one level before, fix it.

Fixes: 02b26c02c7c4 ("xen/scripts: add cppcheck tool to the xen-analysis.py script")
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
 xen/scripts/xen_analysis/cppcheck_report_utils.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/scripts/xen_analysis/cppcheck_report_utils.py b/xen/scripts/xen_analysis/cppcheck_report_utils.py
index c5f466aff141..fdc299c7e029 100644
--- a/xen/scripts/xen_analysis/cppcheck_report_utils.py
+++ b/xen/scripts/xen_analysis/cppcheck_report_utils.py
@@ -104,8 +104,8 @@ def cppcheck_merge_txt_fragments(fragments_list, out_txt_file, strip_paths):
                 for path in strip_paths:
                     text_report_content[i] = text_report_content[i].replace(
                                                                 path + "/", "")
-                    # Split by : separator
-                    text_report_content[i] = text_report_content[i].split(":")
+                # Split by : separator
+                text_report_content[i] = text_report_content[i].split(":")
 
             # sort alphabetically for second field (misra rule) and as second
             # criteria for the first field (file name)
-- 
2.34.1

[PATCH 3/3] xen/misra: xen-analysis.py: Fix cppcheck report relative paths

[Luca Fancellu]

Fix the generation of the relative path from the repo, for cppcheck
reports, when the script is launching make with in-tree build.

Fixes: b046f7e37489 ("xen/misra: xen-analysis.py: use the relative path from the ...")
Reported-by: Michal Orzel <michal.orzel@amd.com>
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
 .../xen_analysis/cppcheck_report_utils.py     | 25 ++++++++++++++++---
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/xen/scripts/xen_analysis/cppcheck_report_utils.py b/xen/scripts/xen_analysis/cppcheck_report_utils.py
index fdc299c7e029..10100f6c6a57 100644
--- a/xen/scripts/xen_analysis/cppcheck_report_utils.py
+++ b/xen/scripts/xen_analysis/cppcheck_report_utils.py
@@ -1,6 +1,7 @@
 #!/usr/bin/env python3
 
-import os
+import os, re
+from . import settings
 from xml.etree import ElementTree
 
 class CppcheckHTMLReportError(Exception):
@@ -101,12 +102,28 @@ def cppcheck_merge_txt_fragments(fragments_list, out_txt_file, strip_paths):
             text_report_content = list(text_report_content)
             # Strip path from report lines
             for i in list(range(0, len(text_report_content))):
-                for path in strip_paths:
-                    text_report_content[i] = text_report_content[i].replace(
-                                                                path + "/", "")
                 # Split by : separator
                 text_report_content[i] = text_report_content[i].split(":")
 
+                for path in strip_paths:
+                    text_report_content[i][0] = \
+                        text_report_content[i][0].replace(path + "/", "")
+
+                # When the compilation is in-tree, the makefile places
+                # the directory in /xen/xen, making cppcheck produce
+                # relative path from there, so check if "xen/" is a prefix
+                # of the path and if it's not, check if it can be added to
+                # have a relative path from the repository instead of from
+                # /xen/xen
+                if not text_report_content[i][0].startswith("xen/"):
+                    # cppcheck first entry is in this format:
+                    # path/to/file(line,cols), remove (line,cols)
+                    cppcheck_file = re.sub(r'\(.*\)', '',
+                                           text_report_content[i][0])
+                    if os.path.isfile(settings.xen_dir + "/" + cppcheck_file):
+                        text_report_content[i][0] = \
+                            "xen/" + text_report_content[i][0]
+
             # sort alphabetically for second field (misra rule) and as second
             # criteria for the first field (file name)
             text_report_content.sort(key = lambda x: (x[1], x[0]))
-- 
2.34.1

Re: [PATCH 1/3] xen/misra: xen-analysis.py: Improve the cppcheck version check

[Stefano Stabellini]

On Fri, 19 May 2023, Luca Fancellu wrote:
> Use tuple comparison to check the cppcheck version.
> 
> Take the occasion to harden the regex, escaping the dots so that we
> check for them instead of generic characters.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/scripts/xen_analysis/cppcheck_analysis.py | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/scripts/xen_analysis/cppcheck_analysis.py b/xen/scripts/xen_analysis/cppcheck_analysis.py
> index c8abbe0fca79..8dc45e653b79 100644
> --- a/xen/scripts/xen_analysis/cppcheck_analysis.py
> +++ b/xen/scripts/xen_analysis/cppcheck_analysis.py
> @@ -157,7 +157,7 @@ def generate_cppcheck_deps():
>              "Error occured retrieving cppcheck version:\n{}\n\n{}"
>          )
>  
> -    version_regex = re.search('^Cppcheck (\d+).(\d+)(?:.\d+)?$',
> +    version_regex = re.search('^Cppcheck (\d+)\.(\d+)(?:\.\d+)?$',
>                                invoke_cppcheck, flags=re.M)
>      # Currently, only cppcheck version >= 2.7 is supported, but version 2.8 is
>      # known to be broken, please refer to docs/misra/cppcheck.txt
> @@ -166,15 +166,10 @@ def generate_cppcheck_deps():
>              "Can't find cppcheck version or version not identified: "
>              "{}".format(invoke_cppcheck)
>          )
> -    major = int(version_regex.group(1))
> -    minor = int(version_regex.group(2))
> -    if major < 2 or (major == 2 and minor < 7):
> +    version = (int(version_regex.group(1)), int(version_regex.group(2)))
> +    if version < (2, 7) or version == (2, 8):
>          raise CppcheckDepsPhaseError(
> -            "Cppcheck version < 2.7 is not supported"
> -        )
> -    if major == 2 and minor == 8:
> -        raise CppcheckDepsPhaseError(
> -            "Cppcheck version 2.8 is known to be broken, see the documentation"
> +            "Cppcheck version < 2.7 or 2.8 are not supported"
>          )
>  
>      # If misra option is selected, append misra addon and generate cppcheck
> -- 
> 2.34.1
> 

Re: [PATCH 3/3] xen/misra: xen-analysis.py: Fix cppcheck report relative paths

[Stefano Stabellini]

On Fri, 19 May 2023, Luca Fancellu wrote:
> Fix the generation of the relative path from the repo, for cppcheck
> reports, when the script is launching make with in-tree build.
> 
> Fixes: b046f7e37489 ("xen/misra: xen-analysis.py: use the relative path from the ...")
> Reported-by: Michal Orzel <michal.orzel@amd.com>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
>  .../xen_analysis/cppcheck_report_utils.py     | 25 ++++++++++++++++---
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/scripts/xen_analysis/cppcheck_report_utils.py b/xen/scripts/xen_analysis/cppcheck_report_utils.py
> index fdc299c7e029..10100f6c6a57 100644
> --- a/xen/scripts/xen_analysis/cppcheck_report_utils.py
> +++ b/xen/scripts/xen_analysis/cppcheck_report_utils.py
> @@ -1,6 +1,7 @@
>  #!/usr/bin/env python3
>  
> -import os
> +import os, re
> +from . import settings
>  from xml.etree import ElementTree
>  
>  class CppcheckHTMLReportError(Exception):
> @@ -101,12 +102,28 @@ def cppcheck_merge_txt_fragments(fragments_list, out_txt_file, strip_paths):
>              text_report_content = list(text_report_content)
>              # Strip path from report lines
>              for i in list(range(0, len(text_report_content))):
> -                for path in strip_paths:
> -                    text_report_content[i] = text_report_content[i].replace(
> -                                                                path + "/", "")
>                  # Split by : separator
>                  text_report_content[i] = text_report_content[i].split(":")
>  
> +                for path in strip_paths:
> +                    text_report_content[i][0] = \
> +                        text_report_content[i][0].replace(path + "/", "")

Why moving this for loop after "Split by : separator"? If it is
necessary, would it make sense to do it in the previous patch?


> +                # When the compilation is in-tree, the makefile places
> +                # the directory in /xen/xen, making cppcheck produce
> +                # relative path from there, so check if "xen/" is a prefix
> +                # of the path and if it's not, check if it can be added to
> +                # have a relative path from the repository instead of from
> +                # /xen/xen
> +                if not text_report_content[i][0].startswith("xen/"):
> +                    # cppcheck first entry is in this format:
> +                    # path/to/file(line,cols), remove (line,cols)
> +                    cppcheck_file = re.sub(r'\(.*\)', '',
> +                                           text_report_content[i][0])
> +                    if os.path.isfile(settings.xen_dir + "/" + cppcheck_file):
> +                        text_report_content[i][0] = \
> +                            "xen/" + text_report_content[i][0]
> +
>              # sort alphabetically for second field (misra rule) and as second
>              # criteria for the first field (file name)
>              text_report_content.sort(key = lambda x: (x[1], x[0]))
> -- 
> 2.34.1
> 

Re: [PATCH 3/3] xen/misra: xen-analysis.py: Fix cppcheck report relative paths

[Luca Fancellu]

> On 25 May 2023, at 01:46, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Fri, 19 May 2023, Luca Fancellu wrote:
>> Fix the generation of the relative path from the repo, for cppcheck
>> reports, when the script is launching make with in-tree build.
>> 
>> Fixes: b046f7e37489 ("xen/misra: xen-analysis.py: use the relative path from the ...")
>> Reported-by: Michal Orzel <michal.orzel@amd.com>
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> ---
>> .../xen_analysis/cppcheck_report_utils.py     | 25 ++++++++++++++++---
>> 1 file changed, 21 insertions(+), 4 deletions(-)
>> 
>> diff --git a/xen/scripts/xen_analysis/cppcheck_report_utils.py b/xen/scripts/xen_analysis/cppcheck_report_utils.py
>> index fdc299c7e029..10100f6c6a57 100644
>> --- a/xen/scripts/xen_analysis/cppcheck_report_utils.py
>> +++ b/xen/scripts/xen_analysis/cppcheck_report_utils.py
>> @@ -1,6 +1,7 @@
>> #!/usr/bin/env python3
>> 
>> -import os
>> +import os, re
>> +from . import settings
>> from xml.etree import ElementTree
>> 
>> class CppcheckHTMLReportError(Exception):
>> @@ -101,12 +102,28 @@ def cppcheck_merge_txt_fragments(fragments_list, out_txt_file, strip_paths):
>>             text_report_content = list(text_report_content)
>>             # Strip path from report lines
>>             for i in list(range(0, len(text_report_content))):
>> -                for path in strip_paths:
>> -                    text_report_content[i] = text_report_content[i].replace(
>> -                                                                path + "/", "")
>>                 # Split by : separator
>>                 text_report_content[i] = text_report_content[i].split(":")
>> 
>> +                for path in strip_paths:
>> +                    text_report_content[i][0] = \
>> +                        text_report_content[i][0].replace(path + "/", "")
> 
> Why moving this for loop after "Split by : separator"? If it is
> necessary, would it make sense to do it in the previous patch?

Hi Stefano,

In the patch before I was fixing the bug, so I thought it was better to don’t introduce other changes,
here I moved the loop after the split because in this way we take into account only the first element
of the ‘:’ separated string, that is the path + “(line,col)”.

Before this path it was ok to do the replace on the full string because we were going just to remove the
path, now instead we use that path to check if it actually exists.

> 
> 
>> +                # When the compilation is in-tree, the makefile places
>> +                # the directory in /xen/xen, making cppcheck produce
>> +                # relative path from there, so check if "xen/" is a prefix
>> +                # of the path and if it's not, check if it can be added to
>> +                # have a relative path from the repository instead of from
>> +                # /xen/xen
>> +                if not text_report_content[i][0].startswith("xen/"):
>> +                    # cppcheck first entry is in this format:
>> +                    # path/to/file(line,cols), remove (line,cols)
>> +                    cppcheck_file = re.sub(r'\(.*\)', '',
>> +                                           text_report_content[i][0])
>> +                    if os.path.isfile(settings.xen_dir + "/" + cppcheck_file):
>> +                        text_report_content[i][0] = \
>> +                            "xen/" + text_report_content[i][0]
>> +
>>             # sort alphabetically for second field (misra rule) and as second
>>             # criteria for the first field (file name)
>>             text_report_content.sort(key = lambda x: (x[1], x[0]))
>> -- 
>> 2.34.1

Re: [PATCH 2/3] xen/misra: xen-analysis.py: Fix latent bug

[Stefano Stabellini]

On Fri, 19 May 2023, Luca Fancellu wrote:
> Currenly there is a latent bug that is not triggered because
> the function cppcheck_merge_txt_fragments is called with the
> parameter strip_paths having a list of only one element.
> 
> The bug is that the split function should not be in the
> loop for strip_paths, but one level before, fix it.
> 
> Fixes: 02b26c02c7c4 ("xen/scripts: add cppcheck tool to the xen-analysis.py script")
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/scripts/xen_analysis/cppcheck_report_utils.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/scripts/xen_analysis/cppcheck_report_utils.py b/xen/scripts/xen_analysis/cppcheck_report_utils.py
> index c5f466aff141..fdc299c7e029 100644
> --- a/xen/scripts/xen_analysis/cppcheck_report_utils.py
> +++ b/xen/scripts/xen_analysis/cppcheck_report_utils.py
> @@ -104,8 +104,8 @@ def cppcheck_merge_txt_fragments(fragments_list, out_txt_file, strip_paths):
>                  for path in strip_paths:
>                      text_report_content[i] = text_report_content[i].replace(
>                                                                  path + "/", "")
> -                    # Split by : separator
> -                    text_report_content[i] = text_report_content[i].split(":")
> +                # Split by : separator
> +                text_report_content[i] = text_report_content[i].split(":")
>  
>              # sort alphabetically for second field (misra rule) and as second
>              # criteria for the first field (file name)
> -- 
> 2.34.1
> 

Re: [PATCH 3/3] xen/misra: xen-analysis.py: Fix cppcheck report relative paths

[Stefano Stabellini]

On Fri, 19 May 2023, Luca Fancellu wrote:
> Fix the generation of the relative path from the repo, for cppcheck
> reports, when the script is launching make with in-tree build.
> 
> Fixes: b046f7e37489 ("xen/misra: xen-analysis.py: use the relative path from the ...")
> Reported-by: Michal Orzel <michal.orzel@amd.com>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  .../xen_analysis/cppcheck_report_utils.py     | 25 ++++++++++++++++---
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/scripts/xen_analysis/cppcheck_report_utils.py b/xen/scripts/xen_analysis/cppcheck_report_utils.py
> index fdc299c7e029..10100f6c6a57 100644
> --- a/xen/scripts/xen_analysis/cppcheck_report_utils.py
> +++ b/xen/scripts/xen_analysis/cppcheck_report_utils.py
> @@ -1,6 +1,7 @@
>  #!/usr/bin/env python3
>  
> -import os
> +import os, re
> +from . import settings
>  from xml.etree import ElementTree
>  
>  class CppcheckHTMLReportError(Exception):
> @@ -101,12 +102,28 @@ def cppcheck_merge_txt_fragments(fragments_list, out_txt_file, strip_paths):
>              text_report_content = list(text_report_content)
>              # Strip path from report lines
>              for i in list(range(0, len(text_report_content))):
> -                for path in strip_paths:
> -                    text_report_content[i] = text_report_content[i].replace(
> -                                                                path + "/", "")
>                  # Split by : separator
>                  text_report_content[i] = text_report_content[i].split(":")
>  
> +                for path in strip_paths:
> +                    text_report_content[i][0] = \
> +                        text_report_content[i][0].replace(path + "/", "")
> +
> +                # When the compilation is in-tree, the makefile places
> +                # the directory in /xen/xen, making cppcheck produce
> +                # relative path from there, so check if "xen/" is a prefix
> +                # of the path and if it's not, check if it can be added to
> +                # have a relative path from the repository instead of from
> +                # /xen/xen
> +                if not text_report_content[i][0].startswith("xen/"):
> +                    # cppcheck first entry is in this format:
> +                    # path/to/file(line,cols), remove (line,cols)
> +                    cppcheck_file = re.sub(r'\(.*\)', '',
> +                                           text_report_content[i][0])
> +                    if os.path.isfile(settings.xen_dir + "/" + cppcheck_file):
> +                        text_report_content[i][0] = \
> +                            "xen/" + text_report_content[i][0]
> +
>              # sort alphabetically for second field (misra rule) and as second
>              # criteria for the first field (file name)
>              text_report_content.sort(key = lambda x: (x[1], x[0]))
> -- 
> 2.34.1
> 

Re: [PATCH 2/3] xen/misra: xen-analysis.py: Fix latent bug

[Jan Beulich]

On 19.05.2023 11:30, Luca Fancellu wrote:
> Currenly there is a latent bug that is not triggered because
> the function cppcheck_merge_txt_fragments is called with the
> parameter strip_paths having a list of only one element.
> 
> The bug is that the split function should not be in the
> loop for strip_paths, but one level before, fix it.
> 
> Fixes: 02b26c02c7c4 ("xen/scripts: add cppcheck tool to the xen-analysis.py script")
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>

Just wanted to mention it: I've committed the patch as is, but in
the future could you please try to find slightly more specific a
title for such a change? The way it is now, someone going over
just the titles in the log will have no clue at all what kind of
bug was addressed here.

Jan

Re: [PATCH 2/3] xen/misra: xen-analysis.py: Fix latent bug

[Luca Fancellu]

> On 30 May 2023, at 11:32, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 19.05.2023 11:30, Luca Fancellu wrote:
>> Currenly there is a latent bug that is not triggered because
>> the function cppcheck_merge_txt_fragments is called with the
>> parameter strip_paths having a list of only one element.
>> 
>> The bug is that the split function should not be in the
>> loop for strip_paths, but one level before, fix it.
>> 
>> Fixes: 02b26c02c7c4 ("xen/scripts: add cppcheck tool to the xen-analysis.py script")
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> 
> Just wanted to mention it: I've committed the patch as is, but in
> the future could you please try to find slightly more specific a
> title for such a change? The way it is now, someone going over
> just the titles in the log will have no clue at all what kind of
> bug was addressed here.

Sure, I will be more specific in the future, thank you

> 
> Jan