Re: [edk2-platform patch 1/2] Platform/Intel:Add GenBiosId into edk2-platforms/Platform/Intel/Tools

["Leif Lindholm" <leif.lindholm@linaro.org>]

On Fri, Jun 21, 2019 at 01:58:17AM +0000, Fan, ZhijuX wrote:
> BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=1855
> 
> GenBiosId is a tool to generate the BIOS ID binary file which uses
> the data from the configuration file.
> This tool can be run in both Py2 and Py3.
> 
> This patch is going to add GenBiosId to Platform/Intel/Tools
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Zhiju.Fan <zhijux.fan@intel.com>
> ---
>  Platform/Intel/Tools/GenBiosId/BiosId.env   |  27 +++++
>  Platform/Intel/Tools/GenBiosId/GenBiosId.py | 180 ++++++++++++++++++++++++++++
>  2 files changed, 207 insertions(+)
>  create mode 100644 Platform/Intel/Tools/GenBiosId/BiosId.env
>  create mode 100644 Platform/Intel/Tools/GenBiosId/GenBiosId.py
> 
> diff --git a/Platform/Intel/Tools/GenBiosId/BiosId.env b/Platform/Intel/Tools/GenBiosId/BiosId.env
> new file mode 100644
> index 0000000000..e1e913da76
> --- /dev/null
> +++ b/Platform/Intel/Tools/GenBiosId/BiosId.env
> @@ -0,0 +1,27 @@
> +## @file
> +#  This file is used to define the BIOS ID parameters of the build.
> +#  This file is processed by GenBiosId.
> +#  Here, it is just a template and can be customized by user.
> +#
> +#  BIOS ID string format:
> +#    $(BOARD_ID)$(BOARD_REV).$(BOARD_EXT).$(VERSION_MAJOR).$(BUILD_TYPE)$(VERSION_MINOR).YYMMDDHHMM
> +#  All fields must have a fixed length. YYMMDDHHMM is UTC time.
> +#    Example: "EMLATOR1.000.0001.D01.1906141517"
> +#
> +#  If DATE is specified for YYMMDD and TIME is specified for HHMM like below,
> +#  GenBiosId will use the value of DATE and TIME to fill YYMMDDHHMM,
> +#  otherwise GenBiosId will fill YYMMDDHHMM with current UTC time of the build machine.
> +#    DATE          = 190614
> +#    TIME          = 1517
> +#
> +#  Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +[config]
> +BOARD_ID      = KBLRVP3
> +BOARD_REV     = 1
> +BOARD_EXT     = 000
> +BUILD_TYPE    = D
> +VERSION_MAJOR = 0001
> +VERSION_MINOR = 01
> diff --git a/Platform/Intel/Tools/GenBiosId/GenBiosId.py b/Platform/Intel/Tools/GenBiosId/GenBiosId.py
> new file mode 100644
> index 0000000000..20fb7592b4
> --- /dev/null
> +++ b/Platform/Intel/Tools/GenBiosId/GenBiosId.py
> @@ -0,0 +1,180 @@
> +## @file
> +# Trim files preprocessed by compiler
> +#
> +# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +
> +##
> +# Import Modules
> +#
> +import os
> +import sys
> +import time
> +import logging
> +import struct
> +import datetime
> +import argparse
> +import platform
> +
> +try:
> +    from configparser import ConfigParser
> +except:
> +    from ConfigParser import ConfigParser
> +
> +# Config message
> +_BIOS_Signature = "$IBIOSI$"
> +_SectionKeyName = '__name__'
> +_SectionName = 'config'
> +
> +_ConfigItem = {
> +    "BOARD_ID": {'Value': '', 'Length': 7},
> +    "BOARD_REV": {'Value': '', 'Length': 1},
> +    "BOARD_EXT": {'Value': '', 'Length': 3},
> +    "BUILD_TYPE": {'Value': '', 'Length': 1},
> +    "VERSION_MAJOR": {'Value': '0000', 'Length': 4},
> +    "VERSION_MINOR": {'Value': '00', 'Length': 2},
> +}
> +
> +# Version message
> +__prog__ = 'GenBiosld'
> +__description__ = 'Trim files preprocessed by compiler'
> +__copyright__ = 'Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> '
> +__version__ = '%s Version %s' % (__prog__, '0.1 ')
> +
> +# ExtraData message
> +_Usage = "Usage: GenBiosId -i Configfile -o OutputFile [-ot OutputTextFile]"
> +_ConfigSectionNotDefine = "Not support the config file format, need config section"
> +_ErrorMessageTemplate = '\n\n%(tool)s...\n : error: %(msg)s\n\t%(extra)s'
> +_ErrorLogger = logging.getLogger("tool_error")
> +_ErrorFormatter = logging.Formatter("%(message)s")
> +_ConfigLenInvalid = "Config item %s length is invalid"
> +_ConfigItemInvalid = "Item %s is invalid"
> +
> +# Error message
> +INFO = 20
> +ERRORCODE = 50
> +OPTION_MISSING = 'Missing option'
> +FORMAT_INVALID = 'Invalid syntax/format'
> +FILE_NOT_FOUND = 'File/directory not found in workspace'
> +FORMAT_UNKNOWN_ERROR = 'Unknown error in syntax/format'
> +FORMAT_NOT_SUPPORTED = 'Not supported syntax/format'
> +
> +
> +def SetEdkLogger():
> +    _ErrorLogger.setLevel(INFO)
> +    _ErrorCh = logging.StreamHandler(sys.stderr)
> +    _ErrorCh.setFormatter(_ErrorFormatter)
> +    _ErrorLogger.addHandler(_ErrorCh)
> +    return _ErrorLogger
> +
> +
> +# Output the error message and exit the tool
> +def EdkLogger(ToolName, Message, ExtraData):
> +    _ErrorLogger = SetEdkLogger()
> +    TemplateDict = {"tool": ToolName, "msg": Message, "extra": ExtraData}
> +    LogText = _ErrorMessageTemplate % TemplateDict
> +    _ErrorLogger.log(ERRORCODE, LogText)
> +    sys.exit()

