Hello Ben,

Thanks a lot for your review and sorry for taking your time.
We will have an internal review and take your comments into account to prepare a new proposal.

Kind regards,
Daniel

> -----Original Message-----
> From: Ben Hutchings <ben.hutchings@codethink.co.uk>
> Sent: Wednesday, October 7, 2020 6:28 AM
> To: sangorrin daniel(サンゴリン ダニエル □ＳＷＣ◯ＡＣＴ) <daniel.sangorrin@toshiba.co.jp>; sz.lin@moxa.com; wens@csie.org
> Cc: cip-dev@lists.cip-project.org
> Subject: Re: [cip-kernel-sec] reports: add script to convert reports to csv format
> 
> On Fri, 2020-09-25 at 14:07 +0900, Daniel Sangorrin wrote:
> > The text version is probably enough for developers but customers
> > usually prefer to have a CSV that you can open with a spreadsheet
> > program and contains additional information. CVEs are sorted in rows
> > according to their criticality.
> [...]
> 
> I think this script is trying to do too many different things:
> 
> 1. Importing data from NVD
> 2. Importing data from Debian security tracker 3. Parsing an existing report (!) 4. Generating a new report
> 
> 1. If there's useful information from NVD that belongs in reports, and the license allows us to redistribute it, we should add an import
> script that adds that to the issue files (and extend the schema if necessary).  We can then use that in any of the reporting scripts.
> 
> 2. I'm not sure why the script is using Debian's general security tracker.  Debian's kernel-sec normally has better information for kernel
> issues, and the import_debian.py script already imports that.
> 
> 3. The output of report_affected.py is intended to be human-readable, and just happens to be relatively easy to parse.  If you want to
> use its output as input, that should either be done by adding a structured format (e.g. JSON) for the intermediate file, or by sharing
> code between the two reporting scripts so there's no need to use an intermediate file.
> 
> Other comments:
> 
> - The new script needs to be documented in README.md.
> 
> - Any files created in the process of importing data should go under the import/ subdirectory.
> 
> - Error handling needs improvement, e.g.:
> 
> > +def download_file(src, file, bar=""):
> > +    """Re-download file when an error occurred due to network connection problem.
> > +    """
> > +    for i in range(3):
> > +        try:
> > +            wget.download(src, file, bar)
> > +            break
> > +        except:
> > +            pass
> 
> This doesn't check whether there was a network error; it retries in case of *any* error.  The except block should specify which
> exception types we want to handle.
> 
> > +    if not os.path.exists(file):
> > +        print("ERROR: Can't download %s" % src)
> 
> Error messages should go to stderr.
> 
> > +        exit(1)
> 
> This should call sys.exit.
> 
> Ben.
> 
> --
> Ben Hutchings, Software Developer                         Codethink Ltd
> https://www.codethink.co.uk/                 Dale House, 35 Dale Street
>                                      Manchester, M1 2HF, United Kingdom