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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 2 Mar 2023, Luca Fancellu wrote:
> >> +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?

Yes I think that's fine. I like that the python script becomes simpler