Should the above two functions be in a common module in edk2?

> +
> +
> +# Open the file in the correct way
> +def FileOpen(FileName, Mode, Buffer=-1):
> +    def LongFilePath(FileName):
> +        FileName = os.path.normpath(FileName)
> +        if platform.system() == 'Windows':
> +            if FileName.startswith('\\\\?\\'):
> +                return FileName
> +            if FileName.startswith('\\\\'):
> +                return '\\\\?\\UNC\\' + FileName[2:]
> +            if os.path.isabs(FileName):
> +                return '\\\\?\\' + FileName
> +        return FileName
> +
> +    return open(LongFilePath(FileName), Mode, Buffer)
> +
> +
> +# Parse command line options
> +def MyOptionParser():
> +    parser = argparse.ArgumentParser(prog=__prog__,
> +                                     description=__description__ + __copyright__ + _Usage,
> +                                     conflict_handler='resolve')
> +    parser.add_argument('-v', '--version', action='version', version=__version__,
> +                        help="show program's version number and exit")
> +    parser.add_argument('-i', '--int', metavar='FILENAME', dest='InputFile', help="Input Config file")
> +    parser.add_argument('-o', '--out', metavar='FILENAME', dest='OutputFile', help="Output file")
> +    parser.add_argument('-ot', '--text', metavar='FILENAME', dest='OutputTextFile', help="Output Text file")

'-ot' is long for a short argument. Could it just be '-t', which also
matches '--text' better?

> +    Options = parser.parse_args()
> +    return Options
> +
> +
> +# Check the Tool for missing variables
> +def CheckOptions(Options):
> +    if len(sys.argv) != 5 and not (len(sys.argv) == 7 and Options.OutputTextFile):
> +        EdkLogger("GenBiosId", OPTION_MISSING, ExtraData=_Usage)
> +    elif not Options.InputFile or not Options.OutputFile:
> +        EdkLogger("GenBiosId", OPTION_MISSING, ExtraData=_Usage)

