[Buildroot] [PATCH v3 5/6] support/scripts/pkgstat: add target CPE reporting

[Ricardo Martincoski]

Hello,

This is not a complete code-review, but it seems you will send a new iteration.

When sending new iteration(s) of this series, could you run flake8 on patches 5
and 6 and fix all warnings?
https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/67435667


On Mon, May 07, 2018 at 05:30 PM, Matt Weber wrote:

> support/scripts/pkgstat: add target CPE reporting

For this patch and also for patch 6, one of these prefixes would be better:
support/scripts/pkg-stats: ...
pkg-stats: ...

[snip]
> +    def get_xml_dict(self):
> +        print "CPE: Fetching xml manifest..."
> +        try:
> +            compressed_cpe_file = urllib2.urlopen(CPE_XML_URL)
> +            print "CPE: Unzipping xml manifest..."
> +            cpe_file = gzip.GzipFile(fileobj=StringIO(compressed_cpe_file.read())).read()
> +            print "CPE: Converting xml manifest to dict..."
> +            self.all_cpes = xmltodict.parse(cpe_file)
> +        except urllib2.HTTPError, e:
> +            print "HTTP Error:", e.code, url
> +        except urllib2.URLError, e:
> +            print "URL Error:", e.reason, url

(after fixing the warning from flake8)
If the download fails the script continues and dies like this:

CPE: Fetching xml manifest...
URL Error: [Errno -2] Name or service not known
CPE: Checking for matches...
CPE: Searching for [cpe:2.3:a:gnu:bash:4.4.18:*:*:*:*:*:*:*]
Traceback (most recent call last):
  File "support/scripts/pkg-stats", line 595, in <module>
    __main__()
  File "support/scripts/pkg-stats", line 576, in __main__
    get_target_cpe_report(args.cpe_report,cpe_dict)
  File "support/scripts/pkg-stats", line 531, in get_target_cpe_report
    result = cpe_dict.find(cpe[0])
  File "support/scripts/pkg-stats", line 517, in find
    for cpe in self.all_cpes['cpe-list']['cpe-item']:
KeyError: 'cpe-list'

Maybe this is what you want:
        except ...
            print ...
            sys.exit(1)

[snip]
> +def get_target_cpe_report(cpe_report_file, cpe_dict):
> +    report_cpe_exact_match = ""
> +    report_cpe_needing_update = ""
> +    report_cpe_missing = ""
> +
> +    print "CPE: Checking for matches..."
> +    with open(cpe_report_file) as cpe_file:
> +        cpe_list = csv.reader(cpe_file)
> +        for cpe in cpe_list:
> +            if "CPE ID" not in cpe[0]:
           
The .csv file contains always only one header, right?
If so, you could do like it is done on utils/size-stats-compare (code not
tested):
        cpe_list = csv.reader(cpe_file)
        header = next(cpe_list)
        (... code to validate the header ...)
        for cpe in cpe_list:
            (... assume it is an entry, not a header...)

> +                result = cpe_dict.find(cpe[0])
> +                if not result:
> +                    cpe_no_version = cpe[0].split(":")[0]+":"+cpe[0].split(":")[1]+":"+cpe[0].split(":")[2]+":"+cpe[0].split(":")[3]+":"+cpe[0].split(":")[4]

Could this long line be replaced by this one?
                    cpe_no_version = ":".join(cpe[0].split(":", 5)[:5])

[snip]
>  def parse_args():
>      parser = argparse.ArgumentParser()
> @@ -493,6 +555,8 @@ def parse_args():
>                          help='Number of packages')
>      parser.add_argument('-p', dest='packages', action='store',
>                          help='List of packages (comma separated)')
> +    parser.add_argument('-c', dest='cpe_report', action='store',
> +                        help='Buildroot CPE Report (csv format)')

What do you think about this help instead:
                     help='CPE Report generated by make cpe-info (csv format)')
or something like this?

>      return parser.parse_args()
>  
>  
> @@ -505,22 +569,27 @@ def __main__():
[snip]
> +    if args.cpe_report:
> +        print "Performing Target CPE Report Analysis..."
> +        cpe_dict = CPE()
> +        cpe_dict.get_xml_dict()
> +        get_target_cpe_report(args.cpe_report,cpe_dict)

Is this code supposed to be used only to debug the script?
Or is its output to stdout supposed to used by the user to get a list of
packages without cpe info?

I ask this because this code branch still requires a '-o file' to be passed,
but it does not use it.

For the first case (only to debug the script) maybe it is acceptable as is.

But for the second case (script to be called by the end user), it is not nice to
require an argument that is not used. required=True could then be removed from
"parser.add_argument('-o'," and the check could be moved to __main__ (code not
tested):
def __main__():
    args = parse_args()
    if args.output is None and args.cpe_report is None:
        print "ERROR: either argument -o or -c required"
        sys.exit(1)
    if args.output and args.cpe_report:
        print "ERROR: -o and -c are mutually exclusive"
        sys.exit(1)


Regards,
Ricardo