Re: [PATCH v2 1/2] xen/cppcheck: add a way to exclude files from the scan

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

>> +Exclude file list for xen-analysis script
>> +=========================================
>> +
>> +The code analysis is performed on the Xen codebase for both MISRA checkers 
>> and
>> +static analysis checkers, there are some files however that needs to be 
>> removed
>> +from the findings report because they are not owned by Xen and they must be 
>> kept
>> +in sync with their origin (completely or even partially), hence we can't 
>> easily
>> +fix findings or deviate from them.
> 
> I would stay more generic and say something like:
> 
> The code analysis is performed on the Xen codebase for both MISRA
> checkers and static analysis checkers, there are some files however that
> needs to be removed from the findings report for various reasons (e.g.
> they are imported from external sources, they generate too many false
> positive results, etc.).
> 
> But what you wrote is also OK.

I’m ok with that too, I can update with your wordings
>> 
>> +++ b/xen/scripts/xen_analysis/exclusion_file_list.py
>> @@ -0,0 +1,79 @@
>> +#!/usr/bin/env python3
>> +
>> +import os, glob, json
>> +from . import settings
>> +
>> +class ExclusionFileListError(Exception):
>> +    pass
>> +
>> +
>> +def __cppcheck_path_exclude_syntax(path):
>> +    # Prepending * to the relative path to match every path where the Xen
>> +    # codebase could be
>> +    path = "*" + path
>> +
>> +    # Check if the path is to a folder without the wildcard at the end
>> +    if not (path.endswith(".c") or path.endswith(".h") or 
>> path.endswith("*")):
> 
> Isn't there a python call to check that it is actually a folder? I think
> that would be more resilient because otherwise if someone passes a .S or
> .cpp file it would be detected as directory.
> 
> 
>> +        # The path is to a folder, if it doesn't have the final /, add it
>> +        if not path.endswith("/"):
>> +            path = path + "/"
>> +        # Since the path is a folder, add a wildcard to the end so that
>> +        # cppcheck will remove every issue related with this path
>> +        path = path + "*"
>> +
>> +    return path

Yes you are very right, here I wanted to accept the relative path to a folder 
with
or without the ending '/*’, but it carries on much more complexity because here 
the
relative path can contain wildcards in it, so we can’t use os.path.isdir() 
which would
fail.

At cost of being more strict on how folders shall be declared, I think it’s 
better to
enforce the ‘/*’ at the end of a path that is excluding a folder.

We have a previous check using glob() to ensure path with wildcards are real 
path
so we are safe that the passed relative path are OK.

Dropping the requirement of passing folder paths with or without the ‘/*’ 
simplifies
the code and this would be the final result:


diff --git a/docs/misra/exclude-list.rst b/docs/misra/exclude-list.rst
index 969539c46beb..c97431a86120 100644
--- a/docs/misra/exclude-list.rst
+++ b/docs/misra/exclude-list.rst
@@ -3,11 +3,11 @@
 Exclude file list for xen-analysis script
 =========================================
 
-The code analysis is performed on the Xen codebase for both MISRA checkers and
-static analysis checkers, there are some files however that needs to be removed
-from the findings report because they are not owned by Xen and they must be 
kept
-in sync with their origin (completely or even partially), hence we can't easily
-fix findings or deviate from them.
+The code analysis is performed on the Xen codebase for both MISRA
+checkers and static analysis checkers, there are some files however that
+needs to be removed from the findings report for various reasons (e.g.
+they are imported from external sources, they generate too many false
+positive results, etc.).
 
 For this reason the file docs/misra/exclude-list.json is used to exclude every
 entry listed in that file from the final report.
@@ -42,3 +42,5 @@ Here is an explanation of the fields inside an object of the 
"content" array:
 
 To ease the review and the modifications of the entries, they shall be listed 
in
 alphabetical order referring to the rel_path field.
+Excluded folder paths shall end with '/*' in order to match everything on that
+folder.
diff --git a/xen/scripts/xen_analysis/exclusion_file_list.py 
b/xen/scripts/xen_analysis/exclusion_file_list.py
index 4a47a90f5944..871e480586bb 100644
--- a/xen/scripts/xen_analysis/exclusion_file_list.py
+++ b/xen/scripts/xen_analysis/exclusion_file_list.py
@@ -12,15 +12,6 @@ def __cppcheck_path_exclude_syntax(path):
     # codebase could be
     path = "*" + path
 
-    # Check if the path is to a folder without the wildcard at the end
-    if not (path.endswith(".c") or path.endswith(".h") or path.endswith("*")):
-        # The path is to a folder, if it doesn't have the final /, add it
-        if not path.endswith("/"):
-            path = path + "/"
-        # Since the path is a folder, add a wildcard to the end so that
-        # cppcheck will remove every issue related with this path
-        path = path + "*"
-
     return path
 
 
I’ve included also your wording and I’ve specified in the docs how to exclude a 
folder.

Do you think it’s ok to proceed in this way?

Cheers,
Luca