Can we use argparse to validate the arguments instead - that's what
it's for?

> +    InputFile = Options.InputFile
> +    OutputFile = Options.OutputFile
> +    OutputTextFile = Options.OutputTextFile
> +    if not os.path.exists(InputFile):
> +        EdkLogger("GenBiosId", FILE_NOT_FOUND, ExtraData="Input file not found")

https://stackoverflow.com/questions/37471636/python-argument-parsing-validation-best-practices
gives an example on how to make argparse verify that files exist.

> +    return InputFile, OutputFile, OutputTextFile
> +
> +
> +# Parse the input file and extract the information
> +def ParserInputFile(InputFile):
> +    cf = ConfigParser()
> +    cf.optionxform = str
> +    cf.read(InputFile)
> +    if _SectionName not in cf._sections:
> +        EdkLogger("GenBiosId", FORMAT_NOT_SUPPORTED, ExtraData=_ConfigSectionNotDefine)
> +    for Item in cf._sections[_SectionName]:
> +        if Item == _SectionKeyName:
> +            continue
> +        if Item not in _ConfigItem:
> +            EdkLogger("GenBiosId", FORMAT_INVALID, ExtraData=_ConfigItemInvalid % Item)
> +        _ConfigItem[Item]['Value'] = cf._sections[_SectionName][Item]
> +        if len(_ConfigItem[Item]['Value']) != _ConfigItem[Item]['Length']:
> +            EdkLogger("GenBiosId", FORMAT_INVALID, ExtraData=_ConfigLenInvalid % Item)
> +    for Item in _ConfigItem:
> +        if not _ConfigItem[Item]['Value']:
> +            EdkLogger("GenBiosId", FORMAT_UNKNOWN_ERROR, ExtraData="Item %s is missing" % Item)
> +    utcnow = datetime.datetime.utcnow()
> +    TimeStamp = time.strftime("%y%m%d%H%M", utcnow.timetuple())
> +
> +    Id_Str = _ConfigItem['BOARD_ID']['Value'] + _ConfigItem['BOARD_REV']['Value'] + '.' + _ConfigItem['BOARD_EXT'][
> +        'Value'] + '.' + _ConfigItem['VERSION_MAJOR']['Value'] + \
> +             '.' + _ConfigItem["BUILD_TYPE"]['Value'] + _ConfigItem['VERSION_MINOR']['Value'] + '.' + TimeStamp
> +    return Id_Str
> +
> +
> +# Output information to a file
> +def PrintOutputFile(OutputFile, OutputTextFile, Id_Str):
> +    with FileOpen(OutputFile, 'wb') as FdOut:
> +        for i in _BIOS_Signature:
> +            FdOut.write(struct.pack('B', ord(i)))
> +
> +        for i in Id_Str:
> +            FdOut.write(struct.pack('H', ord(i)))
> +
> +        FdOut.write(struct.pack('H', 0x00))
> +    if OutputTextFile:
> +        with FileOpen(OutputTextFile, 'w') as FdOut:
> +            FdOut.write(Id_Str)
> +
> +
> +# Tool entrance method
> +def Main():
> +    Options = MyOptionParser()
> +    InputFile, OutputFile, OutputTextFile = CheckOptions(Options)
> +    Id_Str = ParserInputFile(InputFile)
> +    PrintOutputFile(OutputFile, OutputTextFile, Id_Str)
> +    return 0
> +
> +
> +if __name__ == '__main__':
> +    r = Main()
> +    ## 0-127 is a safe return range, and 1 is a standard default error
> +    if r < 0 or r > 127: r = 1

What is the point of this dance? Main always returns 0 (which does not
sound ideal).

> +    sys.exit(r)

Implications for asynchronous i/o?

/
    Leif

> -- 
> 2.14.1.windows.1
>