In v8, - Assigne patch file order
       - Add Acked-by tags
In v7, - Remove C RTC header files to under [Include.Common.Private]
         in RedfishPkg.dec.
       - address comments given by Mike Kinney.
In v6, Remove JanssonJsonMapping.h
In v5, move BaseUcs2Utf8Lib to under RedfishPkg.
In v4,
       - Address review comments
       - Seperate CRT functions to a individule library CrtLib under
         RedfishPkg.
       - Seperate UCS2-UTF8 functions to a individule library
         BaseUcs2Utf8Lib under MdeModulePkg.

In v3, Add jansson library as the required submoudle in
       CiSettings.py for CI test.
In v2, JsonLib is moved to under RedfishPkg.

edk2 JSON library is based on jansson open source
(https://github.com/akheron/jansson) and wrapped as an edk2
library. edk2 JsonLib will be used by edk2 Redfish feature
drivers (not contributed yet) and the edk2 port of libredfish
library (not contributed yet) based on DMTF GitHub
(https://github.com/DMTF/libredfish).

Jansson is licensed under the MIT license(refer to ReadMe.rst under edk2).
It is used in production and its API is stable. In UEFI/EDKII environment,
Redfish project consumes jansson to achieve JSON operations.

* Jansson version on edk2: 2.13.1

* EDKII jansson library wrapper:
   - JsonLib.h:
     This is the denifitions of EDKII JSON APIs which are mapped to
     jannson funcitons accordingly.

   - JanssonJsonLibMapping.h:
     This is the wrapper file to map funcitons and definitions used in
     native jannson applications to edk2 JsonLib. This avoids the
     modifications on native jannson applications to be built under
     edk2 environment.

*Known issue:
  Build fail with jansson/src/load.c, overrride and add code in load.c
  to conditionally use stdin according to HAVE_UNISTD_H macro.
  The PR is submitted to jansson open source community.
  https://github.com/akheron/jansson/pull/558

Signed-off-by: Abner Chang <abner.chang@hpe.com>

Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Nickle Wang <nickle.wang@hpe.com>
Cc: Peter O'Hanley <peter.ohanley@hpe.com>

Abner Chang (6):
  RedfishPkg/Ucs2Utf8lib: UCS2 to UFT8 manipulation library
  edk2: jansson submodule for edk2 JSON library
  RedfishPkg/CrtLib: C runtime library
  RedfishPkg/library: EDK2 port of jansson library
  RedfishPkg: Add EDK2 port of jansson library to build
  .pytool: Add required submodule for JsonLib

 .gitmodules                                   |    3 +
 .pytool/CISettings.py                         |    2 +
 ReadMe.rst                                    |    1 +
 RedfishPkg/Include/Crt/assert.h               |   16 +
 RedfishPkg/Include/Crt/errno.h                |   16 +
 RedfishPkg/Include/Crt/limits.h               |   16 +
 RedfishPkg/Include/Crt/math.h                 |   16 +
 RedfishPkg/Include/Crt/stdarg.h               |   15 +
 RedfishPkg/Include/Crt/stddef.h               |   16 +
 RedfishPkg/Include/Crt/stdio.h                |   15 +
 RedfishPkg/Include/Crt/stdlib.h               |   16 +
 RedfishPkg/Include/Crt/string.h               |   16 +
 RedfishPkg/Include/Crt/sys/time.h             |   15 +
 RedfishPkg/Include/Crt/sys/types.h            |   15 +
 RedfishPkg/Include/Crt/time.h                 |   15 +
 RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h  |   61 +
 RedfishPkg/Include/Library/CrtLib.h           |  191 +++
 RedfishPkg/Include/Library/JsonLib.h          |  763 +++++++++++
 .../Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c |  421 +++++++
 .../BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf       |   31 +
 RedfishPkg/Library/CrtLib/CrtLib.c            |  595 +++++++++
 RedfishPkg/Library/CrtLib/CrtLib.inf          |   38 +
 RedfishPkg/Library/JsonLib/JsonLib.c          |  964 ++++++++++++++
 RedfishPkg/Library/JsonLib/JsonLib.inf        |   89 ++
 RedfishPkg/Library/JsonLib/Readme.rst         |   40 +
 RedfishPkg/Library/JsonLib/jansson            |    1 +
 RedfishPkg/Library/JsonLib/jansson_config.h   |   41 +
 .../Library/JsonLib/jansson_private_config.h  |   19 +
 RedfishPkg/Library/JsonLib/load.c             | 1111 +++++++++++++++++
 RedfishPkg/RedfishLibs.dsc.inc                |    3 +
 RedfishPkg/RedfishPkg.ci.yaml                 |   25 +
 RedfishPkg/RedfishPkg.dec                     |   25 +
 RedfishPkg/RedfishPkg.dsc                     |    3 +
 33 files changed, 4614 insertions(+)
 create mode 100644 RedfishPkg/Include/Crt/assert.h
 create mode 100644 RedfishPkg/Include/Crt/errno.h
 create mode 100644 RedfishPkg/Include/Crt/limits.h
 create mode 100644 RedfishPkg/Include/Crt/math.h
 create mode 100644 RedfishPkg/Include/Crt/stdarg.h
 create mode 100644 RedfishPkg/Include/Crt/stddef.h
 create mode 100644 RedfishPkg/Include/Crt/stdio.h
 create mode 100644 RedfishPkg/Include/Crt/stdlib.h
 create mode 100644 RedfishPkg/Include/Crt/string.h
 create mode 100644 RedfishPkg/Include/Crt/sys/time.h
 create mode 100644 RedfishPkg/Include/Crt/sys/types.h
 create mode 100644 RedfishPkg/Include/Crt/time.h
 create mode 100644 RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h
 create mode 100644 RedfishPkg/Include/Library/CrtLib.h
 create mode 100644 RedfishPkg/Include/Library/JsonLib.h
 create mode 100644 RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c
 create mode 100644 RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
 create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.c
 create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.inf
 create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.c
 create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.inf
 create mode 100644 RedfishPkg/Library/JsonLib/Readme.rst
 create mode 160000 RedfishPkg/Library/JsonLib/jansson
 create mode 100644 RedfishPkg/Library/JsonLib/jansson_config.h
 create mode 100644 RedfishPkg/Library/JsonLib/jansson_private_config.h
 create mode 100644 RedfishPkg/Library/JsonLib/load.c

-- 
2.17.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69164): https://edk2.groups.io/g/devel/message/69164
Mute This Topic: https://groups.io/mt/79052636/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Abner,

The include path in the [Include.Common.Private] section should not be 
in the same file path as the [Include] section.  This allows private include
files to be accessed.

Instead, you should create a new top level package directory called PrivateInclude
and put all non-public include content in that directory.  The UnitTestFrameworkPkg
is a good reference that demonstrates this design pattern.

The library class name CrtLib is very generic and the library class is in the 
public includes so it is exported as a public interface from the RedfishPkg.
This CrtLib implementation appears to be tuned to support the dependencies
for the jansson submodule.  I recommend changing the library class name and
the library instance name so it is clear that this CrtLib is only for jansson.
Perhaps 'JanssonCrtLib'.  Also, does this library class (CrtLib.h) need to 
be exported from RedfishPkg as a public interface?

* RedfishPkg/Include/Library/CrtLib.h
  + Remove reference to MDE_CPU_IA64.  This has been retired from the edk2 repo.
  + I do see one remaining reference to this in the CryptoPkg that need to be
    removed.

* RedfishPkg/Include/Library/JsonLib.h
  + Some of the function descriptions are very brief and I can not tell 
    how to use the service from the description and more importantly, I 
    would not know how to write a unit test to verify the expected behavior.
    Since these are a set of public APIs from this package that you 
    expect modules/libs from outside of RedfishPkg to use, then all of 
    these public APIs must be fully documented.
  + Are there any of these APIs that are not really needed by modules/libs
    outside the RedfishPkg?  It would be better to remove APIs that are
    not needed outside RedfishPkg from this public include file.
  + A couple examples that stood out are:
    JsonDecreaseReference()
    JsonIncreaseReference()
    JsonObjectIterator()
    JsonObjectIteratorValue()
    JsonObjectIteratorNext()
  + JsonDumpString() 
    func header params do not match func prototype
    Does not describe who allocates the return buffer and who
    is responsible for freeing the buffer returned.
    What do Flags do?  What is the difference between this
    function and JsonToText()?
  + JsonLoadBuffer() - Error param description missing
  + JsonArrayGetValue() does not describe the conditions NULL
    is returned.
  + JsonObjectGetKeys().  Return type is CHAR8**.  What
    function need to be called to free the array?
  + JsonValueGetAsciiString() and JsonValueGetUnicodeString()
    are asymmetric.  The Ascii one states that change will
    modify the original string.  The Unicode one says that the
    caller needs to free the returned string.  Any reason we
    can not make them symmetric?  Also, if Ascii one should
    not be modified, why isn’t the return type CONST?

* RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
  + Change [Sources.common] to [Sources]

* RedfishPkg/Library/CrtLib/CrtLib.inf
  + Does this lib instance really depend on MdeModulePkg?
  + Have you reviewed the module types this lib instance is
    compatible with?  Why does it need to support DXE_CORE?
    Are their JsonLib use cases for DXE_RUNTIME and SMM drivers?
    If there are SMM use cases, then why is MM excluded?

* RedfishPkg/Library/JsonLib
  + Readme.rst still has references to JanssonJsonLibMapping.h
  + Have you reviewed the module types this lib instance is
    compatible with?  Why does it need to support DXE_CORE?
    Are their JsonLib use cases for DXE_RUNTIME and SMM drivers?
    If there are SMM use cases, then why is MM excluded?
  + JsonLib.inf has a [BuildOptions] section that disables some warnings that
    make me concerned that this library may have some issues with 32-bit builds.
    Have you fully validated all 32-bit CPU builds and validated the functionality
    of all services from the JsonLib for all ranges of input values?

* RedfishLibs.dsc.inc
  It is better to add [LibraryClasses] statement to this file, so it 
  can be include anywhere in a DSC file.  With current implementation
  if it is not included within a [LibraryClasses] section, it will
  generate a build error.  You might also consider changing the name
  of this file in case you want to add more than just lib mappings
  to this file in the future.  See UnitTestFramworkPkg for examples.

Best regards,

Mike

> -----Original Message-----
> From: Abner Chang <abner.chang@hpe.com>
> Sent: Thursday, December 17, 2020 5:19 AM
> To: devel@edk2.groups.io
> Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Liming Gao <gaoliming@byosoft.com.cn>; Nickle Wang <nickle.wang@hpe.com>; Peter O'Hanley <peter.ohanley@hpe.com>
> Subject: [PATCH v8 0/6] jansson edk2 port
> 
> In v8, - Assigne patch file order
>        - Add Acked-by tags
> In v7, - Remove C RTC header files to under [Include.Common.Private]
>          in RedfishPkg.dec.
>        - address comments given by Mike Kinney.
> In v6, Remove JanssonJsonMapping.h
> In v5, move BaseUcs2Utf8Lib to under RedfishPkg.
> In v4,
>        - Address review comments
>        - Seperate CRT functions to a individule library CrtLib under
>          RedfishPkg.
>        - Seperate UCS2-UTF8 functions to a individule library
>          BaseUcs2Utf8Lib under MdeModulePkg.
> 
> In v3, Add jansson library as the required submoudle in
>        CiSettings.py for CI test.
> In v2, JsonLib is moved to under RedfishPkg.
> 
> edk2 JSON library is based on jansson open source
> (https://github.com/akheron/jansson) and wrapped as an edk2
> library. edk2 JsonLib will be used by edk2 Redfish feature
> drivers (not contributed yet) and the edk2 port of libredfish
> library (not contributed yet) based on DMTF GitHub
> (https://github.com/DMTF/libredfish).
> 
> Jansson is licensed under the MIT license(refer to ReadMe.rst under edk2).
> It is used in production and its API is stable. In UEFI/EDKII environment,
> Redfish project consumes jansson to achieve JSON operations.
> 
> * Jansson version on edk2: 2.13.1
> 
> * EDKII jansson library wrapper:
>    - JsonLib.h:
>      This is the denifitions of EDKII JSON APIs which are mapped to
>      jannson funcitons accordingly.
> 
>    - JanssonJsonLibMapping.h:
>      This is the wrapper file to map funcitons and definitions used in
>      native jannson applications to edk2 JsonLib. This avoids the
>      modifications on native jannson applications to be built under
>      edk2 environment.
> 
> *Known issue:
>   Build fail with jansson/src/load.c, overrride and add code in load.c
>   to conditionally use stdin according to HAVE_UNISTD_H macro.
>   The PR is submitted to jansson open source community.
>   https://github.com/akheron/jansson/pull/558
> 
> Signed-off-by: Abner Chang <abner.chang@hpe.com>
> 
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Nickle Wang <nickle.wang@hpe.com>
> Cc: Peter O'Hanley <peter.ohanley@hpe.com>
> 
> Abner Chang (6):
>   RedfishPkg/Ucs2Utf8lib: UCS2 to UFT8 manipulation library
>   edk2: jansson submodule for edk2 JSON library
>   RedfishPkg/CrtLib: C runtime library
>   RedfishPkg/library: EDK2 port of jansson library
>   RedfishPkg: Add EDK2 port of jansson library to build
>   .pytool: Add required submodule for JsonLib
> 
>  .gitmodules                                   |    3 +
>  .pytool/CISettings.py                         |    2 +
>  ReadMe.rst                                    |    1 +
>  RedfishPkg/Include/Crt/assert.h               |   16 +
>  RedfishPkg/Include/Crt/errno.h                |   16 +
>  RedfishPkg/Include/Crt/limits.h               |   16 +
>  RedfishPkg/Include/Crt/math.h                 |   16 +
>  RedfishPkg/Include/Crt/stdarg.h               |   15 +
>  RedfishPkg/Include/Crt/stddef.h               |   16 +
>  RedfishPkg/Include/Crt/stdio.h                |   15 +
>  RedfishPkg/Include/Crt/stdlib.h               |   16 +
>  RedfishPkg/Include/Crt/string.h               |   16 +
>  RedfishPkg/Include/Crt/sys/time.h             |   15 +
>  RedfishPkg/Include/Crt/sys/types.h            |   15 +
>  RedfishPkg/Include/Crt/time.h                 |   15 +
>  RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h  |   61 +
>  RedfishPkg/Include/Library/CrtLib.h           |  191 +++
>  RedfishPkg/Include/Library/JsonLib.h          |  763 +++++++++++
>  .../Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c |  421 +++++++
>  .../BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf       |   31 +
>  RedfishPkg/Library/CrtLib/CrtLib.c            |  595 +++++++++
>  RedfishPkg/Library/CrtLib/CrtLib.inf          |   38 +
>  RedfishPkg/Library/JsonLib/JsonLib.c          |  964 ++++++++++++++
>  RedfishPkg/Library/JsonLib/JsonLib.inf        |   89 ++
>  RedfishPkg/Library/JsonLib/Readme.rst         |   40 +
>  RedfishPkg/Library/JsonLib/jansson            |    1 +
>  RedfishPkg/Library/JsonLib/jansson_config.h   |   41 +
>  .../Library/JsonLib/jansson_private_config.h  |   19 +
>  RedfishPkg/Library/JsonLib/load.c             | 1111 +++++++++++++++++
>  RedfishPkg/RedfishLibs.dsc.inc                |    3 +
>  RedfishPkg/RedfishPkg.ci.yaml                 |   25 +
>  RedfishPkg/RedfishPkg.dec                     |   25 +
>  RedfishPkg/RedfishPkg.dsc                     |    3 +
>  33 files changed, 4614 insertions(+)
>  create mode 100644 RedfishPkg/Include/Crt/assert.h
>  create mode 100644 RedfishPkg/Include/Crt/errno.h
>  create mode 100644 RedfishPkg/Include/Crt/limits.h
>  create mode 100644 RedfishPkg/Include/Crt/math.h
>  create mode 100644 RedfishPkg/Include/Crt/stdarg.h
>  create mode 100644 RedfishPkg/Include/Crt/stddef.h
>  create mode 100644 RedfishPkg/Include/Crt/stdio.h
>  create mode 100644 RedfishPkg/Include/Crt/stdlib.h
>  create mode 100644 RedfishPkg/Include/Crt/string.h
>  create mode 100644 RedfishPkg/Include/Crt/sys/time.h
>  create mode 100644 RedfishPkg/Include/Crt/sys/types.h
>  create mode 100644 RedfishPkg/Include/Crt/time.h
>  create mode 100644 RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h
>  create mode 100644 RedfishPkg/Include/Library/CrtLib.h
>  create mode 100644 RedfishPkg/Include/Library/JsonLib.h
>  create mode 100644 RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c
>  create mode 100644 RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
>  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.c
>  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.inf
>  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.c
>  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.inf
>  create mode 100644 RedfishPkg/Library/JsonLib/Readme.rst
>  create mode 160000 RedfishPkg/Library/JsonLib/jansson
>  create mode 100644 RedfishPkg/Library/JsonLib/jansson_config.h
>  create mode 100644 RedfishPkg/Library/JsonLib/jansson_private_config.h
>  create mode 100644 RedfishPkg/Library/JsonLib/load.c
> 
> --
> 2.17.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69272): https://edk2.groups.io/g/devel/message/69272
Mute This Topic: https://groups.io/mt/79052636/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Mike, response in below. Also v9 is sent.

Thanks for review this in detail.
Abner

> -----Original Message-----
> From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> Sent: Monday, December 21, 2020 3:43 AM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>; Laszlo
> Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> <nickle.wang@hpe.com>; O'Hanley, Peter (EXL) <peter.ohanley@hpe.com>
> Subject: RE: [PATCH v8 0/6] jansson edk2 port
> 
> Hi Abner,
> 
> The include path in the [Include.Common.Private] section should not be in
> the same file path as the [Include] section.  This allows private include files to
> be accessed.
> 
> Instead, you should create a new top level package directory called
> PrivateInclude and put all non-public include content in that directory.  The
> UnitTestFrameworkPkg is a good reference that demonstrates this design
> pattern.
Yes, this looks better. All Crt header files and CrtLib.h will be moved to under PrivateInclude.
> 
> The library class name CrtLib is very generic and the library class is in the
> public includes so it is exported as a public interface from the RedfishPkg.
> This CrtLib implementation appears to be tuned to support the
> dependencies for the jansson submodule.  I recommend changing the library
> class name and the library instance name so it is clear that this CrtLib is only
> for jansson.
> Perhaps 'JanssonCrtLib'. 
The CrtLib is not used by jannson lib, libredfish as well (https://github.com/DMTF/libredfish). So JanssonCrtLib is not a good naming, will keep that as it.

>Also, does this library class (CrtLib.h) need to be
> exported from RedfishPkg as a public interface?
No, it will be moved to under PrivateInclude.

If CrtLib.h is defined as a private library and CrtLib.h is in PrivateInclude which is not exposed to other packages, then why do we care about the naming of CrtLib is too generic? Which is the private library under RedfishPkg.
Move those header files to under PrivateInclude seems clear to people that CrtLib is only for RedfishPkg. I think we don’t have to change the naming in this case.

> 
> * RedfishPkg/Include/Library/CrtLib.h
>   + Remove reference to MDE_CPU_IA64.  This has been retired from the
> edk2 repo.
Ok.
>   + I do see one remaining reference to this in the CryptoPkg that need to be
>     removed.
> 
> * RedfishPkg/Include/Library/JsonLib.h
>   + Some of the function descriptions are very brief and I can not tell
>     how to use the service from the description and more importantly, I
>     would not know how to write a unit test to verify the expected behavior.
>     Since these are a set of public APIs from this package that you
>     expect modules/libs from outside of RedfishPkg to use, then all of
>     these public APIs must be fully documented.
Most of the API in JsonLib are wrappers of native jansson APIs. We can mention the URL to jansson API reference document in file header. https://jansson.readthedocs.io/en/2.13/index.html
I will review all function header again,  please check v9 patch later.

>   + Are there any of these APIs that are not really needed by modules/libs
>     outside the RedfishPkg?  It would be better to remove APIs that are
>     not needed outside RedfishPkg from this public include file.
>   + A couple examples that stood out are:
>     JsonDecreaseReference()
>     JsonIncreaseReference()
>     JsonObjectIterator()
>     JsonObjectIteratorValue()
>     JsonObjectIteratorNext()
The native jansson APIs of all above wrappers are used in libredfish. So that is the potential use case for other JSON applications.

>   + JsonDumpString()
>     func header params do not match func prototype
>     Does not describe who allocates the return buffer and who
>     is responsible for freeing the buffer returned.
>     What do Flags do?  What is the difference between this
>     function and JsonToText()?
In JsonToText, it only converts JSON array and object to text. We could just remove this for now because I don’t really remember the reason to create this function, it has been a while. Seems to me it is fine to remove this function.

>   + JsonLoadBuffer() - Error param description missing
>   + JsonArrayGetValue() does not describe the conditions NULL
>     is returned.
>   + JsonObjectGetKeys().  Return type is CHAR8**.  What
>     function need to be called to free the array?
All above addressed.

>   + JsonValueGetAsciiString() and JsonValueGetUnicodeString()
>     are asymmetric.  The Ascii one states that change will
>     modify the original string.  The Unicode one says that the
>     caller needs to free the returned string.  Any reason we
>     can not make them symmetric? 
Jansson doesn't support Unicode string. The memory of the unicode string returned by JsonValueGetUnicodeString
Is allocated by UTF8StrToUCS2().
 > Also, if Ascii one should
>     not be modified, why isn’t the return type CONST?
Yes. will do that.

> 
> * RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
>   + Change [Sources.common] to [Sources]
done
> * RedfishPkg/Library/CrtLib/CrtLib.inf
>   + Does this lib instance really depend on MdeModulePkg?
Yes, SortLib
>   + Have you reviewed the module types this lib instance is
>     compatible with?  Why does it need to support DXE_CORE?
>     Are their JsonLib use cases for DXE_RUNTIME and SMM drivers?
>     If there are SMM use cases, then why is MM excluded?
Will just keep DXE_DRVER, UEFI_APPLICATION and UEFI_DRIVER. These are module types have potential use cases I can think of now.
> 
> * RedfishPkg/Library/JsonLib
>   + Readme.rst still has references to JanssonJsonLibMapping.h
done
>   + Have you reviewed the module types this lib instance is
>     compatible with?  Why does it need to support DXE_CORE?
>     Are their JsonLib use cases for DXE_RUNTIME and SMM drivers?
>     If there are SMM use cases, then why is MM excluded?
Answered above
>   + JsonLib.inf has a [BuildOptions] section that disables some warnings that
>     make me concerned that this library may have some issues with 32-bit
> builds.
>     Have you fully validated all 32-bit CPU builds and validated the functionality
>     of all services from the JsonLib for all ranges of input values?
Yes, IA32 build is validated. But not validating on the functionalities IA32. I don't have that use case, we can get back to fix the issue if any when  someone runs this on IA32.
> 
> * RedfishLibs.dsc.inc
>   It is better to add [LibraryClasses] statement to this file, so it
>   can be include anywhere in a DSC file.  With current implementation
>   if it is not included within a [LibraryClasses] section, it will
>   generate a build error. 
I will separate this to another set of patch. Don’t mix up with jansson edk2 port.

>You might also consider changing the name
>   of this file in case you want to add more than just lib mappings
>   to this file in the future.  See UnitTestFramworkPkg for examples.
> 
> Best regards,
> 
> Mike
> 
> > -----Original Message-----
> > From: Abner Chang <abner.chang@hpe.com>
> > Sent: Thursday, December 17, 2020 5:19 AM
> > To: devel@edk2.groups.io
> > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>; Laszlo
> > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Kinney,
> > Michael D <michael.d.kinney@intel.com>; Liming Gao
> > <gaoliming@byosoft.com.cn>; Nickle Wang <nickle.wang@hpe.com>;
> Peter
> > O'Hanley <peter.ohanley@hpe.com>
> > Subject: [PATCH v8 0/6] jansson edk2 port
> >
> > In v8, - Assigne patch file order
> >        - Add Acked-by tags
> > In v7, - Remove C RTC header files to under [Include.Common.Private]
> >          in RedfishPkg.dec.
> >        - address comments given by Mike Kinney.
> > In v6, Remove JanssonJsonMapping.h
> > In v5, move BaseUcs2Utf8Lib to under RedfishPkg.
> > In v4,
> >        - Address review comments
> >        - Seperate CRT functions to a individule library CrtLib under
> >          RedfishPkg.
> >        - Seperate UCS2-UTF8 functions to a individule library
> >          BaseUcs2Utf8Lib under MdeModulePkg.
> >
> > In v3, Add jansson library as the required submoudle in
> >        CiSettings.py for CI test.
> > In v2, JsonLib is moved to under RedfishPkg.
> >
> > edk2 JSON library is based on jansson open source
> > (https://github.com/akheron/jansson) and wrapped as an edk2 library.
> > edk2 JsonLib will be used by edk2 Redfish feature drivers (not
> > contributed yet) and the edk2 port of libredfish library (not
> > contributed yet) based on DMTF GitHub
> > (https://github.com/DMTF/libredfish).
> >
> > Jansson is licensed under the MIT license(refer to ReadMe.rst under edk2).
> > It is used in production and its API is stable. In UEFI/EDKII
> > environment, Redfish project consumes jansson to achieve JSON
> operations.
> >
> > * Jansson version on edk2: 2.13.1
> >
> > * EDKII jansson library wrapper:
> >    - JsonLib.h:
> >      This is the denifitions of EDKII JSON APIs which are mapped to
> >      jannson funcitons accordingly.
> >
> >    - JanssonJsonLibMapping.h:
> >      This is the wrapper file to map funcitons and definitions used in
> >      native jannson applications to edk2 JsonLib. This avoids the
> >      modifications on native jannson applications to be built under
> >      edk2 environment.
> >
> > *Known issue:
> >   Build fail with jansson/src/load.c, overrride and add code in load.c
> >   to conditionally use stdin according to HAVE_UNISTD_H macro.
> >   The PR is submitted to jansson open source community.
> >   https://github.com/akheron/jansson/pull/558
> >
> > Signed-off-by: Abner Chang <abner.chang@hpe.com>
> >
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > Cc: Andrew Fish <afish@apple.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Leif Lindholm <leif@nuviainc.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Nickle Wang <nickle.wang@hpe.com>
> > Cc: Peter O'Hanley <peter.ohanley@hpe.com>
> >
> > Abner Chang (6):
> >   RedfishPkg/Ucs2Utf8lib: UCS2 to UFT8 manipulation library
> >   edk2: jansson submodule for edk2 JSON library
> >   RedfishPkg/CrtLib: C runtime library
> >   RedfishPkg/library: EDK2 port of jansson library
> >   RedfishPkg: Add EDK2 port of jansson library to build
> >   .pytool: Add required submodule for JsonLib
> >
> >  .gitmodules                                   |    3 +
> >  .pytool/CISettings.py                         |    2 +
> >  ReadMe.rst                                    |    1 +
> >  RedfishPkg/Include/Crt/assert.h               |   16 +
> >  RedfishPkg/Include/Crt/errno.h                |   16 +
> >  RedfishPkg/Include/Crt/limits.h               |   16 +
> >  RedfishPkg/Include/Crt/math.h                 |   16 +
> >  RedfishPkg/Include/Crt/stdarg.h               |   15 +
> >  RedfishPkg/Include/Crt/stddef.h               |   16 +
> >  RedfishPkg/Include/Crt/stdio.h                |   15 +
> >  RedfishPkg/Include/Crt/stdlib.h               |   16 +
> >  RedfishPkg/Include/Crt/string.h               |   16 +
> >  RedfishPkg/Include/Crt/sys/time.h             |   15 +
> >  RedfishPkg/Include/Crt/sys/types.h            |   15 +
> >  RedfishPkg/Include/Crt/time.h                 |   15 +
> >  RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h  |   61 +
> >  RedfishPkg/Include/Library/CrtLib.h           |  191 +++
> >  RedfishPkg/Include/Library/JsonLib.h          |  763 +++++++++++
> >  .../Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c |  421 +++++++
> >  .../BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf       |   31 +
> >  RedfishPkg/Library/CrtLib/CrtLib.c            |  595 +++++++++
> >  RedfishPkg/Library/CrtLib/CrtLib.inf          |   38 +
> >  RedfishPkg/Library/JsonLib/JsonLib.c          |  964 ++++++++++++++
> >  RedfishPkg/Library/JsonLib/JsonLib.inf        |   89 ++
> >  RedfishPkg/Library/JsonLib/Readme.rst         |   40 +
> >  RedfishPkg/Library/JsonLib/jansson            |    1 +
> >  RedfishPkg/Library/JsonLib/jansson_config.h   |   41 +
> >  .../Library/JsonLib/jansson_private_config.h  |   19 +
> >  RedfishPkg/Library/JsonLib/load.c             | 1111 +++++++++++++++++
> >  RedfishPkg/RedfishLibs.dsc.inc                |    3 +
> >  RedfishPkg/RedfishPkg.ci.yaml                 |   25 +
> >  RedfishPkg/RedfishPkg.dec                     |   25 +
> >  RedfishPkg/RedfishPkg.dsc                     |    3 +
> >  33 files changed, 4614 insertions(+)
> >  create mode 100644 RedfishPkg/Include/Crt/assert.h  create mode
> > 100644 RedfishPkg/Include/Crt/errno.h  create mode 100644
> > RedfishPkg/Include/Crt/limits.h  create mode 100644
> > RedfishPkg/Include/Crt/math.h  create mode 100644
> > RedfishPkg/Include/Crt/stdarg.h  create mode 100644
> > RedfishPkg/Include/Crt/stddef.h  create mode 100644
> > RedfishPkg/Include/Crt/stdio.h  create mode 100644
> > RedfishPkg/Include/Crt/stdlib.h  create mode 100644
> > RedfishPkg/Include/Crt/string.h  create mode 100644
> > RedfishPkg/Include/Crt/sys/time.h  create mode 100644
> > RedfishPkg/Include/Crt/sys/types.h
> >  create mode 100644 RedfishPkg/Include/Crt/time.h  create mode 100644
> > RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h
> >  create mode 100644 RedfishPkg/Include/Library/CrtLib.h
> >  create mode 100644 RedfishPkg/Include/Library/JsonLib.h
> >  create mode 100644
> > RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c
> >  create mode 100644
> > RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
> >  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.c
> >  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.inf
> >  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.c
> >  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.inf
> >  create mode 100644 RedfishPkg/Library/JsonLib/Readme.rst
> >  create mode 160000 RedfishPkg/Library/JsonLib/jansson
> >  create mode 100644 RedfishPkg/Library/JsonLib/jansson_config.h
> >  create mode 100644
> > RedfishPkg/Library/JsonLib/jansson_private_config.h
> >  create mode 100644 RedfishPkg/Library/JsonLib/load.c
> >
> > --
> > 2.17.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69310): https://edk2.groups.io/g/devel/message/69310
Mute This Topic: https://groups.io/mt/79052636/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Abner,

A few comments below.

Best regards,

Mike

> -----Original Message-----
> From: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>
> Sent: Monday, December 21, 2020 12:17 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
> Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming Gao <gaoliming@byosoft.com.cn>; Wang, Nickle
> (HPS SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL) <peter.ohanley@hpe.com>
> Subject: RE: [PATCH v8 0/6] jansson edk2 port
> 
> Mike, response in below. Also v9 is sent.
> 
> Thanks for review this in detail.
> Abner
> 
> > -----Original Message-----
> > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> > Sent: Monday, December 21, 2020 3:43 AM
> > To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> > devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>; Laszlo
> > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming Gao
> > <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL) <peter.ohanley@hpe.com>
> > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> >
> > Hi Abner,
> >
> > The include path in the [Include.Common.Private] section should not be in
> > the same file path as the [Include] section.  This allows private include files to
> > be accessed.
> >
> > Instead, you should create a new top level package directory called
> > PrivateInclude and put all non-public include content in that directory.  The
> > UnitTestFrameworkPkg is a good reference that demonstrates this design
> > pattern.
> Yes, this looks better. All Crt header files and CrtLib.h will be moved to under PrivateInclude.
> >
> > The library class name CrtLib is very generic and the library class is in the
> > public includes so it is exported as a public interface from the RedfishPkg.
> > This CrtLib implementation appears to be tuned to support the
> > dependencies for the jansson submodule.  I recommend changing the library
> > class name and the library instance name so it is clear that this CrtLib is only
> > for jansson.
> > Perhaps 'JanssonCrtLib'.
> The CrtLib is not used by jannson lib, libredfish as well (https://github.com/DMTF/libredfish). So JanssonCrtLib is not a
> good naming, will keep that as it.
> 
> >Also, does this library class (CrtLib.h) need to be
> > exported from RedfishPkg as a public interface?
> No, it will be moved to under PrivateInclude.
> 
> If CrtLib.h is defined as a private library and CrtLib.h is in PrivateInclude which is not exposed to other packages, then
> why do we care about the naming of CrtLib is too generic? Which is the private library under RedfishPkg.
> Move those header files to under PrivateInclude seems clear to people that CrtLib is only for RedfishPkg. I think we don’t
> have to change the naming in this case.

In general I agree, but there is one place where the lib mapping has wider scope, and that is in a platform DSC
file.  The lib class name CrtLib has to be listed in the DSC file.  If more than one package defines a library
class called CrtLib, then the last mapping used will be used for all modules if it is listed in the [LibraryClasses]
section of the DSC file.  The only was to handle this type of name collision is to use a module scoped 
library mapping in the <LibraryClasses> section of each individual module that uses a CrtLib.  I am only
trying to avoid the potential for future name collisions for a library class/instance that is specific to
the JSON use case.  

We recognize that several modules in edk2 have this type of gasket between an EDK II env and a standard C
lib.  We may find a we to implement a more generic gasket and retire the use of the implementation specific
ones.  This is another reason to use an implementation specific name in this use case and be able to
introduce a more generic name in the future.

Another comment below to see if there is a way to avoid introducing a CrtLib class.

> 
> >
> > * RedfishPkg/Include/Library/CrtLib.h
> >   + Remove reference to MDE_CPU_IA64.  This has been retired from the
> > edk2 repo.
> Ok.
> >   + I do see one remaining reference to this in the CryptoPkg that need to be
> >     removed.
> >
> > * RedfishPkg/Include/Library/JsonLib.h
> >   + Some of the function descriptions are very brief and I can not tell
> >     how to use the service from the description and more importantly, I
> >     would not know how to write a unit test to verify the expected behavior.
> >     Since these are a set of public APIs from this package that you
> >     expect modules/libs from outside of RedfishPkg to use, then all of
> >     these public APIs must be fully documented.
> Most of the API in JsonLib are wrappers of native jansson APIs. We can mention the URL to jansson API reference document
> in file header. https://jansson.readthedocs.io/en/2.13/index.html
> I will review all function header again,  please check v9 patch later.
> 
> >   + Are there any of these APIs that are not really needed by modules/libs
> >     outside the RedfishPkg?  It would be better to remove APIs that are
> >     not needed outside RedfishPkg from this public include file.
> >   + A couple examples that stood out are:
> >     JsonDecreaseReference()
> >     JsonIncreaseReference()
> >     JsonObjectIterator()
> >     JsonObjectIteratorValue()
> >     JsonObjectIteratorNext()
> The native jansson APIs of all above wrappers are used in libredfish. So that is the potential use case for other JSON
> applications.

Can you provide a pointer to an example of this use case?

I am trying to make sure we are doing the right design of RedfishPkg with respect to CrtLib
for this use case.  Are these are JSON apps that do not use JsonLib, but are expected to build
in an EDK II build environment?  Would these JSON apps require more standard C lib services 
than the CrtLib used to build JsonLib?  Where would those additional C lib services come from?

There is a standard C lib in edk2-libc.  Would that be required to build the JSON apps.

In other uses of submodules with C lib dependencies, a new CrtLib like lib class was not
added and instead the support was added directly to the EDK II wrapper APIs. Some examples are:
  * MdeModulePkg\Library\BrotliCustomDecompressLib
  * MdeModulePkg\Universal\RegularExpressionDxe

If there are use cases where the CrtLib will be added to an INF outside the RedfishPkg, then 
I still think the name needs to be changed to be specific to the JSON use case.

If the use of CrtLib is only within the RedfishPkg, then the Library Class can be 
declared in a [LibraryClasses.Common.Private] section of the DEC file.  See
UnitTestFrameworkPkg for examples.

> 
> >   + JsonDumpString()
> >     func header params do not match func prototype
> >     Does not describe who allocates the return buffer and who
> >     is responsible for freeing the buffer returned.
> >     What do Flags do?  What is the difference between this
> >     function and JsonToText()?
> In JsonToText, it only converts JSON array and object to text. We could just remove this for now because I don’t really
> remember the reason to create this function, it has been a while. Seems to me it is fine to remove this function.
> 
> >   + JsonLoadBuffer() - Error param description missing
> >   + JsonArrayGetValue() does not describe the conditions NULL
> >     is returned.
> >   + JsonObjectGetKeys().  Return type is CHAR8**.  What
> >     function need to be called to free the array?
> All above addressed.
> 
> >   + JsonValueGetAsciiString() and JsonValueGetUnicodeString()
> >     are asymmetric.  The Ascii one states that change will
> >     modify the original string.  The Unicode one says that the
> >     caller needs to free the returned string.  Any reason we
> >     can not make them symmetric?
> Jansson doesn't support Unicode string. The memory of the unicode string returned by JsonValueGetUnicodeString
> Is allocated by UTF8StrToUCS2().
>  > Also, if Ascii one should
> >     not be modified, why isn’t the return type CONST?
> Yes. will do that.
> 
> >
> > * RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
> >   + Change [Sources.common] to [Sources]
> done
> > * RedfishPkg/Library/CrtLib/CrtLib.inf
> >   + Does this lib instance really depend on MdeModulePkg?
> Yes, SortLib
> >   + Have you reviewed the module types this lib instance is
> >     compatible with?  Why does it need to support DXE_CORE?
> >     Are their JsonLib use cases for DXE_RUNTIME and SMM drivers?
> >     If there are SMM use cases, then why is MM excluded?
> Will just keep DXE_DRVER, UEFI_APPLICATION and UEFI_DRIVER. These are module types have potential use cases I can think of
> now.
> >
> > * RedfishPkg/Library/JsonLib
> >   + Readme.rst still has references to JanssonJsonLibMapping.h
> done
> >   + Have you reviewed the module types this lib instance is
> >     compatible with?  Why does it need to support DXE_CORE?
> >     Are their JsonLib use cases for DXE_RUNTIME and SMM drivers?
> >     If there are SMM use cases, then why is MM excluded?
> Answered above
> >   + JsonLib.inf has a [BuildOptions] section that disables some warnings that
> >     make me concerned that this library may have some issues with 32-bit
> > builds.
> >     Have you fully validated all 32-bit CPU builds and validated the functionality
> >     of all services from the JsonLib for all ranges of input values?
> Yes, IA32 build is validated. But not validating on the functionalities IA32. I don't have that use case, we can get back
> to fix the issue if any when  someone runs this on IA32.

I did some local testing with VS2019.  I found that a much smaller set of warning disables
are required for IA32 and X64 and the set is different for IA32 and X64.  Can you review
what is needed and minimize the required set of each supported arch?

  MSFT:*_*_X64_CC_FLAGS = /wd4244 /wd4090 /wd4334 /DHAVE_CONFIG_H=1 /U_WIN32 /UWIN64 /U_MSC_VER
  MSFT:*_*_IA32_CC_FLAGS = /wd4244 /wd4090 /DHAVE_CONFIG_H=1 /U_WIN32 /UWIN64 /U_MSC_VER

> >
> > * RedfishLibs.dsc.inc
> >   It is better to add [LibraryClasses] statement to this file, so it
> >   can be include anywhere in a DSC file.  With current implementation
> >   if it is not included within a [LibraryClasses] section, it will
> >   generate a build error.
> I will separate this to another set of patch. Don’t mix up with jansson edk2 port.
> 
> >You might also consider changing the name
> >   of this file in case you want to add more than just lib mappings
> >   to this file in the future.  See UnitTestFramworkPkg for examples.
> >
> > Best regards,
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Abner Chang <abner.chang@hpe.com>
> > > Sent: Thursday, December 17, 2020 5:19 AM
> > > To: devel@edk2.groups.io
> > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>; Laszlo
> > > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Kinney,
> > > Michael D <michael.d.kinney@intel.com>; Liming Gao
> > > <gaoliming@byosoft.com.cn>; Nickle Wang <nickle.wang@hpe.com>;
> > Peter
> > > O'Hanley <peter.ohanley@hpe.com>
> > > Subject: [PATCH v8 0/6] jansson edk2 port
> > >
> > > In v8, - Assigne patch file order
> > >        - Add Acked-by tags
> > > In v7, - Remove C RTC header files to under [Include.Common.Private]
> > >          in RedfishPkg.dec.
> > >        - address comments given by Mike Kinney.
> > > In v6, Remove JanssonJsonMapping.h
> > > In v5, move BaseUcs2Utf8Lib to under RedfishPkg.
> > > In v4,
> > >        - Address review comments
> > >        - Seperate CRT functions to a individule library CrtLib under
> > >          RedfishPkg.
> > >        - Seperate UCS2-UTF8 functions to a individule library
> > >          BaseUcs2Utf8Lib under MdeModulePkg.
> > >
> > > In v3, Add jansson library as the required submoudle in
> > >        CiSettings.py for CI test.
> > > In v2, JsonLib is moved to under RedfishPkg.
> > >
> > > edk2 JSON library is based on jansson open source
> > > (https://github.com/akheron/jansson) and wrapped as an edk2 library.
> > > edk2 JsonLib will be used by edk2 Redfish feature drivers (not
> > > contributed yet) and the edk2 port of libredfish library (not
> > > contributed yet) based on DMTF GitHub
> > > (https://github.com/DMTF/libredfish).
> > >
> > > Jansson is licensed under the MIT license(refer to ReadMe.rst under edk2).
> > > It is used in production and its API is stable. In UEFI/EDKII
> > > environment, Redfish project consumes jansson to achieve JSON
> > operations.
> > >
> > > * Jansson version on edk2: 2.13.1
> > >
> > > * EDKII jansson library wrapper:
> > >    - JsonLib.h:
> > >      This is the denifitions of EDKII JSON APIs which are mapped to
> > >      jannson funcitons accordingly.
> > >
> > >    - JanssonJsonLibMapping.h:
> > >      This is the wrapper file to map funcitons and definitions used in
> > >      native jannson applications to edk2 JsonLib. This avoids the
> > >      modifications on native jannson applications to be built under
> > >      edk2 environment.
> > >
> > > *Known issue:
> > >   Build fail with jansson/src/load.c, overrride and add code in load.c
> > >   to conditionally use stdin according to HAVE_UNISTD_H macro.
> > >   The PR is submitted to jansson open source community.
> > >   https://github.com/akheron/jansson/pull/558
> > >
> > > Signed-off-by: Abner Chang <abner.chang@hpe.com>
> > >
> > > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > > Cc: Andrew Fish <afish@apple.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > Cc: Leif Lindholm <leif@nuviainc.com>
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > Cc: Nickle Wang <nickle.wang@hpe.com>
> > > Cc: Peter O'Hanley <peter.ohanley@hpe.com>
> > >
> > > Abner Chang (6):
> > >   RedfishPkg/Ucs2Utf8lib: UCS2 to UFT8 manipulation library
> > >   edk2: jansson submodule for edk2 JSON library
> > >   RedfishPkg/CrtLib: C runtime library
> > >   RedfishPkg/library: EDK2 port of jansson library
> > >   RedfishPkg: Add EDK2 port of jansson library to build
> > >   .pytool: Add required submodule for JsonLib
> > >
> > >  .gitmodules                                   |    3 +
> > >  .pytool/CISettings.py                         |    2 +
> > >  ReadMe.rst                                    |    1 +
> > >  RedfishPkg/Include/Crt/assert.h               |   16 +
> > >  RedfishPkg/Include/Crt/errno.h                |   16 +
> > >  RedfishPkg/Include/Crt/limits.h               |   16 +
> > >  RedfishPkg/Include/Crt/math.h                 |   16 +
> > >  RedfishPkg/Include/Crt/stdarg.h               |   15 +
> > >  RedfishPkg/Include/Crt/stddef.h               |   16 +
> > >  RedfishPkg/Include/Crt/stdio.h                |   15 +
> > >  RedfishPkg/Include/Crt/stdlib.h               |   16 +
> > >  RedfishPkg/Include/Crt/string.h               |   16 +
> > >  RedfishPkg/Include/Crt/sys/time.h             |   15 +
> > >  RedfishPkg/Include/Crt/sys/types.h            |   15 +
> > >  RedfishPkg/Include/Crt/time.h                 |   15 +
> > >  RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h  |   61 +
> > >  RedfishPkg/Include/Library/CrtLib.h           |  191 +++
> > >  RedfishPkg/Include/Library/JsonLib.h          |  763 +++++++++++
> > >  .../Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c |  421 +++++++
> > >  .../BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf       |   31 +
> > >  RedfishPkg/Library/CrtLib/CrtLib.c            |  595 +++++++++
> > >  RedfishPkg/Library/CrtLib/CrtLib.inf          |   38 +
> > >  RedfishPkg/Library/JsonLib/JsonLib.c          |  964 ++++++++++++++
> > >  RedfishPkg/Library/JsonLib/JsonLib.inf        |   89 ++
> > >  RedfishPkg/Library/JsonLib/Readme.rst         |   40 +
> > >  RedfishPkg/Library/JsonLib/jansson            |    1 +
> > >  RedfishPkg/Library/JsonLib/jansson_config.h   |   41 +
> > >  .../Library/JsonLib/jansson_private_config.h  |   19 +
> > >  RedfishPkg/Library/JsonLib/load.c             | 1111 +++++++++++++++++
> > >  RedfishPkg/RedfishLibs.dsc.inc                |    3 +
> > >  RedfishPkg/RedfishPkg.ci.yaml                 |   25 +
> > >  RedfishPkg/RedfishPkg.dec                     |   25 +
> > >  RedfishPkg/RedfishPkg.dsc                     |    3 +
> > >  33 files changed, 4614 insertions(+)
> > >  create mode 100644 RedfishPkg/Include/Crt/assert.h  create mode
> > > 100644 RedfishPkg/Include/Crt/errno.h  create mode 100644
> > > RedfishPkg/Include/Crt/limits.h  create mode 100644
> > > RedfishPkg/Include/Crt/math.h  create mode 100644
> > > RedfishPkg/Include/Crt/stdarg.h  create mode 100644
> > > RedfishPkg/Include/Crt/stddef.h  create mode 100644
> > > RedfishPkg/Include/Crt/stdio.h  create mode 100644
> > > RedfishPkg/Include/Crt/stdlib.h  create mode 100644
> > > RedfishPkg/Include/Crt/string.h  create mode 100644
> > > RedfishPkg/Include/Crt/sys/time.h  create mode 100644
> > > RedfishPkg/Include/Crt/sys/types.h
> > >  create mode 100644 RedfishPkg/Include/Crt/time.h  create mode 100644
> > > RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h
> > >  create mode 100644 RedfishPkg/Include/Library/CrtLib.h
> > >  create mode 100644 RedfishPkg/Include/Library/JsonLib.h
> > >  create mode 100644
> > > RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c
> > >  create mode 100644
> > > RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
> > >  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.c
> > >  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.inf
> > >  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.c
> > >  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.inf
> > >  create mode 100644 RedfishPkg/Library/JsonLib/Readme.rst
> > >  create mode 160000 RedfishPkg/Library/JsonLib/jansson
> > >  create mode 100644 RedfishPkg/Library/JsonLib/jansson_config.h
> > >  create mode 100644
> > > RedfishPkg/Library/JsonLib/jansson_private_config.h
> > >  create mode 100644 RedfishPkg/Library/JsonLib/load.c
> > >
> > > --
> > > 2.17.1
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69385): https://edk2.groups.io/g/devel/message/69385
Mute This Topic: https://groups.io/mt/79052636/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> -----Original Message-----
> From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> Sent: Wednesday, December 23, 2020 2:14 AM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>; Laszlo
> Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> <nickle.wang@hpe.com>; O'Hanley, Peter (EXL) <peter.ohanley@hpe.com>
> Subject: RE: [PATCH v8 0/6] jansson edk2 port
> 
> Hi Abner,
> 
> A few comments below.
> 
> Best regards,
> 
> Mike
> 
> > -----Original Message-----
> > From: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>
> > Sent: Monday, December 21, 2020 12:17 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > devel@edk2.groups.io
> > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>; Laszlo
> > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming
> > Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley@hpe.com>
> > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> >
> > Mike, response in below. Also v9 is sent.
> >
> > Thanks for review this in detail.
> > Abner
> >
> > > -----Original Message-----
> > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> > > Sent: Monday, December 21, 2020 3:43 AM
> > > To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> > > devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> Laszlo
> > > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming
> > > Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley@hpe.com>
> > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > >
> > > Hi Abner,
> > >
> > > The include path in the [Include.Common.Private] section should not
> > > be in the same file path as the [Include] section.  This allows
> > > private include files to be accessed.
> > >
> > > Instead, you should create a new top level package directory called
> > > PrivateInclude and put all non-public include content in that
> > > directory.  The UnitTestFrameworkPkg is a good reference that
> > > demonstrates this design pattern.
> > Yes, this looks better. All Crt header files and CrtLib.h will be moved to
> under PrivateInclude.
> > >
> > > The library class name CrtLib is very generic and the library class
> > > is in the public includes so it is exported as a public interface from the
> RedfishPkg.
> > > This CrtLib implementation appears to be tuned to support the
> > > dependencies for the jansson submodule.  I recommend changing the
> > > library class name and the library instance name so it is clear that
> > > this CrtLib is only for jansson.
> > > Perhaps 'JanssonCrtLib'.
> > The CrtLib is not used by jannson lib, libredfish as well
> > (https://github.com/DMTF/libredfish). So JanssonCrtLib is not a good
> naming, will keep that as it.
> >
> > >Also, does this library class (CrtLib.h) need to be  exported from
> > >RedfishPkg as a public interface?
> > No, it will be moved to under PrivateInclude.
> >
> > If CrtLib.h is defined as a private library and CrtLib.h is in
> > PrivateInclude which is not exposed to other packages, then why do we
> care about the naming of CrtLib is too generic? Which is the private library
> under RedfishPkg.
> > Move those header files to under PrivateInclude seems clear to people
> > that CrtLib is only for RedfishPkg. I think we don’t have to change the
> naming in this case.
> 
> In general I agree, but there is one place where the lib mapping has wider
> scope, and that is in a platform DSC file.  The lib class name CrtLib has to be
> listed in the DSC file.  If more than one package defines a library class called
> CrtLib, then the last mapping used will be used for all modules if it is listed in
> the [LibraryClasses] section of the DSC file.  The only was to handle this type
> of name collision is to use a module scoped library mapping in the
> <LibraryClasses> section of each individual module that uses a CrtLib.  I am
> only trying to avoid the potential for future name collisions for a library
> class/instance that is specific to the JSON use case.
> 
> We recognize that several modules in edk2 have this type of gasket between
> an EDK II env and a standard C lib.  We may find a we to implement a more
> generic gasket and retire the use of the implementation specific ones.  This is
> another reason to use an implementation specific name in this use case and
> be able to introduce a more generic name in the future.
> 
> Another comment below to see if there is a way to avoid introducing a CrtLib
> class.
Responses all together in below,
> 
> >
> > >
> > > * RedfishPkg/Include/Library/CrtLib.h
> > >   + Remove reference to MDE_CPU_IA64.  This has been retired from
> > > the
> > > edk2 repo.
> > Ok.
> > >   + I do see one remaining reference to this in the CryptoPkg that need to
> be
> > >     removed.
> > >
> > > * RedfishPkg/Include/Library/JsonLib.h
> > >   + Some of the function descriptions are very brief and I can not tell
> > >     how to use the service from the description and more importantly, I
> > >     would not know how to write a unit test to verify the expected
> behavior.
> > >     Since these are a set of public APIs from this package that you
> > >     expect modules/libs from outside of RedfishPkg to use, then all of
> > >     these public APIs must be fully documented.
> > Most of the API in JsonLib are wrappers of native jansson APIs. We can
> > mention the URL to jansson API reference document in file header.
> > INVALID URI REMOVED
> 3A__jansson.readthedo
> >
> cs.io_en_2.13_index.html&d=DwIGaQ&c=C5b8zRQO1miGmBeVZ2LFWg&r=_
> SN6FZBN4
> > Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=vd-
> uPz864czoYA5C7YaPgqXfdPjv4c5kB
> > zTwV60NvNQ&s=acVzN3x4yND0jkg44bJ48ZkyPyugjwsYvUGdD4nJL74&e=
> > I will review all function header again,  please check v9 patch later.
> >
> > >   + Are there any of these APIs that are not really needed by modules/libs
> > >     outside the RedfishPkg?  It would be better to remove APIs that are
> > >     not needed outside RedfishPkg from this public include file.
> > >   + A couple examples that stood out are:
> > >     JsonDecreaseReference()
> > >     JsonIncreaseReference()
> > >     JsonObjectIterator()
> > >     JsonObjectIteratorValue()
> > >     JsonObjectIteratorNext()
> > The native jansson APIs of all above wrappers are used in libredfish.
> > So that is the potential use case for other JSON applications.
> 
> Can you provide a pointer to an example of this use case?
Again, those are used in open source project published by DMTF to access to Redfish service. https://github.com/DMTF/libredfish
Not sure if you can access to it, I just created an a branch of RedfishLib for your convenience. Take a look at the last commit on below link if you can't access to libredfish on DMTF GitHub.
https://github.com/changab/edk2/tree/RedfishLib
you can search above APIs (the native ones) in payload.c and service.c.

> 
> I am trying to make sure we are doing the right design of RedfishPkg with
> respect to CrtLib for this use case.  Are these are JSON apps that do not use
> JsonLib, but are expected to build in an EDK II build environment? 
No, libredfish uses native jansson APIs.

>Would
> these JSON apps require more standard C lib services than the CrtLib used to
> build JsonLib?  Where would those additional C lib services come from?
CrtLib is created for both jansson and libredfish. C services are ready in CrtLib for both open source projects.
> 
> There is a standard C lib in edk2-libc.  Would that be required to build the
> JSON apps
No requirements for edk2-libc, at least for jansson and libredfish. And above two open source projects are what we need for Redfish.
Any other edk2 based JSON apps/drivers should use JsonLib.
> 
> In other uses of submodules with C lib dependencies, a new CrtLib like lib
> class was not added and instead the support was added directly to the EDK II
> wrapper APIs. Some examples are:
>   * MdeModulePkg\Library\BrotliCustomDecompressLib
>   * MdeModulePkg\Universal\RegularExpressionDxe
We have two libraries under RedfishPkg require CRT library. To have one instance for both makes sense to me.
> 
> If there are use cases where the CrtLib will be added to an INF outside the
> RedfishPkg, then I still think the name needs to be changed to be specific to
> the JSON use case.
No, CrtLib shouldn’t be published to outside RedfishPkg. That is only used for those two open source projects.

> If the use of CrtLib is only within the RedfishPkg, then the Library Class can be
> declared in a [LibraryClasses.Common.Private] section of the DEC file.  See
> UnitTestFrameworkPkg for examples.
I like this way. Will adopt this method and send v10 for review.

> 
> >
> > >   + JsonDumpString()
> > >     func header params do not match func prototype
> > >     Does not describe who allocates the return buffer and who
> > >     is responsible for freeing the buffer returned.
> > >     What do Flags do?  What is the difference between this
> > >     function and JsonToText()?
> > In JsonToText, it only converts JSON array and object to text. We
> > could just remove this for now because I don’t really remember the reason
> to create this function, it has been a while. Seems to me it is fine to remove
> this function.
> >
> > >   + JsonLoadBuffer() - Error param description missing
> > >   + JsonArrayGetValue() does not describe the conditions NULL
> > >     is returned.
> > >   + JsonObjectGetKeys().  Return type is CHAR8**.  What
> > >     function need to be called to free the array?
> > All above addressed.
> >
> > >   + JsonValueGetAsciiString() and JsonValueGetUnicodeString()
> > >     are asymmetric.  The Ascii one states that change will
> > >     modify the original string.  The Unicode one says that the
> > >     caller needs to free the returned string.  Any reason we
> > >     can not make them symmetric?
> > Jansson doesn't support Unicode string. The memory of the unicode
> > string returned by JsonValueGetUnicodeString Is allocated by
> UTF8StrToUCS2().
> >  > Also, if Ascii one should
> > >     not be modified, why isn’t the return type CONST?
> > Yes. will do that.
> >
> > >
> > > * RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
> > >   + Change [Sources.common] to [Sources]
> > done
> > > * RedfishPkg/Library/CrtLib/CrtLib.inf
> > >   + Does this lib instance really depend on MdeModulePkg?
> > Yes, SortLib
> > >   + Have you reviewed the module types this lib instance is
> > >     compatible with?  Why does it need to support DXE_CORE?
> > >     Are their JsonLib use cases for DXE_RUNTIME and SMM drivers?
> > >     If there are SMM use cases, then why is MM excluded?
> > Will just keep DXE_DRVER, UEFI_APPLICATION and UEFI_DRIVER. These
> are
> > module types have potential use cases I can think of now.
> > >
> > > * RedfishPkg/Library/JsonLib
> > >   + Readme.rst still has references to JanssonJsonLibMapping.h
> > done
> > >   + Have you reviewed the module types this lib instance is
> > >     compatible with?  Why does it need to support DXE_CORE?
> > >     Are their JsonLib use cases for DXE_RUNTIME and SMM drivers?
> > >     If there are SMM use cases, then why is MM excluded?
> > Answered above
> > >   + JsonLib.inf has a [BuildOptions] section that disables some warnings
> that
> > >     make me concerned that this library may have some issues with
> > > 32-bit builds.
> > >     Have you fully validated all 32-bit CPU builds and validated the
> functionality
> > >     of all services from the JsonLib for all ranges of input values?
> > Yes, IA32 build is validated. But not validating on the
> > functionalities IA32. I don't have that use case, we can get back to fix the
> issue if any when  someone runs this on IA32.
> 
> I did some local testing with VS2019.  I found that a much smaller set of
> warning disables are required for IA32 and X64 and the set is different for
> IA32 and X64.  Can you review what is needed and minimize the required set
> of each supported arch?
> 
>   MSFT:*_*_X64_CC_FLAGS = /wd4244 /wd4090 /wd4334
> /DHAVE_CONFIG_H=1 /U_WIN32 /UWIN64 /U_MSC_VER
>   MSFT:*_*_IA32_CC_FLAGS = /wd4244 /wd4090 /DHAVE_CONFIG_H=1
> /U_WIN32 /UWIN64 /U_MSC_VER
> 
> > >
> > > * RedfishLibs.dsc.inc
> > >   It is better to add [LibraryClasses] statement to this file, so it
> > >   can be include anywhere in a DSC file.  With current implementation
> > >   if it is not included within a [LibraryClasses] section, it will
> > >   generate a build error.
> > I will separate this to another set of patch. Don’t mix up with jansson edk2
> port.
> >
> > >You might also consider changing the name
> > >   of this file in case you want to add more than just lib mappings
> > >   to this file in the future.  See UnitTestFramworkPkg for examples.
> > >
> > > Best regards,
> > >
> > > Mike
> > >
> > > > -----Original Message-----
> > > > From: Abner Chang <abner.chang@hpe.com>
> > > > Sent: Thursday, December 17, 2020 5:19 AM
> > > > To: devel@edk2.groups.io
> > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> > > > Laszlo Ersek <lersek@redhat.com>; Leif Lindholm
> > > > <leif@nuviainc.com>; Kinney, Michael D
> > > > <michael.d.kinney@intel.com>; Liming Gao
> > > > <gaoliming@byosoft.com.cn>; Nickle Wang <nickle.wang@hpe.com>;
> > > Peter
> > > > O'Hanley <peter.ohanley@hpe.com>
> > > > Subject: [PATCH v8 0/6] jansson edk2 port
> > > >
> > > > In v8, - Assigne patch file order
> > > >        - Add Acked-by tags
> > > > In v7, - Remove C RTC header files to under [Include.Common.Private]
> > > >          in RedfishPkg.dec.
> > > >        - address comments given by Mike Kinney.
> > > > In v6, Remove JanssonJsonMapping.h In v5, move BaseUcs2Utf8Lib to
> > > > under RedfishPkg.
> > > > In v4,
> > > >        - Address review comments
> > > >        - Seperate CRT functions to a individule library CrtLib under
> > > >          RedfishPkg.
> > > >        - Seperate UCS2-UTF8 functions to a individule library
> > > >          BaseUcs2Utf8Lib under MdeModulePkg.
> > > >
> > > > In v3, Add jansson library as the required submoudle in
> > > >        CiSettings.py for CI test.
> > > > In v2, JsonLib is moved to under RedfishPkg.
> > > >
> > > > edk2 JSON library is based on jansson open source
> > > > (https://github.com/akheron/jansson) and wrapped as an edk2 library.
> > > > edk2 JsonLib will be used by edk2 Redfish feature drivers (not
> > > > contributed yet) and the edk2 port of libredfish library (not
> > > > contributed yet) based on DMTF GitHub
> > > > (https://github.com/DMTF/libredfish).
> > > >
> > > > Jansson is licensed under the MIT license(refer to ReadMe.rst under
> edk2).
> > > > It is used in production and its API is stable. In UEFI/EDKII
> > > > environment, Redfish project consumes jansson to achieve JSON
> > > operations.
> > > >
> > > > * Jansson version on edk2: 2.13.1
> > > >
> > > > * EDKII jansson library wrapper:
> > > >    - JsonLib.h:
> > > >      This is the denifitions of EDKII JSON APIs which are mapped to
> > > >      jannson funcitons accordingly.
> > > >
> > > >    - JanssonJsonLibMapping.h:
> > > >      This is the wrapper file to map funcitons and definitions used in
> > > >      native jannson applications to edk2 JsonLib. This avoids the
> > > >      modifications on native jannson applications to be built under
> > > >      edk2 environment.
> > > >
> > > > *Known issue:
> > > >   Build fail with jansson/src/load.c, overrride and add code in load.c
> > > >   to conditionally use stdin according to HAVE_UNISTD_H macro.
> > > >   The PR is submitted to jansson open source community.
> > > >   https://github.com/akheron/jansson/pull/558
> > > >
> > > > Signed-off-by: Abner Chang <abner.chang@hpe.com>
> > > >
> > > > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > > > Cc: Andrew Fish <afish@apple.com>
> > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > Cc: Leif Lindholm <leif@nuviainc.com>
> > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > > Cc: Nickle Wang <nickle.wang@hpe.com>
> > > > Cc: Peter O'Hanley <peter.ohanley@hpe.com>
> > > >
> > > > Abner Chang (6):
> > > >   RedfishPkg/Ucs2Utf8lib: UCS2 to UFT8 manipulation library
> > > >   edk2: jansson submodule for edk2 JSON library
> > > >   RedfishPkg/CrtLib: C runtime library
> > > >   RedfishPkg/library: EDK2 port of jansson library
> > > >   RedfishPkg: Add EDK2 port of jansson library to build
> > > >   .pytool: Add required submodule for JsonLib
> > > >
> > > >  .gitmodules                                   |    3 +
> > > >  .pytool/CISettings.py                         |    2 +
> > > >  ReadMe.rst                                    |    1 +
> > > >  RedfishPkg/Include/Crt/assert.h               |   16 +
> > > >  RedfishPkg/Include/Crt/errno.h                |   16 +
> > > >  RedfishPkg/Include/Crt/limits.h               |   16 +
> > > >  RedfishPkg/Include/Crt/math.h                 |   16 +
> > > >  RedfishPkg/Include/Crt/stdarg.h               |   15 +
> > > >  RedfishPkg/Include/Crt/stddef.h               |   16 +
> > > >  RedfishPkg/Include/Crt/stdio.h                |   15 +
> > > >  RedfishPkg/Include/Crt/stdlib.h               |   16 +
> > > >  RedfishPkg/Include/Crt/string.h               |   16 +
> > > >  RedfishPkg/Include/Crt/sys/time.h             |   15 +
> > > >  RedfishPkg/Include/Crt/sys/types.h            |   15 +
> > > >  RedfishPkg/Include/Crt/time.h                 |   15 +
> > > >  RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h  |   61 +
> > > >  RedfishPkg/Include/Library/CrtLib.h           |  191 +++
> > > >  RedfishPkg/Include/Library/JsonLib.h          |  763 +++++++++++
> > > >  .../Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c |  421 +++++++
> > > >  .../BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf       |   31 +
> > > >  RedfishPkg/Library/CrtLib/CrtLib.c            |  595 +++++++++
> > > >  RedfishPkg/Library/CrtLib/CrtLib.inf          |   38 +
> > > >  RedfishPkg/Library/JsonLib/JsonLib.c          |  964 ++++++++++++++
> > > >  RedfishPkg/Library/JsonLib/JsonLib.inf        |   89 ++
> > > >  RedfishPkg/Library/JsonLib/Readme.rst         |   40 +
> > > >  RedfishPkg/Library/JsonLib/jansson            |    1 +
> > > >  RedfishPkg/Library/JsonLib/jansson_config.h   |   41 +
> > > >  .../Library/JsonLib/jansson_private_config.h  |   19 +
> > > >  RedfishPkg/Library/JsonLib/load.c             | 1111 +++++++++++++++++
> > > >  RedfishPkg/RedfishLibs.dsc.inc                |    3 +
> > > >  RedfishPkg/RedfishPkg.ci.yaml                 |   25 +
> > > >  RedfishPkg/RedfishPkg.dec                     |   25 +
> > > >  RedfishPkg/RedfishPkg.dsc                     |    3 +
> > > >  33 files changed, 4614 insertions(+)  create mode 100644
> > > > RedfishPkg/Include/Crt/assert.h  create mode
> > > > 100644 RedfishPkg/Include/Crt/errno.h  create mode 100644
> > > > RedfishPkg/Include/Crt/limits.h  create mode 100644
> > > > RedfishPkg/Include/Crt/math.h  create mode 100644
> > > > RedfishPkg/Include/Crt/stdarg.h  create mode 100644
> > > > RedfishPkg/Include/Crt/stddef.h  create mode 100644
> > > > RedfishPkg/Include/Crt/stdio.h  create mode 100644
> > > > RedfishPkg/Include/Crt/stdlib.h  create mode 100644
> > > > RedfishPkg/Include/Crt/string.h  create mode 100644
> > > > RedfishPkg/Include/Crt/sys/time.h  create mode 100644
> > > > RedfishPkg/Include/Crt/sys/types.h
> > > >  create mode 100644 RedfishPkg/Include/Crt/time.h  create mode
> > > > 100644 RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h
> > > >  create mode 100644 RedfishPkg/Include/Library/CrtLib.h
> > > >  create mode 100644 RedfishPkg/Include/Library/JsonLib.h
> > > >  create mode 100644
> > > > RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c
> > > >  create mode 100644
> > > > RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
> > > >  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.c
> > > >  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.inf
> > > >  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.c
> > > >  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.inf
> > > >  create mode 100644 RedfishPkg/Library/JsonLib/Readme.rst
> > > >  create mode 160000 RedfishPkg/Library/JsonLib/jansson
> > > >  create mode 100644 RedfishPkg/Library/JsonLib/jansson_config.h
> > > >  create mode 100644
> > > > RedfishPkg/Library/JsonLib/jansson_private_config.h
> > > >  create mode 100644 RedfishPkg/Library/JsonLib/load.c
> > > >
> > > > --
> > > > 2.17.1
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69394): https://edk2.groups.io/g/devel/message/69394
Mute This Topic: https://groups.io/mt/79052636/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> -----Original Message-----
> From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> Sent: Wednesday, December 23, 2020 2:14 AM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>; Laszlo
> Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> <nickle.wang@hpe.com>; O'Hanley, Peter (EXL) <peter.ohanley@hpe.com>
> Subject: RE: [PATCH v8 0/6] jansson edk2 port
> 
> Hi Abner,
> 
> A few comments below.
> 
> Best regards,
> 
> Mike
> 
> > -----Original Message-----
> > From: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>
> > Sent: Monday, December 21, 2020 12:17 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > devel@edk2.groups.io
> > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>; Laszlo
> > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming
> > Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley@hpe.com>
> > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> >
> > Mike, response in below. Also v9 is sent.
> >
> > Thanks for review this in detail.
> > Abner
> >
> > > -----Original Message-----
> > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> > > Sent: Monday, December 21, 2020 3:43 AM
> > > To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> > > devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> Laszlo
> > > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming
> > > Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley@hpe.com>
> > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > >
> > > Hi Abner,
> > >
> > > The include path in the [Include.Common.Private] section should not
> > > be in the same file path as the [Include] section.  This allows
> > > private include files to be accessed.
> > >
> > > Instead, you should create a new top level package directory called
> > > PrivateInclude and put all non-public include content in that
> > > directory.  The UnitTestFrameworkPkg is a good reference that
> > > demonstrates this design pattern.
> > Yes, this looks better. All Crt header files and CrtLib.h will be moved to
> under PrivateInclude.
> > >
> > > The library class name CrtLib is very generic and the library class
> > > is in the public includes so it is exported as a public interface from the
> RedfishPkg.
> > > This CrtLib implementation appears to be tuned to support the
> > > dependencies for the jansson submodule.  I recommend changing the
> > > library class name and the library instance name so it is clear that
> > > this CrtLib is only for jansson.
> > > Perhaps 'JanssonCrtLib'.
> > The CrtLib is not used by jannson lib, libredfish as well
> > (https://github.com/DMTF/libredfish). So JanssonCrtLib is not a good
> naming, will keep that as it.
> >
> > >Also, does this library class (CrtLib.h) need to be  exported from
> > >RedfishPkg as a public interface?
> > No, it will be moved to under PrivateInclude.
> >
> > If CrtLib.h is defined as a private library and CrtLib.h is in
> > PrivateInclude which is not exposed to other packages, then why do we
> care about the naming of CrtLib is too generic? Which is the private library
> under RedfishPkg.
> > Move those header files to under PrivateInclude seems clear to people
> > that CrtLib is only for RedfishPkg. I think we don’t have to change the
> naming in this case.
> 
> In general I agree, but there is one place where the lib mapping has wider
> scope, and that is in a platform DSC file.  The lib class name CrtLib has to be
> listed in the DSC file.  If more than one package defines a library class called
> CrtLib, then the last mapping used will be used for all modules if it is listed in
> the [LibraryClasses] section of the DSC file.  The only was to handle this type
> of name collision is to use a module scoped library mapping in the
> <LibraryClasses> section of each individual module that uses a CrtLib.  I am
> only trying to avoid the potential for future name collisions for a library
> class/instance that is specific to the JSON use case.
> 
> We recognize that several modules in edk2 have this type of gasket between
> an EDK II env and a standard C lib.  We may find a we to implement a more
> generic gasket and retire the use of the implementation specific ones.  This is
> another reason to use an implementation specific name in this use case and
> be able to introduce a more generic name in the future.
> 
> Another comment below to see if there is a way to avoid introducing a CrtLib
> class.
> 
> >
> > >
> > > * RedfishPkg/Include/Library/CrtLib.h
> > >   + Remove reference to MDE_CPU_IA64.  This has been retired from
> > > the
> > > edk2 repo.
> > Ok.
> > >   + I do see one remaining reference to this in the CryptoPkg that need to
> be
> > >     removed.
> > >
> > > * RedfishPkg/Include/Library/JsonLib.h
> > >   + Some of the function descriptions are very brief and I can not tell
> > >     how to use the service from the description and more importantly, I
> > >     would not know how to write a unit test to verify the expected
> behavior.
> > >     Since these are a set of public APIs from this package that you
> > >     expect modules/libs from outside of RedfishPkg to use, then all of
> > >     these public APIs must be fully documented.
> > Most of the API in JsonLib are wrappers of native jansson APIs. We can
> > mention the URL to jansson API reference document in file header.
> > INVALID URI REMOVED
> 3A__jansson.readthedo
> >
> cs.io_en_2.13_index.html&d=DwIGaQ&c=C5b8zRQO1miGmBeVZ2LFWg&r=_
> SN6FZBN4
> > Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=vd-
> uPz864czoYA5C7YaPgqXfdPjv4c5kB
> > zTwV60NvNQ&s=acVzN3x4yND0jkg44bJ48ZkyPyugjwsYvUGdD4nJL74&e=
> > I will review all function header again,  please check v9 patch later.
> >
> > >   + Are there any of these APIs that are not really needed by modules/libs
> > >     outside the RedfishPkg?  It would be better to remove APIs that are
> > >     not needed outside RedfishPkg from this public include file.
> > >   + A couple examples that stood out are:
> > >     JsonDecreaseReference()
> > >     JsonIncreaseReference()
> > >     JsonObjectIterator()
> > >     JsonObjectIteratorValue()
> > >     JsonObjectIteratorNext()
> > The native jansson APIs of all above wrappers are used in libredfish.
> > So that is the potential use case for other JSON applications.
> 
> Can you provide a pointer to an example of this use case?
> 
> I am trying to make sure we are doing the right design of RedfishPkg with
> respect to CrtLib for this use case.  Are these are JSON apps that do not use
> JsonLib, but are expected to build in an EDK II build environment?  Would
> these JSON apps require more standard C lib services than the CrtLib used to
> build JsonLib?  Where would those additional C lib services come from?
> 
> There is a standard C lib in edk2-libc.  Would that be required to build the
> JSON apps.
> 
> In other uses of submodules with C lib dependencies, a new CrtLib like lib
> class was not added and instead the support was added directly to the EDK II
> wrapper APIs. Some examples are:
>   * MdeModulePkg\Library\BrotliCustomDecompressLib
>   * MdeModulePkg\Universal\RegularExpressionDxe
> 
> If there are use cases where the CrtLib will be added to an INF outside the
> RedfishPkg, then I still think the name needs to be changed to be specific to
> the JSON use case.
> 
> If the use of CrtLib is only within the RedfishPkg, then the Library Class can be
> declared in a [LibraryClasses.Common.Private] section of the DEC file.  See
> UnitTestFrameworkPkg for examples.
> 
> >
> > >   + JsonDumpString()
> > >     func header params do not match func prototype
> > >     Does not describe who allocates the return buffer and who
> > >     is responsible for freeing the buffer returned.
> > >     What do Flags do?  What is the difference between this
> > >     function and JsonToText()?
> > In JsonToText, it only converts JSON array and object to text. We
> > could just remove this for now because I don’t really remember the reason
> to create this function, it has been a while. Seems to me it is fine to remove
> this function.
> >
> > >   + JsonLoadBuffer() - Error param description missing
> > >   + JsonArrayGetValue() does not describe the conditions NULL
> > >     is returned.
> > >   + JsonObjectGetKeys().  Return type is CHAR8**.  What
> > >     function need to be called to free the array?
> > All above addressed.
> >
> > >   + JsonValueGetAsciiString() and JsonValueGetUnicodeString()
> > >     are asymmetric.  The Ascii one states that change will
> > >     modify the original string.  The Unicode one says that the
> > >     caller needs to free the returned string.  Any reason we
> > >     can not make them symmetric?
> > Jansson doesn't support Unicode string. The memory of the unicode
> > string returned by JsonValueGetUnicodeString Is allocated by
> UTF8StrToUCS2().
> >  > Also, if Ascii one should
> > >     not be modified, why isn’t the return type CONST?
> > Yes. will do that.
> >
> > >
> > > * RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
> > >   + Change [Sources.common] to [Sources]
> > done
> > > * RedfishPkg/Library/CrtLib/CrtLib.inf
> > >   + Does this lib instance really depend on MdeModulePkg?
> > Yes, SortLib
> > >   + Have you reviewed the module types this lib instance is
> > >     compatible with?  Why does it need to support DXE_CORE?
> > >     Are their JsonLib use cases for DXE_RUNTIME and SMM drivers?
> > >     If there are SMM use cases, then why is MM excluded?
> > Will just keep DXE_DRVER, UEFI_APPLICATION and UEFI_DRIVER. These
> are
> > module types have potential use cases I can think of now.
> > >
> > > * RedfishPkg/Library/JsonLib
> > >   + Readme.rst still has references to JanssonJsonLibMapping.h
> > done
> > >   + Have you reviewed the module types this lib instance is
> > >     compatible with?  Why does it need to support DXE_CORE?
> > >     Are their JsonLib use cases for DXE_RUNTIME and SMM drivers?
> > >     If there are SMM use cases, then why is MM excluded?
> > Answered above
> > >   + JsonLib.inf has a [BuildOptions] section that disables some warnings
> that
> > >     make me concerned that this library may have some issues with
> > > 32-bit builds.
> > >     Have you fully validated all 32-bit CPU builds and validated the
> functionality
> > >     of all services from the JsonLib for all ranges of input values?
> > Yes, IA32 build is validated. But not validating on the
> > functionalities IA32. I don't have that use case, we can get back to fix the
> issue if any when  someone runs this on IA32.
> 
> I did some local testing with VS2019.  I found that a much smaller set of
> warning disables are required for IA32 and X64 and the set is different for
> IA32 and X64.  Can you review what is needed and minimize the required set
> of each supported arch?
> 
>   MSFT:*_*_X64_CC_FLAGS = /wd4244 /wd4090 /wd4334
> /DHAVE_CONFIG_H=1 /U_WIN32 /UWIN64 /U_MSC_VER
>   MSFT:*_*_IA32_CC_FLAGS = /wd4244 /wd4090 /DHAVE_CONFIG_H=1
> /U_WIN32 /UWIN64 /U_MSC_VER
Ok
Abner
> 
> > >
> > > * RedfishLibs.dsc.inc
> > >   It is better to add [LibraryClasses] statement to this file, so it
> > >   can be include anywhere in a DSC file.  With current implementation
> > >   if it is not included within a [LibraryClasses] section, it will
> > >   generate a build error.
> > I will separate this to another set of patch. Don’t mix up with jansson edk2
> port.
> >
> > >You might also consider changing the name
> > >   of this file in case you want to add more than just lib mappings
> > >   to this file in the future.  See UnitTestFramworkPkg for examples.
> > >
> > > Best regards,
> > >
> > > Mike
> > >
> > > > -----Original Message-----
> > > > From: Abner Chang <abner.chang@hpe.com>
> > > > Sent: Thursday, December 17, 2020 5:19 AM
> > > > To: devel@edk2.groups.io
> > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> > > > Laszlo Ersek <lersek@redhat.com>; Leif Lindholm
> > > > <leif@nuviainc.com>; Kinney, Michael D
> > > > <michael.d.kinney@intel.com>; Liming Gao
> > > > <gaoliming@byosoft.com.cn>; Nickle Wang <nickle.wang@hpe.com>;
> > > Peter
> > > > O'Hanley <peter.ohanley@hpe.com>
> > > > Subject: [PATCH v8 0/6] jansson edk2 port
> > > >
> > > > In v8, - Assigne patch file order
> > > >        - Add Acked-by tags
> > > > In v7, - Remove C RTC header files to under [Include.Common.Private]
> > > >          in RedfishPkg.dec.
> > > >        - address comments given by Mike Kinney.
> > > > In v6, Remove JanssonJsonMapping.h In v5, move BaseUcs2Utf8Lib to
> > > > under RedfishPkg.
> > > > In v4,
> > > >        - Address review comments
> > > >        - Seperate CRT functions to a individule library CrtLib under
> > > >          RedfishPkg.
> > > >        - Seperate UCS2-UTF8 functions to a individule library
> > > >          BaseUcs2Utf8Lib under MdeModulePkg.
> > > >
> > > > In v3, Add jansson library as the required submoudle in
> > > >        CiSettings.py for CI test.
> > > > In v2, JsonLib is moved to under RedfishPkg.
> > > >
> > > > edk2 JSON library is based on jansson open source
> > > > (https://github.com/akheron/jansson) and wrapped as an edk2 library.
> > > > edk2 JsonLib will be used by edk2 Redfish feature drivers (not
> > > > contributed yet) and the edk2 port of libredfish library (not
> > > > contributed yet) based on DMTF GitHub
> > > > (https://github.com/DMTF/libredfish).
> > > >
> > > > Jansson is licensed under the MIT license(refer to ReadMe.rst under
> edk2).
> > > > It is used in production and its API is stable. In UEFI/EDKII
> > > > environment, Redfish project consumes jansson to achieve JSON
> > > operations.
> > > >
> > > > * Jansson version on edk2: 2.13.1
> > > >
> > > > * EDKII jansson library wrapper:
> > > >    - JsonLib.h:
> > > >      This is the denifitions of EDKII JSON APIs which are mapped to
> > > >      jannson funcitons accordingly.
> > > >
> > > >    - JanssonJsonLibMapping.h:
> > > >      This is the wrapper file to map funcitons and definitions used in
> > > >      native jannson applications to edk2 JsonLib. This avoids the
> > > >      modifications on native jannson applications to be built under
> > > >      edk2 environment.
> > > >
> > > > *Known issue:
> > > >   Build fail with jansson/src/load.c, overrride and add code in load.c
> > > >   to conditionally use stdin according to HAVE_UNISTD_H macro.
> > > >   The PR is submitted to jansson open source community.
> > > >   https://github.com/akheron/jansson/pull/558
> > > >
> > > > Signed-off-by: Abner Chang <abner.chang@hpe.com>
> > > >
> > > > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > > > Cc: Andrew Fish <afish@apple.com>
> > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > Cc: Leif Lindholm <leif@nuviainc.com>
> > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > > Cc: Nickle Wang <nickle.wang@hpe.com>
> > > > Cc: Peter O'Hanley <peter.ohanley@hpe.com>
> > > >
> > > > Abner Chang (6):
> > > >   RedfishPkg/Ucs2Utf8lib: UCS2 to UFT8 manipulation library
> > > >   edk2: jansson submodule for edk2 JSON library
> > > >   RedfishPkg/CrtLib: C runtime library
> > > >   RedfishPkg/library: EDK2 port of jansson library
> > > >   RedfishPkg: Add EDK2 port of jansson library to build
> > > >   .pytool: Add required submodule for JsonLib
> > > >
> > > >  .gitmodules                                   |    3 +
> > > >  .pytool/CISettings.py                         |    2 +
> > > >  ReadMe.rst                                    |    1 +
> > > >  RedfishPkg/Include/Crt/assert.h               |   16 +
> > > >  RedfishPkg/Include/Crt/errno.h                |   16 +
> > > >  RedfishPkg/Include/Crt/limits.h               |   16 +
> > > >  RedfishPkg/Include/Crt/math.h                 |   16 +
> > > >  RedfishPkg/Include/Crt/stdarg.h               |   15 +
> > > >  RedfishPkg/Include/Crt/stddef.h               |   16 +
> > > >  RedfishPkg/Include/Crt/stdio.h                |   15 +
> > > >  RedfishPkg/Include/Crt/stdlib.h               |   16 +
> > > >  RedfishPkg/Include/Crt/string.h               |   16 +
> > > >  RedfishPkg/Include/Crt/sys/time.h             |   15 +
> > > >  RedfishPkg/Include/Crt/sys/types.h            |   15 +
> > > >  RedfishPkg/Include/Crt/time.h                 |   15 +
> > > >  RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h  |   61 +
> > > >  RedfishPkg/Include/Library/CrtLib.h           |  191 +++
> > > >  RedfishPkg/Include/Library/JsonLib.h          |  763 +++++++++++
> > > >  .../Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c |  421 +++++++
> > > >  .../BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf       |   31 +
> > > >  RedfishPkg/Library/CrtLib/CrtLib.c            |  595 +++++++++
> > > >  RedfishPkg/Library/CrtLib/CrtLib.inf          |   38 +
> > > >  RedfishPkg/Library/JsonLib/JsonLib.c          |  964 ++++++++++++++
> > > >  RedfishPkg/Library/JsonLib/JsonLib.inf        |   89 ++
> > > >  RedfishPkg/Library/JsonLib/Readme.rst         |   40 +
> > > >  RedfishPkg/Library/JsonLib/jansson            |    1 +
> > > >  RedfishPkg/Library/JsonLib/jansson_config.h   |   41 +
> > > >  .../Library/JsonLib/jansson_private_config.h  |   19 +
> > > >  RedfishPkg/Library/JsonLib/load.c             | 1111 +++++++++++++++++
> > > >  RedfishPkg/RedfishLibs.dsc.inc                |    3 +
> > > >  RedfishPkg/RedfishPkg.ci.yaml                 |   25 +
> > > >  RedfishPkg/RedfishPkg.dec                     |   25 +
> > > >  RedfishPkg/RedfishPkg.dsc                     |    3 +
> > > >  33 files changed, 4614 insertions(+)  create mode 100644
> > > > RedfishPkg/Include/Crt/assert.h  create mode
> > > > 100644 RedfishPkg/Include/Crt/errno.h  create mode 100644
> > > > RedfishPkg/Include/Crt/limits.h  create mode 100644
> > > > RedfishPkg/Include/Crt/math.h  create mode 100644
> > > > RedfishPkg/Include/Crt/stdarg.h  create mode 100644
> > > > RedfishPkg/Include/Crt/stddef.h  create mode 100644
> > > > RedfishPkg/Include/Crt/stdio.h  create mode 100644
> > > > RedfishPkg/Include/Crt/stdlib.h  create mode 100644
> > > > RedfishPkg/Include/Crt/string.h  create mode 100644
> > > > RedfishPkg/Include/Crt/sys/time.h  create mode 100644
> > > > RedfishPkg/Include/Crt/sys/types.h
> > > >  create mode 100644 RedfishPkg/Include/Crt/time.h  create mode
> > > > 100644 RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h
> > > >  create mode 100644 RedfishPkg/Include/Library/CrtLib.h
> > > >  create mode 100644 RedfishPkg/Include/Library/JsonLib.h
> > > >  create mode 100644
> > > > RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c
> > > >  create mode 100644
> > > > RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
> > > >  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.c
> > > >  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.inf
> > > >  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.c
> > > >  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.inf
> > > >  create mode 100644 RedfishPkg/Library/JsonLib/Readme.rst
> > > >  create mode 160000 RedfishPkg/Library/JsonLib/jansson
> > > >  create mode 100644 RedfishPkg/Library/JsonLib/jansson_config.h
> > > >  create mode 100644
> > > > RedfishPkg/Library/JsonLib/jansson_private_config.h
> > > >  create mode 100644 RedfishPkg/Library/JsonLib/load.c
> > > >
> > > > --
> > > > 2.17.1
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69395): https://edk2.groups.io/g/devel/message/69395
Mute This Topic: https://groups.io/mt/79052636/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> -----Original Message-----
> From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> Sent: Wednesday, December 23, 2020 2:14 AM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>; Laszlo
> Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> <nickle.wang@hpe.com>; O'Hanley, Peter (EXL) <peter.ohanley@hpe.com>
> Subject: RE: [PATCH v8 0/6] jansson edk2 port
> 
> Hi Abner,
> 
> A few comments below.
> 
> Best regards,
> 
> Mike
> 
> > -----Original Message-----
> > From: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>
> > Sent: Monday, December 21, 2020 12:17 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > devel@edk2.groups.io
> > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>; Laszlo
> > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming
> > Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley@hpe.com>
> > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> >
> > Mike, response in below. Also v9 is sent.
> >
> > Thanks for review this in detail.
> > Abner
> >
> > > -----Original Message-----
> > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> > > Sent: Monday, December 21, 2020 3:43 AM
> > > To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> > > devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> Laszlo
> > > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming
> > > Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley@hpe.com>
> > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > >
> > > Hi Abner,
> > >
> > > The include path in the [Include.Common.Private] section should not
> > > be in the same file path as the [Include] section.  This allows
> > > private include files to be accessed.
> > >
> > > Instead, you should create a new top level package directory called
> > > PrivateInclude and put all non-public include content in that
> > > directory.  The UnitTestFrameworkPkg is a good reference that
> > > demonstrates this design pattern.
> > Yes, this looks better. All Crt header files and CrtLib.h will be moved to
> under PrivateInclude.
> > >
> > > The library class name CrtLib is very generic and the library class
> > > is in the public includes so it is exported as a public interface from the
> RedfishPkg.
> > > This CrtLib implementation appears to be tuned to support the
> > > dependencies for the jansson submodule.  I recommend changing the
> > > library class name and the library instance name so it is clear that
> > > this CrtLib is only for jansson.
> > > Perhaps 'JanssonCrtLib'.
> > The CrtLib is not used by jannson lib, libredfish as well
> > (https://github.com/DMTF/libredfish). So JanssonCrtLib is not a good
> naming, will keep that as it.
> >
> > >Also, does this library class (CrtLib.h) need to be  exported from
> > >RedfishPkg as a public interface?
> > No, it will be moved to under PrivateInclude.
> >
> > If CrtLib.h is defined as a private library and CrtLib.h is in
> > PrivateInclude which is not exposed to other packages, then why do we
> care about the naming of CrtLib is too generic? Which is the private library
> under RedfishPkg.
> > Move those header files to under PrivateInclude seems clear to people
> > that CrtLib is only for RedfishPkg. I think we don’t have to change the
> naming in this case.
> 
> In general I agree, but there is one place where the lib mapping has wider
> scope, and that is in a platform DSC file.  The lib class name CrtLib has to be
> listed in the DSC file.  If more than one package defines a library class called
> CrtLib, then the last mapping used will be used for all modules if it is listed in
> the [LibraryClasses] section of the DSC file.  The only was to handle this type
> of name collision is to use a module scoped library mapping in the
> <LibraryClasses> section of each individual module that uses a CrtLib.  I am
> only trying to avoid the potential for future name collisions for a library
> class/instance that is specific to the JSON use case.
> 
> We recognize that several modules in edk2 have this type of gasket between
> an EDK II env and a standard C lib.  We may find a we to implement a more
> generic gasket and retire the use of the implementation specific ones.  This is
> another reason to use an implementation specific name in this use case and
> be able to introduce a more generic name in the future.
> 
> Another comment below to see if there is a way to avoid introducing a CrtLib
> class.
> 
> >
> > >
> > > * RedfishPkg/Include/Library/CrtLib.h
> > >   + Remove reference to MDE_CPU_IA64.  This has been retired from
> > > the
> > > edk2 repo.
> > Ok.
> > >   + I do see one remaining reference to this in the CryptoPkg that need to
> be
> > >     removed.
> > >
> > > * RedfishPkg/Include/Library/JsonLib.h
> > >   + Some of the function descriptions are very brief and I can not tell
> > >     how to use the service from the description and more importantly, I
> > >     would not know how to write a unit test to verify the expected
> behavior.
> > >     Since these are a set of public APIs from this package that you
> > >     expect modules/libs from outside of RedfishPkg to use, then all of
> > >     these public APIs must be fully documented.
> > Most of the API in JsonLib are wrappers of native jansson APIs. We can
> > mention the URL to jansson API reference document in file header.
> > INVALID URI REMOVED
> 3A__jansson.readthedo
> >
> cs.io_en_2.13_index.html&d=DwIGaQ&c=C5b8zRQO1miGmBeVZ2LFWg&r=_
> SN6FZBN4
> > Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=vd-
> uPz864czoYA5C7YaPgqXfdPjv4c5kB
> > zTwV60NvNQ&s=acVzN3x4yND0jkg44bJ48ZkyPyugjwsYvUGdD4nJL74&e=
> > I will review all function header again,  please check v9 patch later.
> >
> > >   + Are there any of these APIs that are not really needed by modules/libs
> > >     outside the RedfishPkg?  It would be better to remove APIs that are
> > >     not needed outside RedfishPkg from this public include file.
> > >   + A couple examples that stood out are:
> > >     JsonDecreaseReference()
> > >     JsonIncreaseReference()
> > >     JsonObjectIterator()
> > >     JsonObjectIteratorValue()
> > >     JsonObjectIteratorNext()
> > The native jansson APIs of all above wrappers are used in libredfish.
> > So that is the potential use case for other JSON applications.
> 
> Can you provide a pointer to an example of this use case?
> 
> I am trying to make sure we are doing the right design of RedfishPkg with
> respect to CrtLib for this use case.  Are these are JSON apps that do not use
> JsonLib, but are expected to build in an EDK II build environment?  Would
> these JSON apps require more standard C lib services than the CrtLib used to
> build JsonLib?  Where would those additional C lib services come from?
> 
> There is a standard C lib in edk2-libc.  Would that be required to build the
> JSON apps.
> 
> In other uses of submodules with C lib dependencies, a new CrtLib like lib
> class was not added and instead the support was added directly to the EDK II
> wrapper APIs. Some examples are:
>   * MdeModulePkg\Library\BrotliCustomDecompressLib
>   * MdeModulePkg\Universal\RegularExpressionDxe
> 
> If there are use cases where the CrtLib will be added to an INF outside the
> RedfishPkg, then I still think the name needs to be changed to be specific to
> the JSON use case.
CrtLib should not be added to INF outside RedfishPkg, but it will be added to platform DSC file.
> 
> If the use of CrtLib is only within the RedfishPkg, then the Library Class can be
> declared in a [LibraryClasses.Common.Private] section of the DEC file.  See
> UnitTestFrameworkPkg for examples
Because CrtLib, JsonLib and other RedfishPkg modules will be included in other package dsc file (e.g. EmulatorPkg).
What if in EmulatorPkg one CrtLib libraryclass maps to the private one under RedfishPkg, but another with the same name is the public library provided by other package (e.g. MdeModulePkg) and used by modules other than RedfishPkg?
Is edk2 build tool that smart to link the private CrtLib for RedfishPkg modules and all other non RedfishPkg modules are linked with the public one?

Abner

> 
> >
> > >   + JsonDumpString()
> > >     func header params do not match func prototype
> > >     Does not describe who allocates the return buffer and who
> > >     is responsible for freeing the buffer returned.
> > >     What do Flags do?  What is the difference between this
> > >     function and JsonToText()?
> > In JsonToText, it only converts JSON array and object to text. We
> > could just remove this for now because I don’t really remember the reason
> to create this function, it has been a while. Seems to me it is fine to remove
> this function.
> >
> > >   + JsonLoadBuffer() - Error param description missing
> > >   + JsonArrayGetValue() does not describe the conditions NULL
> > >     is returned.
> > >   + JsonObjectGetKeys().  Return type is CHAR8**.  What
> > >     function need to be called to free the array?
> > All above addressed.
> >
> > >   + JsonValueGetAsciiString() and JsonValueGetUnicodeString()
> > >     are asymmetric.  The Ascii one states that change will
> > >     modify the original string.  The Unicode one says that the
> > >     caller needs to free the returned string.  Any reason we
> > >     can not make them symmetric?
> > Jansson doesn't support Unicode string. The memory of the unicode
> > string returned by JsonValueGetUnicodeString Is allocated by
> UTF8StrToUCS2().
> >  > Also, if Ascii one should
> > >     not be modified, why isn’t the return type CONST?
> > Yes. will do that.
> >
> > >
> > > * RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
> > >   + Change [Sources.common] to [Sources]
> > done
> > > * RedfishPkg/Library/CrtLib/CrtLib.inf
> > >   + Does this lib instance really depend on MdeModulePkg?
> > Yes, SortLib
> > >   + Have you reviewed the module types this lib instance is
> > >     compatible with?  Why does it need to support DXE_CORE?
> > >     Are their JsonLib use cases for DXE_RUNTIME and SMM drivers?
> > >     If there are SMM use cases, then why is MM excluded?
> > Will just keep DXE_DRVER, UEFI_APPLICATION and UEFI_DRIVER. These
> are
> > module types have potential use cases I can think of now.
> > >
> > > * RedfishPkg/Library/JsonLib
> > >   + Readme.rst still has references to JanssonJsonLibMapping.h
> > done
> > >   + Have you reviewed the module types this lib instance is
> > >     compatible with?  Why does it need to support DXE_CORE?
> > >     Are their JsonLib use cases for DXE_RUNTIME and SMM drivers?
> > >     If there are SMM use cases, then why is MM excluded?
> > Answered above
> > >   + JsonLib.inf has a [BuildOptions] section that disables some warnings
> that
> > >     make me concerned that this library may have some issues with
> > > 32-bit builds.
> > >     Have you fully validated all 32-bit CPU builds and validated the
> functionality
> > >     of all services from the JsonLib for all ranges of input values?
> > Yes, IA32 build is validated. But not validating on the
> > functionalities IA32. I don't have that use case, we can get back to fix the
> issue if any when  someone runs this on IA32.
> 
> I did some local testing with VS2019.  I found that a much smaller set of
> warning disables are required for IA32 and X64 and the set is different for
> IA32 and X64.  Can you review what is needed and minimize the required set
> of each supported arch?
> 
>   MSFT:*_*_X64_CC_FLAGS = /wd4244 /wd4090 /wd4334
> /DHAVE_CONFIG_H=1 /U_WIN32 /UWIN64 /U_MSC_VER
>   MSFT:*_*_IA32_CC_FLAGS = /wd4244 /wd4090 /DHAVE_CONFIG_H=1
> /U_WIN32 /UWIN64 /U_MSC_VER
> 
> > >
> > > * RedfishLibs.dsc.inc
> > >   It is better to add [LibraryClasses] statement to this file, so it
> > >   can be include anywhere in a DSC file.  With current implementation
> > >   if it is not included within a [LibraryClasses] section, it will
> > >   generate a build error.
> > I will separate this to another set of patch. Don’t mix up with jansson edk2
> port.
> >
> > >You might also consider changing the name
> > >   of this file in case you want to add more than just lib mappings
> > >   to this file in the future.  See UnitTestFramworkPkg for examples.
> > >
> > > Best regards,
> > >
> > > Mike
> > >
> > > > -----Original Message-----
> > > > From: Abner Chang <abner.chang@hpe.com>
> > > > Sent: Thursday, December 17, 2020 5:19 AM
> > > > To: devel@edk2.groups.io
> > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> > > > Laszlo Ersek <lersek@redhat.com>; Leif Lindholm
> > > > <leif@nuviainc.com>; Kinney, Michael D
> > > > <michael.d.kinney@intel.com>; Liming Gao
> > > > <gaoliming@byosoft.com.cn>; Nickle Wang <nickle.wang@hpe.com>;
> > > Peter
> > > > O'Hanley <peter.ohanley@hpe.com>
> > > > Subject: [PATCH v8 0/6] jansson edk2 port
> > > >
> > > > In v8, - Assigne patch file order
> > > >        - Add Acked-by tags
> > > > In v7, - Remove C RTC header files to under [Include.Common.Private]
> > > >          in RedfishPkg.dec.
> > > >        - address comments given by Mike Kinney.
> > > > In v6, Remove JanssonJsonMapping.h In v5, move BaseUcs2Utf8Lib to
> > > > under RedfishPkg.
> > > > In v4,
> > > >        - Address review comments
> > > >        - Seperate CRT functions to a individule library CrtLib under
> > > >          RedfishPkg.
> > > >        - Seperate UCS2-UTF8 functions to a individule library
> > > >          BaseUcs2Utf8Lib under MdeModulePkg.
> > > >
> > > > In v3, Add jansson library as the required submoudle in
> > > >        CiSettings.py for CI test.
> > > > In v2, JsonLib is moved to under RedfishPkg.
> > > >
> > > > edk2 JSON library is based on jansson open source
> > > > (https://github.com/akheron/jansson) and wrapped as an edk2 library.
> > > > edk2 JsonLib will be used by edk2 Redfish feature drivers (not
> > > > contributed yet) and the edk2 port of libredfish library (not
> > > > contributed yet) based on DMTF GitHub
> > > > (https://github.com/DMTF/libredfish).
> > > >
> > > > Jansson is licensed under the MIT license(refer to ReadMe.rst under
> edk2).
> > > > It is used in production and its API is stable. In UEFI/EDKII
> > > > environment, Redfish project consumes jansson to achieve JSON
> > > operations.
> > > >
> > > > * Jansson version on edk2: 2.13.1
> > > >
> > > > * EDKII jansson library wrapper:
> > > >    - JsonLib.h:
> > > >      This is the denifitions of EDKII JSON APIs which are mapped to
> > > >      jannson funcitons accordingly.
> > > >
> > > >    - JanssonJsonLibMapping.h:
> > > >      This is the wrapper file to map funcitons and definitions used in
> > > >      native jannson applications to edk2 JsonLib. This avoids the
> > > >      modifications on native jannson applications to be built under
> > > >      edk2 environment.
> > > >
> > > > *Known issue:
> > > >   Build fail with jansson/src/load.c, overrride and add code in load.c
> > > >   to conditionally use stdin according to HAVE_UNISTD_H macro.
> > > >   The PR is submitted to jansson open source community.
> > > >   https://github.com/akheron/jansson/pull/558
> > > >
> > > > Signed-off-by: Abner Chang <abner.chang@hpe.com>
> > > >
> > > > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > > > Cc: Andrew Fish <afish@apple.com>
> > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > Cc: Leif Lindholm <leif@nuviainc.com>
> > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > > Cc: Nickle Wang <nickle.wang@hpe.com>
> > > > Cc: Peter O'Hanley <peter.ohanley@hpe.com>
> > > >
> > > > Abner Chang (6):
> > > >   RedfishPkg/Ucs2Utf8lib: UCS2 to UFT8 manipulation library
> > > >   edk2: jansson submodule for edk2 JSON library
> > > >   RedfishPkg/CrtLib: C runtime library
> > > >   RedfishPkg/library: EDK2 port of jansson library
> > > >   RedfishPkg: Add EDK2 port of jansson library to build
> > > >   .pytool: Add required submodule for JsonLib
> > > >
> > > >  .gitmodules                                   |    3 +
> > > >  .pytool/CISettings.py                         |    2 +
> > > >  ReadMe.rst                                    |    1 +
> > > >  RedfishPkg/Include/Crt/assert.h               |   16 +
> > > >  RedfishPkg/Include/Crt/errno.h                |   16 +
> > > >  RedfishPkg/Include/Crt/limits.h               |   16 +
> > > >  RedfishPkg/Include/Crt/math.h                 |   16 +
> > > >  RedfishPkg/Include/Crt/stdarg.h               |   15 +
> > > >  RedfishPkg/Include/Crt/stddef.h               |   16 +
> > > >  RedfishPkg/Include/Crt/stdio.h                |   15 +
> > > >  RedfishPkg/Include/Crt/stdlib.h               |   16 +
> > > >  RedfishPkg/Include/Crt/string.h               |   16 +
> > > >  RedfishPkg/Include/Crt/sys/time.h             |   15 +
> > > >  RedfishPkg/Include/Crt/sys/types.h            |   15 +
> > > >  RedfishPkg/Include/Crt/time.h                 |   15 +
> > > >  RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h  |   61 +
> > > >  RedfishPkg/Include/Library/CrtLib.h           |  191 +++
> > > >  RedfishPkg/Include/Library/JsonLib.h          |  763 +++++++++++
> > > >  .../Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c |  421 +++++++
> > > >  .../BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf       |   31 +
> > > >  RedfishPkg/Library/CrtLib/CrtLib.c            |  595 +++++++++
> > > >  RedfishPkg/Library/CrtLib/CrtLib.inf          |   38 +
> > > >  RedfishPkg/Library/JsonLib/JsonLib.c          |  964 ++++++++++++++
> > > >  RedfishPkg/Library/JsonLib/JsonLib.inf        |   89 ++
> > > >  RedfishPkg/Library/JsonLib/Readme.rst         |   40 +
> > > >  RedfishPkg/Library/JsonLib/jansson            |    1 +
> > > >  RedfishPkg/Library/JsonLib/jansson_config.h   |   41 +
> > > >  .../Library/JsonLib/jansson_private_config.h  |   19 +
> > > >  RedfishPkg/Library/JsonLib/load.c             | 1111 +++++++++++++++++
> > > >  RedfishPkg/RedfishLibs.dsc.inc                |    3 +
> > > >  RedfishPkg/RedfishPkg.ci.yaml                 |   25 +
> > > >  RedfishPkg/RedfishPkg.dec                     |   25 +
> > > >  RedfishPkg/RedfishPkg.dsc                     |    3 +
> > > >  33 files changed, 4614 insertions(+)  create mode 100644
> > > > RedfishPkg/Include/Crt/assert.h  create mode
> > > > 100644 RedfishPkg/Include/Crt/errno.h  create mode 100644
> > > > RedfishPkg/Include/Crt/limits.h  create mode 100644
> > > > RedfishPkg/Include/Crt/math.h  create mode 100644
> > > > RedfishPkg/Include/Crt/stdarg.h  create mode 100644
> > > > RedfishPkg/Include/Crt/stddef.h  create mode 100644
> > > > RedfishPkg/Include/Crt/stdio.h  create mode 100644
> > > > RedfishPkg/Include/Crt/stdlib.h  create mode 100644
> > > > RedfishPkg/Include/Crt/string.h  create mode 100644
> > > > RedfishPkg/Include/Crt/sys/time.h  create mode 100644
> > > > RedfishPkg/Include/Crt/sys/types.h
> > > >  create mode 100644 RedfishPkg/Include/Crt/time.h  create mode
> > > > 100644 RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h
> > > >  create mode 100644 RedfishPkg/Include/Library/CrtLib.h
> > > >  create mode 100644 RedfishPkg/Include/Library/JsonLib.h
> > > >  create mode 100644
> > > > RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c
> > > >  create mode 100644
> > > > RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
> > > >  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.c
> > > >  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.inf
> > > >  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.c
> > > >  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.inf
> > > >  create mode 100644 RedfishPkg/Library/JsonLib/Readme.rst
> > > >  create mode 160000 RedfishPkg/Library/JsonLib/jansson
> > > >  create mode 100644 RedfishPkg/Library/JsonLib/jansson_config.h
> > > >  create mode 100644
> > > > RedfishPkg/Library/JsonLib/jansson_private_config.h
> > > >  create mode 100644 RedfishPkg/Library/JsonLib/load.c
> > > >
> > > > --
> > > > 2.17.1
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69396): https://edk2.groups.io/g/devel/message/69396
Mute This Topic: https://groups.io/mt/79052636/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Abner,

Response below.

Mike


> -----Original Message-----
> From: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>
> Sent: Tuesday, December 22, 2020 8:40 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
> Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming Gao <gaoliming@byosoft.com.cn>; Wang, Nickle
> (HPS SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL) <peter.ohanley@hpe.com>
> Subject: RE: [PATCH v8 0/6] jansson edk2 port
> 
> 
> 
> > -----Original Message-----
> > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> > Sent: Wednesday, December 23, 2020 2:14 AM
> > To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> > devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>; Laszlo
> > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming Gao
> > <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL) <peter.ohanley@hpe.com>
> > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> >
> > Hi Abner,
> >
> > A few comments below.
> >
> > Best regards,
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>
> > > Sent: Monday, December 21, 2020 12:17 AM
> > > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > > devel@edk2.groups.io
> > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>; Laszlo
> > > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming
> > > Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> > <peter.ohanley@hpe.com>
> > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > >
> > > Mike, response in below. Also v9 is sent.
> > >
> > > Thanks for review this in detail.
> > > Abner
> > >
> > > > -----Original Message-----
> > > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> > > > Sent: Monday, December 21, 2020 3:43 AM
> > > > To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> > > > devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> > Laszlo
> > > > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming
> > > > Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > > > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> > <peter.ohanley@hpe.com>
> > > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > > >
> > > > Hi Abner,
> > > >
> > > > The include path in the [Include.Common.Private] section should not
> > > > be in the same file path as the [Include] section.  This allows
> > > > private include files to be accessed.
> > > >
> > > > Instead, you should create a new top level package directory called
> > > > PrivateInclude and put all non-public include content in that
> > > > directory.  The UnitTestFrameworkPkg is a good reference that
> > > > demonstrates this design pattern.
> > > Yes, this looks better. All Crt header files and CrtLib.h will be moved to
> > under PrivateInclude.
> > > >
> > > > The library class name CrtLib is very generic and the library class
> > > > is in the public includes so it is exported as a public interface from the
> > RedfishPkg.
> > > > This CrtLib implementation appears to be tuned to support the
> > > > dependencies for the jansson submodule.  I recommend changing the
> > > > library class name and the library instance name so it is clear that
> > > > this CrtLib is only for jansson.
> > > > Perhaps 'JanssonCrtLib'.
> > > The CrtLib is not used by jannson lib, libredfish as well
> > > (https://github.com/DMTF/libredfish). So JanssonCrtLib is not a good
> > naming, will keep that as it.
> > >
> > > >Also, does this library class (CrtLib.h) need to be  exported from
> > > >RedfishPkg as a public interface?
> > > No, it will be moved to under PrivateInclude.
> > >
> > > If CrtLib.h is defined as a private library and CrtLib.h is in
> > > PrivateInclude which is not exposed to other packages, then why do we
> > care about the naming of CrtLib is too generic? Which is the private library
> > under RedfishPkg.
> > > Move those header files to under PrivateInclude seems clear to people
> > > that CrtLib is only for RedfishPkg. I think we don’t have to change the
> > naming in this case.
> >
> > In general I agree, but there is one place where the lib mapping has wider
> > scope, and that is in a platform DSC file.  The lib class name CrtLib has to be
> > listed in the DSC file.  If more than one package defines a library class called
> > CrtLib, then the last mapping used will be used for all modules if it is listed in
> > the [LibraryClasses] section of the DSC file.  The only was to handle this type
> > of name collision is to use a module scoped library mapping in the
> > <LibraryClasses> section of each individual module that uses a CrtLib.  I am
> > only trying to avoid the potential for future name collisions for a library
> > class/instance that is specific to the JSON use case.
> >
> > We recognize that several modules in edk2 have this type of gasket between
> > an EDK II env and a standard C lib.  We may find a we to implement a more
> > generic gasket and retire the use of the implementation specific ones.  This is
> > another reason to use an implementation specific name in this use case and
> > be able to introduce a more generic name in the future.
> >
> > Another comment below to see if there is a way to avoid introducing a CrtLib
> > class.
> >
> > >
> > > >
> > > > * RedfishPkg/Include/Library/CrtLib.h
> > > >   + Remove reference to MDE_CPU_IA64.  This has been retired from
> > > > the
> > > > edk2 repo.
> > > Ok.
> > > >   + I do see one remaining reference to this in the CryptoPkg that need to
> > be
> > > >     removed.
> > > >
> > > > * RedfishPkg/Include/Library/JsonLib.h
> > > >   + Some of the function descriptions are very brief and I can not tell
> > > >     how to use the service from the description and more importantly, I
> > > >     would not know how to write a unit test to verify the expected
> > behavior.
> > > >     Since these are a set of public APIs from this package that you
> > > >     expect modules/libs from outside of RedfishPkg to use, then all of
> > > >     these public APIs must be fully documented.
> > > Most of the API in JsonLib are wrappers of native jansson APIs. We can
> > > mention the URL to jansson API reference document in file header.
> > > https://urldefense.proofpoint.com/v2/url?u=https-
> > 3A__jansson.readthedo
> > >
> > cs.io_en_2.13_index.html&d=DwIGaQ&c=C5b8zRQO1miGmBeVZ2LFWg&r=_
> > SN6FZBN4
> > > Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=vd-
> > uPz864czoYA5C7YaPgqXfdPjv4c5kB
> > > zTwV60NvNQ&s=acVzN3x4yND0jkg44bJ48ZkyPyugjwsYvUGdD4nJL74&e=
> > > I will review all function header again,  please check v9 patch later.
> > >
> > > >   + Are there any of these APIs that are not really needed by modules/libs
> > > >     outside the RedfishPkg?  It would be better to remove APIs that are
> > > >     not needed outside RedfishPkg from this public include file.
> > > >   + A couple examples that stood out are:
> > > >     JsonDecreaseReference()
> > > >     JsonIncreaseReference()
> > > >     JsonObjectIterator()
> > > >     JsonObjectIteratorValue()
> > > >     JsonObjectIteratorNext()
> > > The native jansson APIs of all above wrappers are used in libredfish.
> > > So that is the potential use case for other JSON applications.
> >
> > Can you provide a pointer to an example of this use case?
> >
> > I am trying to make sure we are doing the right design of RedfishPkg with
> > respect to CrtLib for this use case.  Are these are JSON apps that do not use
> > JsonLib, but are expected to build in an EDK II build environment?  Would
> > these JSON apps require more standard C lib services than the CrtLib used to
> > build JsonLib?  Where would those additional C lib services come from?
> >
> > There is a standard C lib in edk2-libc.  Would that be required to build the
> > JSON apps.
> >
> > In other uses of submodules with C lib dependencies, a new CrtLib like lib
> > class was not added and instead the support was added directly to the EDK II
> > wrapper APIs. Some examples are:
> >   * MdeModulePkg\Library\BrotliCustomDecompressLib
> >   * MdeModulePkg\Universal\RegularExpressionDxe
> >
> > If there are use cases where the CrtLib will be added to an INF outside the
> > RedfishPkg, then I still think the name needs to be changed to be specific to
> > the JSON use case.
> CrtLib should not be added to INF outside RedfishPkg, but it will be added to platform DSC file.
> >
> > If the use of CrtLib is only within the RedfishPkg, then the Library Class can be
> > declared in a [LibraryClasses.Common.Private] section of the DEC file.  See
> > UnitTestFrameworkPkg for examples
> Because CrtLib, JsonLib and other RedfishPkg modules will be included in other package dsc file (e.g. EmulatorPkg).
> What if in EmulatorPkg one CrtLib libraryclass maps to the private one under RedfishPkg, but another with the same name is
> the public library provided by other package (e.g. MdeModulePkg) and used by modules other than RedfishPkg?
> Is edk2 build tool that smart to link the private CrtLib for RedfishPkg modules and all other non RedfishPkg modules are
> linked with the public one?
> 
> Abner
> 

No.  Not that smart.

There are global lib class -> lib instance mapping in [LibraryClasses] sections of DSC files.  If the same lib class
is listed more than once, then the last mapping wins.

If a module scoped <LibraryClasses> section is used, then then last mapping in the scope of that module wins.

If the same lib class name is declared by more than one package used in a platform DSC, then the only
remediation is to remove use of the global [LibraryClasses] section and use a module scope <LibraryClasses>
section in every module that depends on that lib class name.

> >
> > >
> > > >   + JsonDumpString()
> > > >     func header params do not match func prototype
> > > >     Does not describe who allocates the return buffer and who
> > > >     is responsible for freeing the buffer returned.
> > > >     What do Flags do?  What is the difference between this
> > > >     function and JsonToText()?
> > > In JsonToText, it only converts JSON array and object to text. We
> > > could just remove this for now because I don’t really remember the reason
> > to create this function, it has been a while. Seems to me it is fine to remove
> > this function.
> > >
> > > >   + JsonLoadBuffer() - Error param description missing
> > > >   + JsonArrayGetValue() does not describe the conditions NULL
> > > >     is returned.
> > > >   + JsonObjectGetKeys().  Return type is CHAR8**.  What
> > > >     function need to be called to free the array?
> > > All above addressed.
> > >
> > > >   + JsonValueGetAsciiString() and JsonValueGetUnicodeString()
> > > >     are asymmetric.  The Ascii one states that change will
> > > >     modify the original string.  The Unicode one says that the
> > > >     caller needs to free the returned string.  Any reason we
> > > >     can not make them symmetric?
> > > Jansson doesn't support Unicode string. The memory of the unicode
> > > string returned by JsonValueGetUnicodeString Is allocated by
> > UTF8StrToUCS2().
> > >  > Also, if Ascii one should
> > > >     not be modified, why isn’t the return type CONST?
> > > Yes. will do that.
> > >
> > > >
> > > > * RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
> > > >   + Change [Sources.common] to [Sources]
> > > done
> > > > * RedfishPkg/Library/CrtLib/CrtLib.inf
> > > >   + Does this lib instance really depend on MdeModulePkg?
> > > Yes, SortLib
> > > >   + Have you reviewed the module types this lib instance is
> > > >     compatible with?  Why does it need to support DXE_CORE?
> > > >     Are their JsonLib use cases for DXE_RUNTIME and SMM drivers?
> > > >     If there are SMM use cases, then why is MM excluded?
> > > Will just keep DXE_DRVER, UEFI_APPLICATION and UEFI_DRIVER. These
> > are
> > > module types have potential use cases I can think of now.
> > > >
> > > > * RedfishPkg/Library/JsonLib
> > > >   + Readme.rst still has references to JanssonJsonLibMapping.h
> > > done
> > > >   + Have you reviewed the module types this lib instance is
> > > >     compatible with?  Why does it need to support DXE_CORE?
> > > >     Are their JsonLib use cases for DXE_RUNTIME and SMM drivers?
> > > >     If there are SMM use cases, then why is MM excluded?
> > > Answered above
> > > >   + JsonLib.inf has a [BuildOptions] section that disables some warnings
> > that
> > > >     make me concerned that this library may have some issues with
> > > > 32-bit builds.
> > > >     Have you fully validated all 32-bit CPU builds and validated the
> > functionality
> > > >     of all services from the JsonLib for all ranges of input values?
> > > Yes, IA32 build is validated. But not validating on the
> > > functionalities IA32. I don't have that use case, we can get back to fix the
> > issue if any when  someone runs this on IA32.
> >
> > I did some local testing with VS2019.  I found that a much smaller set of
> > warning disables are required for IA32 and X64 and the set is different for
> > IA32 and X64.  Can you review what is needed and minimize the required set
> > of each supported arch?
> >
> >   MSFT:*_*_X64_CC_FLAGS = /wd4244 /wd4090 /wd4334
> > /DHAVE_CONFIG_H=1 /U_WIN32 /UWIN64 /U_MSC_VER
> >   MSFT:*_*_IA32_CC_FLAGS = /wd4244 /wd4090 /DHAVE_CONFIG_H=1
> > /U_WIN32 /UWIN64 /U_MSC_VER
> >
> > > >
> > > > * RedfishLibs.dsc.inc
> > > >   It is better to add [LibraryClasses] statement to this file, so it
> > > >   can be include anywhere in a DSC file.  With current implementation
> > > >   if it is not included within a [LibraryClasses] section, it will
> > > >   generate a build error.
> > > I will separate this to another set of patch. Don’t mix up with jansson edk2
> > port.
> > >
> > > >You might also consider changing the name
> > > >   of this file in case you want to add more than just lib mappings
> > > >   to this file in the future.  See UnitTestFramworkPkg for examples.
> > > >
> > > > Best regards,
> > > >
> > > > Mike
> > > >
> > > > > -----Original Message-----
> > > > > From: Abner Chang <abner.chang@hpe.com>
> > > > > Sent: Thursday, December 17, 2020 5:19 AM
> > > > > To: devel@edk2.groups.io
> > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> > > > > Laszlo Ersek <lersek@redhat.com>; Leif Lindholm
> > > > > <leif@nuviainc.com>; Kinney, Michael D
> > > > > <michael.d.kinney@intel.com>; Liming Gao
> > > > > <gaoliming@byosoft.com.cn>; Nickle Wang <nickle.wang@hpe.com>;
> > > > Peter
> > > > > O'Hanley <peter.ohanley@hpe.com>
> > > > > Subject: [PATCH v8 0/6] jansson edk2 port
> > > > >
> > > > > In v8, - Assigne patch file order
> > > > >        - Add Acked-by tags
> > > > > In v7, - Remove C RTC header files to under [Include.Common.Private]
> > > > >          in RedfishPkg.dec.
> > > > >        - address comments given by Mike Kinney.
> > > > > In v6, Remove JanssonJsonMapping.h In v5, move BaseUcs2Utf8Lib to
> > > > > under RedfishPkg.
> > > > > In v4,
> > > > >        - Address review comments
> > > > >        - Seperate CRT functions to a individule library CrtLib under
> > > > >          RedfishPkg.
> > > > >        - Seperate UCS2-UTF8 functions to a individule library
> > > > >          BaseUcs2Utf8Lib under MdeModulePkg.
> > > > >
> > > > > In v3, Add jansson library as the required submoudle in
> > > > >        CiSettings.py for CI test.
> > > > > In v2, JsonLib is moved to under RedfishPkg.
> > > > >
> > > > > edk2 JSON library is based on jansson open source
> > > > > (https://github.com/akheron/jansson) and wrapped as an edk2 library.
> > > > > edk2 JsonLib will be used by edk2 Redfish feature drivers (not
> > > > > contributed yet) and the edk2 port of libredfish library (not
> > > > > contributed yet) based on DMTF GitHub
> > > > > (https://github.com/DMTF/libredfish).
> > > > >
> > > > > Jansson is licensed under the MIT license(refer to ReadMe.rst under
> > edk2).
> > > > > It is used in production and its API is stable. In UEFI/EDKII
> > > > > environment, Redfish project consumes jansson to achieve JSON
> > > > operations.
> > > > >
> > > > > * Jansson version on edk2: 2.13.1
> > > > >
> > > > > * EDKII jansson library wrapper:
> > > > >    - JsonLib.h:
> > > > >      This is the denifitions of EDKII JSON APIs which are mapped to
> > > > >      jannson funcitons accordingly.
> > > > >
> > > > >    - JanssonJsonLibMapping.h:
> > > > >      This is the wrapper file to map funcitons and definitions used in
> > > > >      native jannson applications to edk2 JsonLib. This avoids the
> > > > >      modifications on native jannson applications to be built under
> > > > >      edk2 environment.
> > > > >
> > > > > *Known issue:
> > > > >   Build fail with jansson/src/load.c, overrride and add code in load.c
> > > > >   to conditionally use stdin according to HAVE_UNISTD_H macro.
> > > > >   The PR is submitted to jansson open source community.
> > > > >   https://github.com/akheron/jansson/pull/558
> > > > >
> > > > > Signed-off-by: Abner Chang <abner.chang@hpe.com>
> > > > >
> > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > > > > Cc: Andrew Fish <afish@apple.com>
> > > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > > Cc: Leif Lindholm <leif@nuviainc.com>
> > > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > > > Cc: Nickle Wang <nickle.wang@hpe.com>
> > > > > Cc: Peter O'Hanley <peter.ohanley@hpe.com>
> > > > >
> > > > > Abner Chang (6):
> > > > >   RedfishPkg/Ucs2Utf8lib: UCS2 to UFT8 manipulation library
> > > > >   edk2: jansson submodule for edk2 JSON library
> > > > >   RedfishPkg/CrtLib: C runtime library
> > > > >   RedfishPkg/library: EDK2 port of jansson library
> > > > >   RedfishPkg: Add EDK2 port of jansson library to build
> > > > >   .pytool: Add required submodule for JsonLib
> > > > >
> > > > >  .gitmodules                                   |    3 +
> > > > >  .pytool/CISettings.py                         |    2 +
> > > > >  ReadMe.rst                                    |    1 +
> > > > >  RedfishPkg/Include/Crt/assert.h               |   16 +
> > > > >  RedfishPkg/Include/Crt/errno.h                |   16 +
> > > > >  RedfishPkg/Include/Crt/limits.h               |   16 +
> > > > >  RedfishPkg/Include/Crt/math.h                 |   16 +
> > > > >  RedfishPkg/Include/Crt/stdarg.h               |   15 +
> > > > >  RedfishPkg/Include/Crt/stddef.h               |   16 +
> > > > >  RedfishPkg/Include/Crt/stdio.h                |   15 +
> > > > >  RedfishPkg/Include/Crt/stdlib.h               |   16 +
> > > > >  RedfishPkg/Include/Crt/string.h               |   16 +
> > > > >  RedfishPkg/Include/Crt/sys/time.h             |   15 +
> > > > >  RedfishPkg/Include/Crt/sys/types.h            |   15 +
> > > > >  RedfishPkg/Include/Crt/time.h                 |   15 +
> > > > >  RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h  |   61 +
> > > > >  RedfishPkg/Include/Library/CrtLib.h           |  191 +++
> > > > >  RedfishPkg/Include/Library/JsonLib.h          |  763 +++++++++++
> > > > >  .../Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c |  421 +++++++
> > > > >  .../BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf       |   31 +
> > > > >  RedfishPkg/Library/CrtLib/CrtLib.c            |  595 +++++++++
> > > > >  RedfishPkg/Library/CrtLib/CrtLib.inf          |   38 +
> > > > >  RedfishPkg/Library/JsonLib/JsonLib.c          |  964 ++++++++++++++
> > > > >  RedfishPkg/Library/JsonLib/JsonLib.inf        |   89 ++
> > > > >  RedfishPkg/Library/JsonLib/Readme.rst         |   40 +
> > > > >  RedfishPkg/Library/JsonLib/jansson            |    1 +
> > > > >  RedfishPkg/Library/JsonLib/jansson_config.h   |   41 +
> > > > >  .../Library/JsonLib/jansson_private_config.h  |   19 +
> > > > >  RedfishPkg/Library/JsonLib/load.c             | 1111 +++++++++++++++++
> > > > >  RedfishPkg/RedfishLibs.dsc.inc                |    3 +
> > > > >  RedfishPkg/RedfishPkg.ci.yaml                 |   25 +
> > > > >  RedfishPkg/RedfishPkg.dec                     |   25 +
> > > > >  RedfishPkg/RedfishPkg.dsc                     |    3 +
> > > > >  33 files changed, 4614 insertions(+)  create mode 100644
> > > > > RedfishPkg/Include/Crt/assert.h  create mode
> > > > > 100644 RedfishPkg/Include/Crt/errno.h  create mode 100644
> > > > > RedfishPkg/Include/Crt/limits.h  create mode 100644
> > > > > RedfishPkg/Include/Crt/math.h  create mode 100644
> > > > > RedfishPkg/Include/Crt/stdarg.h  create mode 100644
> > > > > RedfishPkg/Include/Crt/stddef.h  create mode 100644
> > > > > RedfishPkg/Include/Crt/stdio.h  create mode 100644
> > > > > RedfishPkg/Include/Crt/stdlib.h  create mode 100644
> > > > > RedfishPkg/Include/Crt/string.h  create mode 100644
> > > > > RedfishPkg/Include/Crt/sys/time.h  create mode 100644
> > > > > RedfishPkg/Include/Crt/sys/types.h
> > > > >  create mode 100644 RedfishPkg/Include/Crt/time.h  create mode
> > > > > 100644 RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h
> > > > >  create mode 100644 RedfishPkg/Include/Library/CrtLib.h
> > > > >  create mode 100644 RedfishPkg/Include/Library/JsonLib.h
> > > > >  create mode 100644
> > > > > RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c
> > > > >  create mode 100644
> > > > > RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
> > > > >  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.c
> > > > >  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.inf
> > > > >  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.c
> > > > >  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.inf
> > > > >  create mode 100644 RedfishPkg/Library/JsonLib/Readme.rst
> > > > >  create mode 160000 RedfishPkg/Library/JsonLib/jansson
> > > > >  create mode 100644 RedfishPkg/Library/JsonLib/jansson_config.h
> > > > >  create mode 100644
> > > > > RedfishPkg/Library/JsonLib/jansson_private_config.h
> > > > >  create mode 100644 RedfishPkg/Library/JsonLib/load.c
> > > > >
> > > > > --
> > > > > 2.17.1
> > >
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69398): https://edk2.groups.io/g/devel/message/69398
Mute This Topic: https://groups.io/mt/79052636/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> -----Original Message-----
> From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> Sent: Wednesday, December 23, 2020 2:10 PM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>; Laszlo
> Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> <nickle.wang@hpe.com>; O'Hanley, Peter (EXL) <peter.ohanley@hpe.com>
> Subject: RE: [PATCH v8 0/6] jansson edk2 port
> 
> Hi Abner,
> 
> Response below.
> 
> Mike
> 
> 
> > -----Original Message-----
> > From: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>
> > Sent: Tuesday, December 22, 2020 8:40 PM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > devel@edk2.groups.io
> > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>; Laszlo
> > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming
> > Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley@hpe.com>
> > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> >
> >
> >
> > > -----Original Message-----
> > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> > > Sent: Wednesday, December 23, 2020 2:14 AM
> > > To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> > > devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> Laszlo
> > > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming
> > > Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley@hpe.com>
> > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > >
> > > Hi Abner,
> > >
> > > A few comments below.
> > >
> > > Best regards,
> > >
> > > Mike
> > >
> > > > -----Original Message-----
> > > > From: Chang, Abner (HPS SW/FW Technologist)
> <abner.chang@hpe.com>
> > > > Sent: Monday, December 21, 2020 12:17 AM
> > > > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > > > devel@edk2.groups.io
> > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> > > > Laszlo Ersek <lersek@redhat.com>; Leif Lindholm
> > > > <leif@nuviainc.com>; Liming Gao <gaoliming@byosoft.com.cn>; Wang,
> > > > Nickle (HPS SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> > > <peter.ohanley@hpe.com>
> > > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > > >
> > > > Mike, response in below. Also v9 is sent.
> > > >
> > > > Thanks for review this in detail.
> > > > Abner
> > > >
> > > > > -----Original Message-----
> > > > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> > > > > Sent: Monday, December 21, 2020 3:43 AM
> > > > > To: Chang, Abner (HPS SW/FW Technologist)
> <abner.chang@hpe.com>;
> > > > > devel@edk2.groups.io; Kinney, Michael D
> > > > > <michael.d.kinney@intel.com>
> > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> > > Laszlo
> > > > > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>;
> > > > > Liming Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > > > > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> > > <peter.ohanley@hpe.com>
> > > > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > > > >
> > > > > Hi Abner,
> > > > >
> > > > > The include path in the [Include.Common.Private] section should
> > > > > not be in the same file path as the [Include] section.  This
> > > > > allows private include files to be accessed.
> > > > >
> > > > > Instead, you should create a new top level package directory
> > > > > called PrivateInclude and put all non-public include content in
> > > > > that directory.  The UnitTestFrameworkPkg is a good reference
> > > > > that demonstrates this design pattern.
> > > > Yes, this looks better. All Crt header files and CrtLib.h will be
> > > > moved to
> > > under PrivateInclude.
> > > > >
> > > > > The library class name CrtLib is very generic and the library
> > > > > class is in the public includes so it is exported as a public
> > > > > interface from the
> > > RedfishPkg.
> > > > > This CrtLib implementation appears to be tuned to support the
> > > > > dependencies for the jansson submodule.  I recommend changing
> > > > > the library class name and the library instance name so it is
> > > > > clear that this CrtLib is only for jansson.
> > > > > Perhaps 'JanssonCrtLib'.
> > > > The CrtLib is not used by jannson lib, libredfish as well
> > > > (https://github.com/DMTF/libredfish). So JanssonCrtLib is not a
> > > > good
> > > naming, will keep that as it.
> > > >
> > > > >Also, does this library class (CrtLib.h) need to be  exported
> > > > >from RedfishPkg as a public interface?
> > > > No, it will be moved to under PrivateInclude.
> > > >
> > > > If CrtLib.h is defined as a private library and CrtLib.h is in
> > > > PrivateInclude which is not exposed to other packages, then why do
> > > > we
> > > care about the naming of CrtLib is too generic? Which is the private
> > > library under RedfishPkg.
> > > > Move those header files to under PrivateInclude seems clear to
> > > > people that CrtLib is only for RedfishPkg. I think we don’t have
> > > > to change the
> > > naming in this case.
> > >
> > > In general I agree, but there is one place where the lib mapping has
> > > wider scope, and that is in a platform DSC file.  The lib class name
> > > CrtLib has to be listed in the DSC file.  If more than one package
> > > defines a library class called CrtLib, then the last mapping used
> > > will be used for all modules if it is listed in the [LibraryClasses]
> > > section of the DSC file.  The only was to handle this type of name
> > > collision is to use a module scoped library mapping in the
> > > <LibraryClasses> section of each individual module that uses a
> > > CrtLib.  I am only trying to avoid the potential for future name collisions
> for a library class/instance that is specific to the JSON use case.
> > >
> > > We recognize that several modules in edk2 have this type of gasket
> > > between an EDK II env and a standard C lib.  We may find a we to
> > > implement a more generic gasket and retire the use of the
> > > implementation specific ones.  This is another reason to use an
> > > implementation specific name in this use case and be able to introduce a
> more generic name in the future.
> > >
> > > Another comment below to see if there is a way to avoid introducing
> > > a CrtLib class.
> > >
> > > >
> > > > >
> > > > > * RedfishPkg/Include/Library/CrtLib.h
> > > > >   + Remove reference to MDE_CPU_IA64.  This has been retired
> > > > > from the
> > > > > edk2 repo.
> > > > Ok.
> > > > >   + I do see one remaining reference to this in the CryptoPkg
> > > > > that need to
> > > be
> > > > >     removed.
> > > > >
> > > > > * RedfishPkg/Include/Library/JsonLib.h
> > > > >   + Some of the function descriptions are very brief and I can not tell
> > > > >     how to use the service from the description and more importantly, I
> > > > >     would not know how to write a unit test to verify the
> > > > > expected
> > > behavior.
> > > > >     Since these are a set of public APIs from this package that you
> > > > >     expect modules/libs from outside of RedfishPkg to use, then all of
> > > > >     these public APIs must be fully documented.
> > > > Most of the API in JsonLib are wrappers of native jansson APIs. We
> > > > can mention the URL to jansson API reference document in file header.
> > > > INVALID URI REMOVED
> > > 3A__jansson.readthedo
> > > >
> > >
> cs.io_en_2.13_index.html&d=DwIGaQ&c=C5b8zRQO1miGmBeVZ2LFWg&r=_
> > > SN6FZBN4
> > > > Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=vd-
> > > uPz864czoYA5C7YaPgqXfdPjv4c5kB
> > > >
> zTwV60NvNQ&s=acVzN3x4yND0jkg44bJ48ZkyPyugjwsYvUGdD4nJL74&e=
> > > > I will review all function header again,  please check v9 patch later.
> > > >
> > > > >   + Are there any of these APIs that are not really needed by
> modules/libs
> > > > >     outside the RedfishPkg?  It would be better to remove APIs that are
> > > > >     not needed outside RedfishPkg from this public include file.
> > > > >   + A couple examples that stood out are:
> > > > >     JsonDecreaseReference()
> > > > >     JsonIncreaseReference()
> > > > >     JsonObjectIterator()
> > > > >     JsonObjectIteratorValue()
> > > > >     JsonObjectIteratorNext()
> > > > The native jansson APIs of all above wrappers are used in libredfish.
> > > > So that is the potential use case for other JSON applications.
> > >
> > > Can you provide a pointer to an example of this use case?
> > >
> > > I am trying to make sure we are doing the right design of RedfishPkg
> > > with respect to CrtLib for this use case.  Are these are JSON apps
> > > that do not use JsonLib, but are expected to build in an EDK II
> > > build environment?  Would these JSON apps require more standard C
> > > lib services than the CrtLib used to build JsonLib?  Where would those
> additional C lib services come from?
> > >
> > > There is a standard C lib in edk2-libc.  Would that be required to
> > > build the JSON apps.
> > >
> > > In other uses of submodules with C lib dependencies, a new CrtLib
> > > like lib class was not added and instead the support was added
> > > directly to the EDK II wrapper APIs. Some examples are:
> > >   * MdeModulePkg\Library\BrotliCustomDecompressLib
> > >   * MdeModulePkg\Universal\RegularExpressionDxe
> > >
> > > If there are use cases where the CrtLib will be added to an INF
> > > outside the RedfishPkg, then I still think the name needs to be
> > > changed to be specific to the JSON use case.
> > CrtLib should not be added to INF outside RedfishPkg, but it will be added
> to platform DSC file.
> > >
> > > If the use of CrtLib is only within the RedfishPkg, then the Library
> > > Class can be declared in a [LibraryClasses.Common.Private] section
> > > of the DEC file.  See UnitTestFrameworkPkg for examples
> > Because CrtLib, JsonLib and other RedfishPkg modules will be included in
> other package dsc file (e.g. EmulatorPkg).
> > What if in EmulatorPkg one CrtLib libraryclass maps to the private one
> > under RedfishPkg, but another with the same name is the public library
> provided by other package (e.g. MdeModulePkg) and used by modules
> other than RedfishPkg?
> > Is edk2 build tool that smart to link the private CrtLib for
> > RedfishPkg modules and all other non RedfishPkg modules are linked with
> the public one?
> >
> > Abner
> >
> 
> No.  Not that smart.
> 
> There are global lib class -> lib instance mapping in [LibraryClasses] sections of
> DSC files.  If the same lib class is listed more than once, then the last mapping
> wins.
> 
> If a module scoped <LibraryClasses> section is used, then then last mapping
> in the scope of that module wins.
> 
> If the same lib class name is declared by more than one package used in a
> platform DSC, then the only remediation is to remove use of the global
> [LibraryClasses] section and use a module scope <LibraryClasses> section in
> every module that depends on that lib class name.
Ok, I will rename it to RedfishCrtLib.

Abner
> 
> > >
> > > >
> > > > >   + JsonDumpString()
> > > > >     func header params do not match func prototype
> > > > >     Does not describe who allocates the return buffer and who
> > > > >     is responsible for freeing the buffer returned.
> > > > >     What do Flags do?  What is the difference between this
> > > > >     function and JsonToText()?
> > > > In JsonToText, it only converts JSON array and object to text. We
> > > > could just remove this for now because I don’t really remember the
> > > > reason
> > > to create this function, it has been a while. Seems to me it is fine
> > > to remove this function.
> > > >
> > > > >   + JsonLoadBuffer() - Error param description missing
> > > > >   + JsonArrayGetValue() does not describe the conditions NULL
> > > > >     is returned.
> > > > >   + JsonObjectGetKeys().  Return type is CHAR8**.  What
> > > > >     function need to be called to free the array?
> > > > All above addressed.
> > > >
> > > > >   + JsonValueGetAsciiString() and JsonValueGetUnicodeString()
> > > > >     are asymmetric.  The Ascii one states that change will
> > > > >     modify the original string.  The Unicode one says that the
> > > > >     caller needs to free the returned string.  Any reason we
> > > > >     can not make them symmetric?
> > > > Jansson doesn't support Unicode string. The memory of the unicode
> > > > string returned by JsonValueGetUnicodeString Is allocated by
> > > UTF8StrToUCS2().
> > > >  > Also, if Ascii one should
> > > > >     not be modified, why isn’t the return type CONST?
> > > > Yes. will do that.
> > > >
> > > > >
> > > > > * RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
> > > > >   + Change [Sources.common] to [Sources]
> > > > done
> > > > > * RedfishPkg/Library/CrtLib/CrtLib.inf
> > > > >   + Does this lib instance really depend on MdeModulePkg?
> > > > Yes, SortLib
> > > > >   + Have you reviewed the module types this lib instance is
> > > > >     compatible with?  Why does it need to support DXE_CORE?
> > > > >     Are their JsonLib use cases for DXE_RUNTIME and SMM drivers?
> > > > >     If there are SMM use cases, then why is MM excluded?
> > > > Will just keep DXE_DRVER, UEFI_APPLICATION and UEFI_DRIVER.
> These
> > > are
> > > > module types have potential use cases I can think of now.
> > > > >
> > > > > * RedfishPkg/Library/JsonLib
> > > > >   + Readme.rst still has references to JanssonJsonLibMapping.h
> > > > done
> > > > >   + Have you reviewed the module types this lib instance is
> > > > >     compatible with?  Why does it need to support DXE_CORE?
> > > > >     Are their JsonLib use cases for DXE_RUNTIME and SMM drivers?
> > > > >     If there are SMM use cases, then why is MM excluded?
> > > > Answered above
> > > > >   + JsonLib.inf has a [BuildOptions] section that disables some
> > > > > warnings
> > > that
> > > > >     make me concerned that this library may have some issues
> > > > > with 32-bit builds.
> > > > >     Have you fully validated all 32-bit CPU builds and validated
> > > > > the
> > > functionality
> > > > >     of all services from the JsonLib for all ranges of input values?
> > > > Yes, IA32 build is validated. But not validating on the
> > > > functionalities IA32. I don't have that use case, we can get back
> > > > to fix the
> > > issue if any when  someone runs this on IA32.
> > >
> > > I did some local testing with VS2019.  I found that a much smaller
> > > set of warning disables are required for IA32 and X64 and the set is
> > > different for
> > > IA32 and X64.  Can you review what is needed and minimize the
> > > required set of each supported arch?
> > >
> > >   MSFT:*_*_X64_CC_FLAGS = /wd4244 /wd4090 /wd4334
> > > /DHAVE_CONFIG_H=1 /U_WIN32 /UWIN64 /U_MSC_VER
> > >   MSFT:*_*_IA32_CC_FLAGS = /wd4244 /wd4090 /DHAVE_CONFIG_H=1
> > > /U_WIN32 /UWIN64 /U_MSC_VER
> > >
> > > > >
> > > > > * RedfishLibs.dsc.inc
> > > > >   It is better to add [LibraryClasses] statement to this file, so it
> > > > >   can be include anywhere in a DSC file.  With current implementation
> > > > >   if it is not included within a [LibraryClasses] section, it will
> > > > >   generate a build error.
> > > > I will separate this to another set of patch. Don’t mix up with
> > > > jansson edk2
> > > port.
> > > >
> > > > >You might also consider changing the name
> > > > >   of this file in case you want to add more than just lib mappings
> > > > >   to this file in the future.  See UnitTestFramworkPkg for examples.
> > > > >
> > > > > Best regards,
> > > > >
> > > > > Mike
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Abner Chang <abner.chang@hpe.com>
> > > > > > Sent: Thursday, December 17, 2020 5:19 AM
> > > > > > To: devel@edk2.groups.io
> > > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> > > > > > Laszlo Ersek <lersek@redhat.com>; Leif Lindholm
> > > > > > <leif@nuviainc.com>; Kinney, Michael D
> > > > > > <michael.d.kinney@intel.com>; Liming Gao
> > > > > > <gaoliming@byosoft.com.cn>; Nickle Wang
> <nickle.wang@hpe.com>;
> > > > > Peter
> > > > > > O'Hanley <peter.ohanley@hpe.com>
> > > > > > Subject: [PATCH v8 0/6] jansson edk2 port
> > > > > >
> > > > > > In v8, - Assigne patch file order
> > > > > >        - Add Acked-by tags
> > > > > > In v7, - Remove C RTC header files to under
> [Include.Common.Private]
> > > > > >          in RedfishPkg.dec.
> > > > > >        - address comments given by Mike Kinney.
> > > > > > In v6, Remove JanssonJsonMapping.h In v5, move BaseUcs2Utf8Lib
> > > > > > to under RedfishPkg.
> > > > > > In v4,
> > > > > >        - Address review comments
> > > > > >        - Seperate CRT functions to a individule library CrtLib under
> > > > > >          RedfishPkg.
> > > > > >        - Seperate UCS2-UTF8 functions to a individule library
> > > > > >          BaseUcs2Utf8Lib under MdeModulePkg.
> > > > > >
> > > > > > In v3, Add jansson library as the required submoudle in
> > > > > >        CiSettings.py for CI test.
> > > > > > In v2, JsonLib is moved to under RedfishPkg.
> > > > > >
> > > > > > edk2 JSON library is based on jansson open source
> > > > > > (https://github.com/akheron/jansson) and wrapped as an edk2
> library.
> > > > > > edk2 JsonLib will be used by edk2 Redfish feature drivers (not
> > > > > > contributed yet) and the edk2 port of libredfish library (not
> > > > > > contributed yet) based on DMTF GitHub
> > > > > > (https://github.com/DMTF/libredfish).
> > > > > >
> > > > > > Jansson is licensed under the MIT license(refer to ReadMe.rst
> > > > > > under
> > > edk2).
> > > > > > It is used in production and its API is stable. In UEFI/EDKII
> > > > > > environment, Redfish project consumes jansson to achieve JSON
> > > > > operations.
> > > > > >
> > > > > > * Jansson version on edk2: 2.13.1
> > > > > >
> > > > > > * EDKII jansson library wrapper:
> > > > > >    - JsonLib.h:
> > > > > >      This is the denifitions of EDKII JSON APIs which are mapped to
> > > > > >      jannson funcitons accordingly.
> > > > > >
> > > > > >    - JanssonJsonLibMapping.h:
> > > > > >      This is the wrapper file to map funcitons and definitions used in
> > > > > >      native jannson applications to edk2 JsonLib. This avoids the
> > > > > >      modifications on native jannson applications to be built under
> > > > > >      edk2 environment.
> > > > > >
> > > > > > *Known issue:
> > > > > >   Build fail with jansson/src/load.c, overrride and add code in load.c
> > > > > >   to conditionally use stdin according to HAVE_UNISTD_H macro.
> > > > > >   The PR is submitted to jansson open source community.
> > > > > >   https://github.com/akheron/jansson/pull/558
> > > > > >
> > > > > > Signed-off-by: Abner Chang <abner.chang@hpe.com>
> > > > > >
> > > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > > > > > Cc: Andrew Fish <afish@apple.com>
> > > > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > > > Cc: Leif Lindholm <leif@nuviainc.com>
> > > > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > > > > Cc: Nickle Wang <nickle.wang@hpe.com>
> > > > > > Cc: Peter O'Hanley <peter.ohanley@hpe.com>
> > > > > >
> > > > > > Abner Chang (6):
> > > > > >   RedfishPkg/Ucs2Utf8lib: UCS2 to UFT8 manipulation library
> > > > > >   edk2: jansson submodule for edk2 JSON library
> > > > > >   RedfishPkg/CrtLib: C runtime library
> > > > > >   RedfishPkg/library: EDK2 port of jansson library
> > > > > >   RedfishPkg: Add EDK2 port of jansson library to build
> > > > > >   .pytool: Add required submodule for JsonLib
> > > > > >
> > > > > >  .gitmodules                                   |    3 +
> > > > > >  .pytool/CISettings.py                         |    2 +
> > > > > >  ReadMe.rst                                    |    1 +
> > > > > >  RedfishPkg/Include/Crt/assert.h               |   16 +
> > > > > >  RedfishPkg/Include/Crt/errno.h                |   16 +
> > > > > >  RedfishPkg/Include/Crt/limits.h               |   16 +
> > > > > >  RedfishPkg/Include/Crt/math.h                 |   16 +
> > > > > >  RedfishPkg/Include/Crt/stdarg.h               |   15 +
> > > > > >  RedfishPkg/Include/Crt/stddef.h               |   16 +
> > > > > >  RedfishPkg/Include/Crt/stdio.h                |   15 +
> > > > > >  RedfishPkg/Include/Crt/stdlib.h               |   16 +
> > > > > >  RedfishPkg/Include/Crt/string.h               |   16 +
> > > > > >  RedfishPkg/Include/Crt/sys/time.h             |   15 +
> > > > > >  RedfishPkg/Include/Crt/sys/types.h            |   15 +
> > > > > >  RedfishPkg/Include/Crt/time.h                 |   15 +
> > > > > >  RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h  |   61 +
> > > > > >  RedfishPkg/Include/Library/CrtLib.h           |  191 +++
> > > > > >  RedfishPkg/Include/Library/JsonLib.h          |  763 +++++++++++
> > > > > >  .../Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c |  421 +++++++
> > > > > >  .../BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf       |   31 +
> > > > > >  RedfishPkg/Library/CrtLib/CrtLib.c            |  595 +++++++++
> > > > > >  RedfishPkg/Library/CrtLib/CrtLib.inf          |   38 +
> > > > > >  RedfishPkg/Library/JsonLib/JsonLib.c          |  964 ++++++++++++++
> > > > > >  RedfishPkg/Library/JsonLib/JsonLib.inf        |   89 ++
> > > > > >  RedfishPkg/Library/JsonLib/Readme.rst         |   40 +
> > > > > >  RedfishPkg/Library/JsonLib/jansson            |    1 +
> > > > > >  RedfishPkg/Library/JsonLib/jansson_config.h   |   41 +
> > > > > >  .../Library/JsonLib/jansson_private_config.h  |   19 +
> > > > > >  RedfishPkg/Library/JsonLib/load.c             | 1111
> +++++++++++++++++
> > > > > >  RedfishPkg/RedfishLibs.dsc.inc                |    3 +
> > > > > >  RedfishPkg/RedfishPkg.ci.yaml                 |   25 +
> > > > > >  RedfishPkg/RedfishPkg.dec                     |   25 +
> > > > > >  RedfishPkg/RedfishPkg.dsc                     |    3 +
> > > > > >  33 files changed, 4614 insertions(+)  create mode 100644
> > > > > > RedfishPkg/Include/Crt/assert.h  create mode
> > > > > > 100644 RedfishPkg/Include/Crt/errno.h  create mode 100644
> > > > > > RedfishPkg/Include/Crt/limits.h  create mode 100644
> > > > > > RedfishPkg/Include/Crt/math.h  create mode 100644
> > > > > > RedfishPkg/Include/Crt/stdarg.h  create mode 100644
> > > > > > RedfishPkg/Include/Crt/stddef.h  create mode 100644
> > > > > > RedfishPkg/Include/Crt/stdio.h  create mode 100644
> > > > > > RedfishPkg/Include/Crt/stdlib.h  create mode 100644
> > > > > > RedfishPkg/Include/Crt/string.h  create mode 100644
> > > > > > RedfishPkg/Include/Crt/sys/time.h  create mode 100644
> > > > > > RedfishPkg/Include/Crt/sys/types.h
> > > > > >  create mode 100644 RedfishPkg/Include/Crt/time.h  create mode
> > > > > > 100644 RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h
> > > > > >  create mode 100644 RedfishPkg/Include/Library/CrtLib.h
> > > > > >  create mode 100644 RedfishPkg/Include/Library/JsonLib.h
> > > > > >  create mode 100644
> > > > > > RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c
> > > > > >  create mode 100644
> > > > > > RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
> > > > > >  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.c
> > > > > >  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.inf
> > > > > >  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.c
> > > > > >  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.inf
> > > > > >  create mode 100644 RedfishPkg/Library/JsonLib/Readme.rst
> > > > > >  create mode 160000 RedfishPkg/Library/JsonLib/jansson
> > > > > >  create mode 100644
> > > > > > RedfishPkg/Library/JsonLib/jansson_config.h
> > > > > >  create mode 100644
> > > > > > RedfishPkg/Library/JsonLib/jansson_private_config.h
> > > > > >  create mode 100644 RedfishPkg/Library/JsonLib/load.c
> > > > > >
> > > > > > --
> > > > > > 2.17.1
> > > >
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69407): https://edk2.groups.io/g/devel/message/69407
Mute This Topic: https://groups.io/mt/79052636/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Mike, v10 patch set sent.
BTW, why don’t you put those private libraries in UniTestFrameworkPkg to under /PrivateLibrary which is similar to /PrivateInclude?

Abner

> -----Original Message-----
> From: Chang, Abner (HPS SW/FW Technologist)
> Sent: Wednesday, December 23, 2020 3:54 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
> Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>; Laszlo
> Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> <nickle.wang@hpe.com>; O'Hanley, Peter (EXL) <peter.ohanley@hpe.com>
> Subject: RE: [PATCH v8 0/6] jansson edk2 port
> 
> 
> 
> > -----Original Message-----
> > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> > Sent: Wednesday, December 23, 2020 2:10 PM
> > To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> > devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>; Laszlo
> > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming
> > Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley@hpe.com>
> > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> >
> > Hi Abner,
> >
> > Response below.
> >
> > Mike
> >
> >
> > > -----Original Message-----
> > > From: Chang, Abner (HPS SW/FW Technologist)
> <abner.chang@hpe.com>
> > > Sent: Tuesday, December 22, 2020 8:40 PM
> > > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > > devel@edk2.groups.io
> > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> Laszlo
> > > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming
> > > Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> > <peter.ohanley@hpe.com>
> > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> > > > Sent: Wednesday, December 23, 2020 2:14 AM
> > > > To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> > > > devel@edk2.groups.io; Kinney, Michael D
> > > > <michael.d.kinney@intel.com>
> > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> > Laszlo
> > > > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>;
> > > > Liming Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > > > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> > <peter.ohanley@hpe.com>
> > > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > > >
> > > > Hi Abner,
> > > >
> > > > A few comments below.
> > > >
> > > > Best regards,
> > > >
> > > > Mike
> > > >
> > > > > -----Original Message-----
> > > > > From: Chang, Abner (HPS SW/FW Technologist)
> > <abner.chang@hpe.com>
> > > > > Sent: Monday, December 21, 2020 12:17 AM
> > > > > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > > > > devel@edk2.groups.io
> > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> > > > > Laszlo Ersek <lersek@redhat.com>; Leif Lindholm
> > > > > <leif@nuviainc.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> > > > > Wang, Nickle (HPS SW) <nickle.wang@hpe.com>; O'Hanley, Peter
> > > > > (EXL)
> > > > <peter.ohanley@hpe.com>
> > > > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > > > >
> > > > > Mike, response in below. Also v9 is sent.
> > > > >
> > > > > Thanks for review this in detail.
> > > > > Abner
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> > > > > > Sent: Monday, December 21, 2020 3:43 AM
> > > > > > To: Chang, Abner (HPS SW/FW Technologist)
> > <abner.chang@hpe.com>;
> > > > > > devel@edk2.groups.io; Kinney, Michael D
> > > > > > <michael.d.kinney@intel.com>
> > > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> > > > Laszlo
> > > > > > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>;
> > > > > > Liming Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > > > > > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> > > > <peter.ohanley@hpe.com>
> > > > > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > > > > >
> > > > > > Hi Abner,
> > > > > >
> > > > > > The include path in the [Include.Common.Private] section
> > > > > > should not be in the same file path as the [Include] section.
> > > > > > This allows private include files to be accessed.
> > > > > >
> > > > > > Instead, you should create a new top level package directory
> > > > > > called PrivateInclude and put all non-public include content
> > > > > > in that directory.  The UnitTestFrameworkPkg is a good
> > > > > > reference that demonstrates this design pattern.
> > > > > Yes, this looks better. All Crt header files and CrtLib.h will
> > > > > be moved to
> > > > under PrivateInclude.
> > > > > >
> > > > > > The library class name CrtLib is very generic and the library
> > > > > > class is in the public includes so it is exported as a public
> > > > > > interface from the
> > > > RedfishPkg.
> > > > > > This CrtLib implementation appears to be tuned to support the
> > > > > > dependencies for the jansson submodule.  I recommend changing
> > > > > > the library class name and the library instance name so it is
> > > > > > clear that this CrtLib is only for jansson.
> > > > > > Perhaps 'JanssonCrtLib'.
> > > > > The CrtLib is not used by jannson lib, libredfish as well
> > > > > (https://github.com/DMTF/libredfish). So JanssonCrtLib is not a
> > > > > good
> > > > naming, will keep that as it.
> > > > >
> > > > > >Also, does this library class (CrtLib.h) need to be  exported
> > > > > >from RedfishPkg as a public interface?
> > > > > No, it will be moved to under PrivateInclude.
> > > > >
> > > > > If CrtLib.h is defined as a private library and CrtLib.h is in
> > > > > PrivateInclude which is not exposed to other packages, then why
> > > > > do we
> > > > care about the naming of CrtLib is too generic? Which is the
> > > > private library under RedfishPkg.
> > > > > Move those header files to under PrivateInclude seems clear to
> > > > > people that CrtLib is only for RedfishPkg. I think we don’t have
> > > > > to change the
> > > > naming in this case.
> > > >
> > > > In general I agree, but there is one place where the lib mapping
> > > > has wider scope, and that is in a platform DSC file.  The lib
> > > > class name CrtLib has to be listed in the DSC file.  If more than
> > > > one package defines a library class called CrtLib, then the last
> > > > mapping used will be used for all modules if it is listed in the
> > > > [LibraryClasses] section of the DSC file.  The only was to handle
> > > > this type of name collision is to use a module scoped library
> > > > mapping in the <LibraryClasses> section of each individual module
> > > > that uses a CrtLib.  I am only trying to avoid the potential for
> > > > future name collisions
> > for a library class/instance that is specific to the JSON use case.
> > > >
> > > > We recognize that several modules in edk2 have this type of gasket
> > > > between an EDK II env and a standard C lib.  We may find a we to
> > > > implement a more generic gasket and retire the use of the
> > > > implementation specific ones.  This is another reason to use an
> > > > implementation specific name in this use case and be able to
> > > > introduce a
> > more generic name in the future.
> > > >
> > > > Another comment below to see if there is a way to avoid
> > > > introducing a CrtLib class.
> > > >
> > > > >
> > > > > >
> > > > > > * RedfishPkg/Include/Library/CrtLib.h
> > > > > >   + Remove reference to MDE_CPU_IA64.  This has been retired
> > > > > > from the
> > > > > > edk2 repo.
> > > > > Ok.
> > > > > >   + I do see one remaining reference to this in the CryptoPkg
> > > > > > that need to
> > > > be
> > > > > >     removed.
> > > > > >
> > > > > > * RedfishPkg/Include/Library/JsonLib.h
> > > > > >   + Some of the function descriptions are very brief and I can not tell
> > > > > >     how to use the service from the description and more importantly,
> I
> > > > > >     would not know how to write a unit test to verify the
> > > > > > expected
> > > > behavior.
> > > > > >     Since these are a set of public APIs from this package that you
> > > > > >     expect modules/libs from outside of RedfishPkg to use, then all of
> > > > > >     these public APIs must be fully documented.
> > > > > Most of the API in JsonLib are wrappers of native jansson APIs.
> > > > > We can mention the URL to jansson API reference document in file
> header.
> > > > > INVALID URI REMOVED
> > > > 3A__jansson.readthedo
> > > > >
> > > >
> >
> cs.io_en_2.13_index.html&d=DwIGaQ&c=C5b8zRQO1miGmBeVZ2LFWg&r=_
> > > > SN6FZBN4
> > > > > Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=vd-
> > > > uPz864czoYA5C7YaPgqXfdPjv4c5kB
> > > > >
> > zTwV60NvNQ&s=acVzN3x4yND0jkg44bJ48ZkyPyugjwsYvUGdD4nJL74&e=
> > > > > I will review all function header again,  please check v9 patch later.
> > > > >
> > > > > >   + Are there any of these APIs that are not really needed by
> > modules/libs
> > > > > >     outside the RedfishPkg?  It would be better to remove APIs that
> are
> > > > > >     not needed outside RedfishPkg from this public include file.
> > > > > >   + A couple examples that stood out are:
> > > > > >     JsonDecreaseReference()
> > > > > >     JsonIncreaseReference()
> > > > > >     JsonObjectIterator()
> > > > > >     JsonObjectIteratorValue()
> > > > > >     JsonObjectIteratorNext()
> > > > > The native jansson APIs of all above wrappers are used in libredfish.
> > > > > So that is the potential use case for other JSON applications.
> > > >
> > > > Can you provide a pointer to an example of this use case?
> > > >
> > > > I am trying to make sure we are doing the right design of
> > > > RedfishPkg with respect to CrtLib for this use case.  Are these
> > > > are JSON apps that do not use JsonLib, but are expected to build
> > > > in an EDK II build environment?  Would these JSON apps require
> > > > more standard C lib services than the CrtLib used to build
> > > > JsonLib?  Where would those
> > additional C lib services come from?
> > > >
> > > > There is a standard C lib in edk2-libc.  Would that be required to
> > > > build the JSON apps.
> > > >
> > > > In other uses of submodules with C lib dependencies, a new CrtLib
> > > > like lib class was not added and instead the support was added
> > > > directly to the EDK II wrapper APIs. Some examples are:
> > > >   * MdeModulePkg\Library\BrotliCustomDecompressLib
> > > >   * MdeModulePkg\Universal\RegularExpressionDxe
> > > >
> > > > If there are use cases where the CrtLib will be added to an INF
> > > > outside the RedfishPkg, then I still think the name needs to be
> > > > changed to be specific to the JSON use case.
> > > CrtLib should not be added to INF outside RedfishPkg, but it will be
> > > added
> > to platform DSC file.
> > > >
> > > > If the use of CrtLib is only within the RedfishPkg, then the
> > > > Library Class can be declared in a [LibraryClasses.Common.Private]
> > > > section of the DEC file.  See UnitTestFrameworkPkg for examples
> > > Because CrtLib, JsonLib and other RedfishPkg modules will be
> > > included in
> > other package dsc file (e.g. EmulatorPkg).
> > > What if in EmulatorPkg one CrtLib libraryclass maps to the private
> > > one under RedfishPkg, but another with the same name is the public
> > > library
> > provided by other package (e.g. MdeModulePkg) and used by modules
> > other than RedfishPkg?
> > > Is edk2 build tool that smart to link the private CrtLib for
> > > RedfishPkg modules and all other non RedfishPkg modules are linked
> > > with
> > the public one?
> > >
> > > Abner
> > >
> >
> > No.  Not that smart.
> >
> > There are global lib class -> lib instance mapping in [LibraryClasses]
> > sections of DSC files.  If the same lib class is listed more than
> > once, then the last mapping wins.
> >
> > If a module scoped <LibraryClasses> section is used, then then last
> > mapping in the scope of that module wins.
> >
> > If the same lib class name is declared by more than one package used
> > in a platform DSC, then the only remediation is to remove use of the
> > global [LibraryClasses] section and use a module scope
> > <LibraryClasses> section in every module that depends on that lib class
> name.
> Ok, I will rename it to RedfishCrtLib.
> 
> Abner
> >
> > > >
> > > > >
> > > > > >   + JsonDumpString()
> > > > > >     func header params do not match func prototype
> > > > > >     Does not describe who allocates the return buffer and who
> > > > > >     is responsible for freeing the buffer returned.
> > > > > >     What do Flags do?  What is the difference between this
> > > > > >     function and JsonToText()?
> > > > > In JsonToText, it only converts JSON array and object to text.
> > > > > We could just remove this for now because I don’t really
> > > > > remember the reason
> > > > to create this function, it has been a while. Seems to me it is
> > > > fine to remove this function.
> > > > >
> > > > > >   + JsonLoadBuffer() - Error param description missing
> > > > > >   + JsonArrayGetValue() does not describe the conditions NULL
> > > > > >     is returned.
> > > > > >   + JsonObjectGetKeys().  Return type is CHAR8**.  What
> > > > > >     function need to be called to free the array?
> > > > > All above addressed.
> > > > >
> > > > > >   + JsonValueGetAsciiString() and JsonValueGetUnicodeString()
> > > > > >     are asymmetric.  The Ascii one states that change will
> > > > > >     modify the original string.  The Unicode one says that the
> > > > > >     caller needs to free the returned string.  Any reason we
> > > > > >     can not make them symmetric?
> > > > > Jansson doesn't support Unicode string. The memory of the
> > > > > unicode string returned by JsonValueGetUnicodeString Is
> > > > > allocated by
> > > > UTF8StrToUCS2().
> > > > >  > Also, if Ascii one should
> > > > > >     not be modified, why isn’t the return type CONST?
> > > > > Yes. will do that.
> > > > >
> > > > > >
> > > > > > * RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
> > > > > >   + Change [Sources.common] to [Sources]
> > > > > done
> > > > > > * RedfishPkg/Library/CrtLib/CrtLib.inf
> > > > > >   + Does this lib instance really depend on MdeModulePkg?
> > > > > Yes, SortLib
> > > > > >   + Have you reviewed the module types this lib instance is
> > > > > >     compatible with?  Why does it need to support DXE_CORE?
> > > > > >     Are their JsonLib use cases for DXE_RUNTIME and SMM drivers?
> > > > > >     If there are SMM use cases, then why is MM excluded?
> > > > > Will just keep DXE_DRVER, UEFI_APPLICATION and UEFI_DRIVER.
> > These
> > > > are
> > > > > module types have potential use cases I can think of now.
> > > > > >
> > > > > > * RedfishPkg/Library/JsonLib
> > > > > >   + Readme.rst still has references to JanssonJsonLibMapping.h
> > > > > done
> > > > > >   + Have you reviewed the module types this lib instance is
> > > > > >     compatible with?  Why does it need to support DXE_CORE?
> > > > > >     Are their JsonLib use cases for DXE_RUNTIME and SMM drivers?
> > > > > >     If there are SMM use cases, then why is MM excluded?
> > > > > Answered above
> > > > > >   + JsonLib.inf has a [BuildOptions] section that disables
> > > > > > some warnings
> > > > that
> > > > > >     make me concerned that this library may have some issues
> > > > > > with 32-bit builds.
> > > > > >     Have you fully validated all 32-bit CPU builds and
> > > > > > validated the
> > > > functionality
> > > > > >     of all services from the JsonLib for all ranges of input values?
> > > > > Yes, IA32 build is validated. But not validating on the
> > > > > functionalities IA32. I don't have that use case, we can get
> > > > > back to fix the
> > > > issue if any when  someone runs this on IA32.
> > > >
> > > > I did some local testing with VS2019.  I found that a much smaller
> > > > set of warning disables are required for IA32 and X64 and the set
> > > > is different for
> > > > IA32 and X64.  Can you review what is needed and minimize the
> > > > required set of each supported arch?
> > > >
> > > >   MSFT:*_*_X64_CC_FLAGS = /wd4244 /wd4090 /wd4334
> > > > /DHAVE_CONFIG_H=1 /U_WIN32 /UWIN64 /U_MSC_VER
> > > >   MSFT:*_*_IA32_CC_FLAGS = /wd4244 /wd4090
> /DHAVE_CONFIG_H=1
> > > > /U_WIN32 /UWIN64 /U_MSC_VER
> > > >
> > > > > >
> > > > > > * RedfishLibs.dsc.inc
> > > > > >   It is better to add [LibraryClasses] statement to this file, so it
> > > > > >   can be include anywhere in a DSC file.  With current
> implementation
> > > > > >   if it is not included within a [LibraryClasses] section, it will
> > > > > >   generate a build error.
> > > > > I will separate this to another set of patch. Don’t mix up with
> > > > > jansson edk2
> > > > port.
> > > > >
> > > > > >You might also consider changing the name
> > > > > >   of this file in case you want to add more than just lib mappings
> > > > > >   to this file in the future.  See UnitTestFramworkPkg for examples.
> > > > > >
> > > > > > Best regards,
> > > > > >
> > > > > > Mike
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Abner Chang <abner.chang@hpe.com>
> > > > > > > Sent: Thursday, December 17, 2020 5:19 AM
> > > > > > > To: devel@edk2.groups.io
> > > > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > > > > <Bret.Barkelew@microsoft.com>; Andrew Fish
> > > > > > > <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Leif
> > > > > > > Lindholm <leif@nuviainc.com>; Kinney, Michael D
> > > > > > > <michael.d.kinney@intel.com>; Liming Gao
> > > > > > > <gaoliming@byosoft.com.cn>; Nickle Wang
> > <nickle.wang@hpe.com>;
> > > > > > Peter
> > > > > > > O'Hanley <peter.ohanley@hpe.com>
> > > > > > > Subject: [PATCH v8 0/6] jansson edk2 port
> > > > > > >
> > > > > > > In v8, - Assigne patch file order
> > > > > > >        - Add Acked-by tags
> > > > > > > In v7, - Remove C RTC header files to under
> > [Include.Common.Private]
> > > > > > >          in RedfishPkg.dec.
> > > > > > >        - address comments given by Mike Kinney.
> > > > > > > In v6, Remove JanssonJsonMapping.h In v5, move
> > > > > > > BaseUcs2Utf8Lib to under RedfishPkg.
> > > > > > > In v4,
> > > > > > >        - Address review comments
> > > > > > >        - Seperate CRT functions to a individule library CrtLib under
> > > > > > >          RedfishPkg.
> > > > > > >        - Seperate UCS2-UTF8 functions to a individule library
> > > > > > >          BaseUcs2Utf8Lib under MdeModulePkg.
> > > > > > >
> > > > > > > In v3, Add jansson library as the required submoudle in
> > > > > > >        CiSettings.py for CI test.
> > > > > > > In v2, JsonLib is moved to under RedfishPkg.
> > > > > > >
> > > > > > > edk2 JSON library is based on jansson open source
> > > > > > > (https://github.com/akheron/jansson) and wrapped as an edk2
> > library.
> > > > > > > edk2 JsonLib will be used by edk2 Redfish feature drivers
> > > > > > > (not contributed yet) and the edk2 port of libredfish
> > > > > > > library (not contributed yet) based on DMTF GitHub
> > > > > > > (https://github.com/DMTF/libredfish).
> > > > > > >
> > > > > > > Jansson is licensed under the MIT license(refer to
> > > > > > > ReadMe.rst under
> > > > edk2).
> > > > > > > It is used in production and its API is stable. In
> > > > > > > UEFI/EDKII environment, Redfish project consumes jansson to
> > > > > > > achieve JSON
> > > > > > operations.
> > > > > > >
> > > > > > > * Jansson version on edk2: 2.13.1
> > > > > > >
> > > > > > > * EDKII jansson library wrapper:
> > > > > > >    - JsonLib.h:
> > > > > > >      This is the denifitions of EDKII JSON APIs which are mapped to
> > > > > > >      jannson funcitons accordingly.
> > > > > > >
> > > > > > >    - JanssonJsonLibMapping.h:
> > > > > > >      This is the wrapper file to map funcitons and definitions used in
> > > > > > >      native jannson applications to edk2 JsonLib. This avoids the
> > > > > > >      modifications on native jannson applications to be built under
> > > > > > >      edk2 environment.
> > > > > > >
> > > > > > > *Known issue:
> > > > > > >   Build fail with jansson/src/load.c, overrride and add code in load.c
> > > > > > >   to conditionally use stdin according to HAVE_UNISTD_H macro.
> > > > > > >   The PR is submitted to jansson open source community.
> > > > > > >   https://github.com/akheron/jansson/pull/558
> > > > > > >
> > > > > > > Signed-off-by: Abner Chang <abner.chang@hpe.com>
> > > > > > >
> > > > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > > > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > > > > > > Cc: Andrew Fish <afish@apple.com>
> > > > > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > > > > Cc: Leif Lindholm <leif@nuviainc.com>
> > > > > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > > > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > > > > > Cc: Nickle Wang <nickle.wang@hpe.com>
> > > > > > > Cc: Peter O'Hanley <peter.ohanley@hpe.com>
> > > > > > >
> > > > > > > Abner Chang (6):
> > > > > > >   RedfishPkg/Ucs2Utf8lib: UCS2 to UFT8 manipulation library
> > > > > > >   edk2: jansson submodule for edk2 JSON library
> > > > > > >   RedfishPkg/CrtLib: C runtime library
> > > > > > >   RedfishPkg/library: EDK2 port of jansson library
> > > > > > >   RedfishPkg: Add EDK2 port of jansson library to build
> > > > > > >   .pytool: Add required submodule for JsonLib
> > > > > > >
> > > > > > >  .gitmodules                                   |    3 +
> > > > > > >  .pytool/CISettings.py                         |    2 +
> > > > > > >  ReadMe.rst                                    |    1 +
> > > > > > >  RedfishPkg/Include/Crt/assert.h               |   16 +
> > > > > > >  RedfishPkg/Include/Crt/errno.h                |   16 +
> > > > > > >  RedfishPkg/Include/Crt/limits.h               |   16 +
> > > > > > >  RedfishPkg/Include/Crt/math.h                 |   16 +
> > > > > > >  RedfishPkg/Include/Crt/stdarg.h               |   15 +
> > > > > > >  RedfishPkg/Include/Crt/stddef.h               |   16 +
> > > > > > >  RedfishPkg/Include/Crt/stdio.h                |   15 +
> > > > > > >  RedfishPkg/Include/Crt/stdlib.h               |   16 +
> > > > > > >  RedfishPkg/Include/Crt/string.h               |   16 +
> > > > > > >  RedfishPkg/Include/Crt/sys/time.h             |   15 +
> > > > > > >  RedfishPkg/Include/Crt/sys/types.h            |   15 +
> > > > > > >  RedfishPkg/Include/Crt/time.h                 |   15 +
> > > > > > >  RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h  |   61 +
> > > > > > >  RedfishPkg/Include/Library/CrtLib.h           |  191 +++
> > > > > > >  RedfishPkg/Include/Library/JsonLib.h          |  763 +++++++++++
> > > > > > >  .../Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c |  421 +++++++
> > > > > > >  .../BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf       |   31 +
> > > > > > >  RedfishPkg/Library/CrtLib/CrtLib.c            |  595 +++++++++
> > > > > > >  RedfishPkg/Library/CrtLib/CrtLib.inf          |   38 +
> > > > > > >  RedfishPkg/Library/JsonLib/JsonLib.c          |  964
> ++++++++++++++
> > > > > > >  RedfishPkg/Library/JsonLib/JsonLib.inf        |   89 ++
> > > > > > >  RedfishPkg/Library/JsonLib/Readme.rst         |   40 +
> > > > > > >  RedfishPkg/Library/JsonLib/jansson            |    1 +
> > > > > > >  RedfishPkg/Library/JsonLib/jansson_config.h   |   41 +
> > > > > > >  .../Library/JsonLib/jansson_private_config.h  |   19 +
> > > > > > >  RedfishPkg/Library/JsonLib/load.c             | 1111
> > +++++++++++++++++
> > > > > > >  RedfishPkg/RedfishLibs.dsc.inc                |    3 +
> > > > > > >  RedfishPkg/RedfishPkg.ci.yaml                 |   25 +
> > > > > > >  RedfishPkg/RedfishPkg.dec                     |   25 +
> > > > > > >  RedfishPkg/RedfishPkg.dsc                     |    3 +
> > > > > > >  33 files changed, 4614 insertions(+)  create mode 100644
> > > > > > > RedfishPkg/Include/Crt/assert.h  create mode
> > > > > > > 100644 RedfishPkg/Include/Crt/errno.h  create mode 100644
> > > > > > > RedfishPkg/Include/Crt/limits.h  create mode 100644
> > > > > > > RedfishPkg/Include/Crt/math.h  create mode 100644
> > > > > > > RedfishPkg/Include/Crt/stdarg.h  create mode 100644
> > > > > > > RedfishPkg/Include/Crt/stddef.h  create mode 100644
> > > > > > > RedfishPkg/Include/Crt/stdio.h  create mode 100644
> > > > > > > RedfishPkg/Include/Crt/stdlib.h  create mode 100644
> > > > > > > RedfishPkg/Include/Crt/string.h  create mode 100644
> > > > > > > RedfishPkg/Include/Crt/sys/time.h  create mode 100644
> > > > > > > RedfishPkg/Include/Crt/sys/types.h
> > > > > > >  create mode 100644 RedfishPkg/Include/Crt/time.h  create
> > > > > > > mode
> > > > > > > 100644 RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h
> > > > > > >  create mode 100644 RedfishPkg/Include/Library/CrtLib.h
> > > > > > >  create mode 100644 RedfishPkg/Include/Library/JsonLib.h
> > > > > > >  create mode 100644
> > > > > > > RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c
> > > > > > >  create mode 100644
> > > > > > > RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
> > > > > > >  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.c
> > > > > > >  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.inf
> > > > > > >  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.c
> > > > > > >  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.inf
> > > > > > >  create mode 100644 RedfishPkg/Library/JsonLib/Readme.rst
> > > > > > >  create mode 160000 RedfishPkg/Library/JsonLib/jansson
> > > > > > >  create mode 100644
> > > > > > > RedfishPkg/Library/JsonLib/jansson_config.h
> > > > > > >  create mode 100644
> > > > > > > RedfishPkg/Library/JsonLib/jansson_private_config.h
> > > > > > >  create mode 100644 RedfishPkg/Library/JsonLib/load.c
> > > > > > >
> > > > > > > --
> > > > > > > 2.17.1
> > > > >
> > >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69416): https://edk2.groups.io/g/devel/message/69416
Mute This Topic: https://groups.io/mt/79052636/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Abner Chang
> Sent: Wednesday, December 23, 2020 1:02 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
> Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming Gao <gaoliming@byosoft.com.cn>; Wang, Nickle
> (HPS SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL) <peter.ohanley@hpe.com>
> Subject: Re: [edk2-devel] [PATCH v8 0/6] jansson edk2 port
> 
> Hi Mike, v10 patch set sent.
> BTW, why don’t you put those private libraries in UniTestFrameworkPkg to under /PrivateLibrary which is similar to
> /PrivateInclude?

That is a good question.  We had not considered that before because it does not change
the visibility of the library instance (we have to support listing these in platform 
DSC files).  Using PrivateInclude actually prevents a module/lib from other packages
from using those include files because the include paths passed into the compiler
do not contain private include paths.

From a self-documenting perspective using a PrivateLibrary directory name would
make it obvious that the library is not intended to be used by any components 
from other packages.

Mike

> 
> Abner
> 
> > -----Original Message-----
> > From: Chang, Abner (HPS SW/FW Technologist)
> > Sent: Wednesday, December 23, 2020 3:54 PM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
> > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>; Laszlo
> > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming Gao
> > <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL) <peter.ohanley@hpe.com>
> > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> >
> >
> >
> > > -----Original Message-----
> > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> > > Sent: Wednesday, December 23, 2020 2:10 PM
> > > To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> > > devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>; Laszlo
> > > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming
> > > Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> > <peter.ohanley@hpe.com>
> > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > >
> > > Hi Abner,
> > >
> > > Response below.
> > >
> > > Mike
> > >
> > >
> > > > -----Original Message-----
> > > > From: Chang, Abner (HPS SW/FW Technologist)
> > <abner.chang@hpe.com>
> > > > Sent: Tuesday, December 22, 2020 8:40 PM
> > > > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > > > devel@edk2.groups.io
> > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> > Laszlo
> > > > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming
> > > > Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > > > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> > > <peter.ohanley@hpe.com>
> > > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> > > > > Sent: Wednesday, December 23, 2020 2:14 AM
> > > > > To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> > > > > devel@edk2.groups.io; Kinney, Michael D
> > > > > <michael.d.kinney@intel.com>
> > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> > > Laszlo
> > > > > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>;
> > > > > Liming Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > > > > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> > > <peter.ohanley@hpe.com>
> > > > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > > > >
> > > > > Hi Abner,
> > > > >
> > > > > A few comments below.
> > > > >
> > > > > Best regards,
> > > > >
> > > > > Mike
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Chang, Abner (HPS SW/FW Technologist)
> > > <abner.chang@hpe.com>
> > > > > > Sent: Monday, December 21, 2020 12:17 AM
> > > > > > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > > > > > devel@edk2.groups.io
> > > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> > > > > > Laszlo Ersek <lersek@redhat.com>; Leif Lindholm
> > > > > > <leif@nuviainc.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> > > > > > Wang, Nickle (HPS SW) <nickle.wang@hpe.com>; O'Hanley, Peter
> > > > > > (EXL)
> > > > > <peter.ohanley@hpe.com>
> > > > > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > > > > >
> > > > > > Mike, response in below. Also v9 is sent.
> > > > > >
> > > > > > Thanks for review this in detail.
> > > > > > Abner
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> > > > > > > Sent: Monday, December 21, 2020 3:43 AM
> > > > > > > To: Chang, Abner (HPS SW/FW Technologist)
> > > <abner.chang@hpe.com>;
> > > > > > > devel@edk2.groups.io; Kinney, Michael D
> > > > > > > <michael.d.kinney@intel.com>
> > > > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > > > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> > > > > Laszlo
> > > > > > > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>;
> > > > > > > Liming Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > > > > > > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> > > > > <peter.ohanley@hpe.com>
> > > > > > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > > > > > >
> > > > > > > Hi Abner,
> > > > > > >
> > > > > > > The include path in the [Include.Common.Private] section
> > > > > > > should not be in the same file path as the [Include] section.
> > > > > > > This allows private include files to be accessed.
> > > > > > >
> > > > > > > Instead, you should create a new top level package directory
> > > > > > > called PrivateInclude and put all non-public include content
> > > > > > > in that directory.  The UnitTestFrameworkPkg is a good
> > > > > > > reference that demonstrates this design pattern.
> > > > > > Yes, this looks better. All Crt header files and CrtLib.h will
> > > > > > be moved to
> > > > > under PrivateInclude.
> > > > > > >
> > > > > > > The library class name CrtLib is very generic and the library
> > > > > > > class is in the public includes so it is exported as a public
> > > > > > > interface from the
> > > > > RedfishPkg.
> > > > > > > This CrtLib implementation appears to be tuned to support the
> > > > > > > dependencies for the jansson submodule.  I recommend changing
> > > > > > > the library class name and the library instance name so it is
> > > > > > > clear that this CrtLib is only for jansson.
> > > > > > > Perhaps 'JanssonCrtLib'.
> > > > > > The CrtLib is not used by jannson lib, libredfish as well
> > > > > > (https://github.com/DMTF/libredfish). So JanssonCrtLib is not a
> > > > > > good
> > > > > naming, will keep that as it.
> > > > > >
> > > > > > >Also, does this library class (CrtLib.h) need to be  exported
> > > > > > >from RedfishPkg as a public interface?
> > > > > > No, it will be moved to under PrivateInclude.
> > > > > >
> > > > > > If CrtLib.h is defined as a private library and CrtLib.h is in
> > > > > > PrivateInclude which is not exposed to other packages, then why
> > > > > > do we
> > > > > care about the naming of CrtLib is too generic? Which is the
> > > > > private library under RedfishPkg.
> > > > > > Move those header files to under PrivateInclude seems clear to
> > > > > > people that CrtLib is only for RedfishPkg. I think we don’t have
> > > > > > to change the
> > > > > naming in this case.
> > > > >
> > > > > In general I agree, but there is one place where the lib mapping
> > > > > has wider scope, and that is in a platform DSC file.  The lib
> > > > > class name CrtLib has to be listed in the DSC file.  If more than
> > > > > one package defines a library class called CrtLib, then the last
> > > > > mapping used will be used for all modules if it is listed in the
> > > > > [LibraryClasses] section of the DSC file.  The only was to handle
> > > > > this type of name collision is to use a module scoped library
> > > > > mapping in the <LibraryClasses> section of each individual module
> > > > > that uses a CrtLib.  I am only trying to avoid the potential for
> > > > > future name collisions
> > > for a library class/instance that is specific to the JSON use case.
> > > > >
> > > > > We recognize that several modules in edk2 have this type of gasket
> > > > > between an EDK II env and a standard C lib.  We may find a we to
> > > > > implement a more generic gasket and retire the use of the
> > > > > implementation specific ones.  This is another reason to use an
> > > > > implementation specific name in this use case and be able to
> > > > > introduce a
> > > more generic name in the future.
> > > > >
> > > > > Another comment below to see if there is a way to avoid
> > > > > introducing a CrtLib class.
> > > > >
> > > > > >
> > > > > > >
> > > > > > > * RedfishPkg/Include/Library/CrtLib.h
> > > > > > >   + Remove reference to MDE_CPU_IA64.  This has been retired
> > > > > > > from the
> > > > > > > edk2 repo.
> > > > > > Ok.
> > > > > > >   + I do see one remaining reference to this in the CryptoPkg
> > > > > > > that need to
> > > > > be
> > > > > > >     removed.
> > > > > > >
> > > > > > > * RedfishPkg/Include/Library/JsonLib.h
> > > > > > >   + Some of the function descriptions are very brief and I can not tell
> > > > > > >     how to use the service from the description and more importantly,
> > I
> > > > > > >     would not know how to write a unit test to verify the
> > > > > > > expected
> > > > > behavior.
> > > > > > >     Since these are a set of public APIs from this package that you
> > > > > > >     expect modules/libs from outside of RedfishPkg to use, then all of
> > > > > > >     these public APIs must be fully documented.
> > > > > > Most of the API in JsonLib are wrappers of native jansson APIs.
> > > > > > We can mention the URL to jansson API reference document in file
> > header.
> > > > > > INVALID URI REMOVED
> > > > > 3A__jansson.readthedo
> > > > > >
> > > > >
> > >
> > cs.io_en_2.13_index.html&d=DwIGaQ&c=C5b8zRQO1miGmBeVZ2LFWg&r=_
> > > > > SN6FZBN4
> > > > > > Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=vd-
> > > > > uPz864czoYA5C7YaPgqXfdPjv4c5kB
> > > > > >
> > > zTwV60NvNQ&s=acVzN3x4yND0jkg44bJ48ZkyPyugjwsYvUGdD4nJL74&e=
> > > > > > I will review all function header again,  please check v9 patch later.
> > > > > >
> > > > > > >   + Are there any of these APIs that are not really needed by
> > > modules/libs
> > > > > > >     outside the RedfishPkg?  It would be better to remove APIs that
> > are
> > > > > > >     not needed outside RedfishPkg from this public include file.
> > > > > > >   + A couple examples that stood out are:
> > > > > > >     JsonDecreaseReference()
> > > > > > >     JsonIncreaseReference()
> > > > > > >     JsonObjectIterator()
> > > > > > >     JsonObjectIteratorValue()
> > > > > > >     JsonObjectIteratorNext()
> > > > > > The native jansson APIs of all above wrappers are used in libredfish.
> > > > > > So that is the potential use case for other JSON applications.
> > > > >
> > > > > Can you provide a pointer to an example of this use case?
> > > > >
> > > > > I am trying to make sure we are doing the right design of
> > > > > RedfishPkg with respect to CrtLib for this use case.  Are these
> > > > > are JSON apps that do not use JsonLib, but are expected to build
> > > > > in an EDK II build environment?  Would these JSON apps require
> > > > > more standard C lib services than the CrtLib used to build
> > > > > JsonLib?  Where would those
> > > additional C lib services come from?
> > > > >
> > > > > There is a standard C lib in edk2-libc.  Would that be required to
> > > > > build the JSON apps.
> > > > >
> > > > > In other uses of submodules with C lib dependencies, a new CrtLib
> > > > > like lib class was not added and instead the support was added
> > > > > directly to the EDK II wrapper APIs. Some examples are:
> > > > >   * MdeModulePkg\Library\BrotliCustomDecompressLib
> > > > >   * MdeModulePkg\Universal\RegularExpressionDxe
> > > > >
> > > > > If there are use cases where the CrtLib will be added to an INF
> > > > > outside the RedfishPkg, then I still think the name needs to be
> > > > > changed to be specific to the JSON use case.
> > > > CrtLib should not be added to INF outside RedfishPkg, but it will be
> > > > added
> > > to platform DSC file.
> > > > >
> > > > > If the use of CrtLib is only within the RedfishPkg, then the
> > > > > Library Class can be declared in a [LibraryClasses.Common.Private]
> > > > > section of the DEC file.  See UnitTestFrameworkPkg for examples
> > > > Because CrtLib, JsonLib and other RedfishPkg modules will be
> > > > included in
> > > other package dsc file (e.g. EmulatorPkg).
> > > > What if in EmulatorPkg one CrtLib libraryclass maps to the private
> > > > one under RedfishPkg, but another with the same name is the public
> > > > library
> > > provided by other package (e.g. MdeModulePkg) and used by modules
> > > other than RedfishPkg?
> > > > Is edk2 build tool that smart to link the private CrtLib for
> > > > RedfishPkg modules and all other non RedfishPkg modules are linked
> > > > with
> > > the public one?
> > > >
> > > > Abner
> > > >
> > >
> > > No.  Not that smart.
> > >
> > > There are global lib class -> lib instance mapping in [LibraryClasses]
> > > sections of DSC files.  If the same lib class is listed more than
> > > once, then the last mapping wins.
> > >
> > > If a module scoped <LibraryClasses> section is used, then then last
> > > mapping in the scope of that module wins.
> > >
> > > If the same lib class name is declared by more than one package used
> > > in a platform DSC, then the only remediation is to remove use of the
> > > global [LibraryClasses] section and use a module scope
> > > <LibraryClasses> section in every module that depends on that lib class
> > name.
> > Ok, I will rename it to RedfishCrtLib.
> >
> > Abner
> > >
> > > > >
> > > > > >
> > > > > > >   + JsonDumpString()
> > > > > > >     func header params do not match func prototype
> > > > > > >     Does not describe who allocates the return buffer and who
> > > > > > >     is responsible for freeing the buffer returned.
> > > > > > >     What do Flags do?  What is the difference between this
> > > > > > >     function and JsonToText()?
> > > > > > In JsonToText, it only converts JSON array and object to text.
> > > > > > We could just remove this for now because I don’t really
> > > > > > remember the reason
> > > > > to create this function, it has been a while. Seems to me it is
> > > > > fine to remove this function.
> > > > > >
> > > > > > >   + JsonLoadBuffer() - Error param description missing
> > > > > > >   + JsonArrayGetValue() does not describe the conditions NULL
> > > > > > >     is returned.
> > > > > > >   + JsonObjectGetKeys().  Return type is CHAR8**.  What
> > > > > > >     function need to be called to free the array?
> > > > > > All above addressed.
> > > > > >
> > > > > > >   + JsonValueGetAsciiString() and JsonValueGetUnicodeString()
> > > > > > >     are asymmetric.  The Ascii one states that change will
> > > > > > >     modify the original string.  The Unicode one says that the
> > > > > > >     caller needs to free the returned string.  Any reason we
> > > > > > >     can not make them symmetric?
> > > > > > Jansson doesn't support Unicode string. The memory of the
> > > > > > unicode string returned by JsonValueGetUnicodeString Is
> > > > > > allocated by
> > > > > UTF8StrToUCS2().
> > > > > >  > Also, if Ascii one should
> > > > > > >     not be modified, why isn’t the return type CONST?
> > > > > > Yes. will do that.
> > > > > >
> > > > > > >
> > > > > > > * RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
> > > > > > >   + Change [Sources.common] to [Sources]
> > > > > > done
> > > > > > > * RedfishPkg/Library/CrtLib/CrtLib.inf
> > > > > > >   + Does this lib instance really depend on MdeModulePkg?
> > > > > > Yes, SortLib
> > > > > > >   + Have you reviewed the module types this lib instance is
> > > > > > >     compatible with?  Why does it need to support DXE_CORE?
> > > > > > >     Are their JsonLib use cases for DXE_RUNTIME and SMM drivers?
> > > > > > >     If there are SMM use cases, then why is MM excluded?
> > > > > > Will just keep DXE_DRVER, UEFI_APPLICATION and UEFI_DRIVER.
> > > These
> > > > > are
> > > > > > module types have potential use cases I can think of now.
> > > > > > >
> > > > > > > * RedfishPkg/Library/JsonLib
> > > > > > >   + Readme.rst still has references to JanssonJsonLibMapping.h
> > > > > > done
> > > > > > >   + Have you reviewed the module types this lib instance is
> > > > > > >     compatible with?  Why does it need to support DXE_CORE?
> > > > > > >     Are their JsonLib use cases for DXE_RUNTIME and SMM drivers?
> > > > > > >     If there are SMM use cases, then why is MM excluded?
> > > > > > Answered above
> > > > > > >   + JsonLib.inf has a [BuildOptions] section that disables
> > > > > > > some warnings
> > > > > that
> > > > > > >     make me concerned that this library may have some issues
> > > > > > > with 32-bit builds.
> > > > > > >     Have you fully validated all 32-bit CPU builds and
> > > > > > > validated the
> > > > > functionality
> > > > > > >     of all services from the JsonLib for all ranges of input values?
> > > > > > Yes, IA32 build is validated. But not validating on the
> > > > > > functionalities IA32. I don't have that use case, we can get
> > > > > > back to fix the
> > > > > issue if any when  someone runs this on IA32.
> > > > >
> > > > > I did some local testing with VS2019.  I found that a much smaller
> > > > > set of warning disables are required for IA32 and X64 and the set
> > > > > is different for
> > > > > IA32 and X64.  Can you review what is needed and minimize the
> > > > > required set of each supported arch?
> > > > >
> > > > >   MSFT:*_*_X64_CC_FLAGS = /wd4244 /wd4090 /wd4334
> > > > > /DHAVE_CONFIG_H=1 /U_WIN32 /UWIN64 /U_MSC_VER
> > > > >   MSFT:*_*_IA32_CC_FLAGS = /wd4244 /wd4090
> > /DHAVE_CONFIG_H=1
> > > > > /U_WIN32 /UWIN64 /U_MSC_VER
> > > > >
> > > > > > >
> > > > > > > * RedfishLibs.dsc.inc
> > > > > > >   It is better to add [LibraryClasses] statement to this file, so it
> > > > > > >   can be include anywhere in a DSC file.  With current
> > implementation
> > > > > > >   if it is not included within a [LibraryClasses] section, it will
> > > > > > >   generate a build error.
> > > > > > I will separate this to another set of patch. Don’t mix up with
> > > > > > jansson edk2
> > > > > port.
> > > > > >
> > > > > > >You might also consider changing the name
> > > > > > >   of this file in case you want to add more than just lib mappings
> > > > > > >   to this file in the future.  See UnitTestFramworkPkg for examples.
> > > > > > >
> > > > > > > Best regards,
> > > > > > >
> > > > > > > Mike
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Abner Chang <abner.chang@hpe.com>
> > > > > > > > Sent: Thursday, December 17, 2020 5:19 AM
> > > > > > > > To: devel@edk2.groups.io
> > > > > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > > > > > <Bret.Barkelew@microsoft.com>; Andrew Fish
> > > > > > > > <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Leif
> > > > > > > > Lindholm <leif@nuviainc.com>; Kinney, Michael D
> > > > > > > > <michael.d.kinney@intel.com>; Liming Gao
> > > > > > > > <gaoliming@byosoft.com.cn>; Nickle Wang
> > > <nickle.wang@hpe.com>;
> > > > > > > Peter
> > > > > > > > O'Hanley <peter.ohanley@hpe.com>
> > > > > > > > Subject: [PATCH v8 0/6] jansson edk2 port
> > > > > > > >
> > > > > > > > In v8, - Assigne patch file order
> > > > > > > >        - Add Acked-by tags
> > > > > > > > In v7, - Remove C RTC header files to under
> > > [Include.Common.Private]
> > > > > > > >          in RedfishPkg.dec.
> > > > > > > >        - address comments given by Mike Kinney.
> > > > > > > > In v6, Remove JanssonJsonMapping.h In v5, move
> > > > > > > > BaseUcs2Utf8Lib to under RedfishPkg.
> > > > > > > > In v4,
> > > > > > > >        - Address review comments
> > > > > > > >        - Seperate CRT functions to a individule library CrtLib under
> > > > > > > >          RedfishPkg.
> > > > > > > >        - Seperate UCS2-UTF8 functions to a individule library
> > > > > > > >          BaseUcs2Utf8Lib under MdeModulePkg.
> > > > > > > >
> > > > > > > > In v3, Add jansson library as the required submoudle in
> > > > > > > >        CiSettings.py for CI test.
> > > > > > > > In v2, JsonLib is moved to under RedfishPkg.
> > > > > > > >
> > > > > > > > edk2 JSON library is based on jansson open source
> > > > > > > > (https://github.com/akheron/jansson) and wrapped as an edk2
> > > library.
> > > > > > > > edk2 JsonLib will be used by edk2 Redfish feature drivers
> > > > > > > > (not contributed yet) and the edk2 port of libredfish
> > > > > > > > library (not contributed yet) based on DMTF GitHub
> > > > > > > > (https://github.com/DMTF/libredfish).
> > > > > > > >
> > > > > > > > Jansson is licensed under the MIT license(refer to
> > > > > > > > ReadMe.rst under
> > > > > edk2).
> > > > > > > > It is used in production and its API is stable. In
> > > > > > > > UEFI/EDKII environment, Redfish project consumes jansson to
> > > > > > > > achieve JSON
> > > > > > > operations.
> > > > > > > >
> > > > > > > > * Jansson version on edk2: 2.13.1
> > > > > > > >
> > > > > > > > * EDKII jansson library wrapper:
> > > > > > > >    - JsonLib.h:
> > > > > > > >      This is the denifitions of EDKII JSON APIs which are mapped to
> > > > > > > >      jannson funcitons accordingly.
> > > > > > > >
> > > > > > > >    - JanssonJsonLibMapping.h:
> > > > > > > >      This is the wrapper file to map funcitons and definitions used in
> > > > > > > >      native jannson applications to edk2 JsonLib. This avoids the
> > > > > > > >      modifications on native jannson applications to be built under
> > > > > > > >      edk2 environment.
> > > > > > > >
> > > > > > > > *Known issue:
> > > > > > > >   Build fail with jansson/src/load.c, overrride and add code in load.c
> > > > > > > >   to conditionally use stdin according to HAVE_UNISTD_H macro.
> > > > > > > >   The PR is submitted to jansson open source community.
> > > > > > > >   https://github.com/akheron/jansson/pull/558
> > > > > > > >
> > > > > > > > Signed-off-by: Abner Chang <abner.chang@hpe.com>
> > > > > > > >
> > > > > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > > > > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > > > > > > > Cc: Andrew Fish <afish@apple.com>
> > > > > > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > > > > > Cc: Leif Lindholm <leif@nuviainc.com>
> > > > > > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > > > > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > > > > > > Cc: Nickle Wang <nickle.wang@hpe.com>
> > > > > > > > Cc: Peter O'Hanley <peter.ohanley@hpe.com>
> > > > > > > >
> > > > > > > > Abner Chang (6):
> > > > > > > >   RedfishPkg/Ucs2Utf8lib: UCS2 to UFT8 manipulation library
> > > > > > > >   edk2: jansson submodule for edk2 JSON library
> > > > > > > >   RedfishPkg/CrtLib: C runtime library
> > > > > > > >   RedfishPkg/library: EDK2 port of jansson library
> > > > > > > >   RedfishPkg: Add EDK2 port of jansson library to build
> > > > > > > >   .pytool: Add required submodule for JsonLib
> > > > > > > >
> > > > > > > >  .gitmodules                                   |    3 +
> > > > > > > >  .pytool/CISettings.py                         |    2 +
> > > > > > > >  ReadMe.rst                                    |    1 +
> > > > > > > >  RedfishPkg/Include/Crt/assert.h               |   16 +
> > > > > > > >  RedfishPkg/Include/Crt/errno.h                |   16 +
> > > > > > > >  RedfishPkg/Include/Crt/limits.h               |   16 +
> > > > > > > >  RedfishPkg/Include/Crt/math.h                 |   16 +
> > > > > > > >  RedfishPkg/Include/Crt/stdarg.h               |   15 +
> > > > > > > >  RedfishPkg/Include/Crt/stddef.h               |   16 +
> > > > > > > >  RedfishPkg/Include/Crt/stdio.h                |   15 +
> > > > > > > >  RedfishPkg/Include/Crt/stdlib.h               |   16 +
> > > > > > > >  RedfishPkg/Include/Crt/string.h               |   16 +
> > > > > > > >  RedfishPkg/Include/Crt/sys/time.h             |   15 +
> > > > > > > >  RedfishPkg/Include/Crt/sys/types.h            |   15 +
> > > > > > > >  RedfishPkg/Include/Crt/time.h                 |   15 +
> > > > > > > >  RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h  |   61 +
> > > > > > > >  RedfishPkg/Include/Library/CrtLib.h           |  191 +++
> > > > > > > >  RedfishPkg/Include/Library/JsonLib.h          |  763 +++++++++++
> > > > > > > >  .../Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c |  421 +++++++
> > > > > > > >  .../BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf       |   31 +
> > > > > > > >  RedfishPkg/Library/CrtLib/CrtLib.c            |  595 +++++++++
> > > > > > > >  RedfishPkg/Library/CrtLib/CrtLib.inf          |   38 +
> > > > > > > >  RedfishPkg/Library/JsonLib/JsonLib.c          |  964
> > ++++++++++++++
> > > > > > > >  RedfishPkg/Library/JsonLib/JsonLib.inf        |   89 ++
> > > > > > > >  RedfishPkg/Library/JsonLib/Readme.rst         |   40 +
> > > > > > > >  RedfishPkg/Library/JsonLib/jansson            |    1 +
> > > > > > > >  RedfishPkg/Library/JsonLib/jansson_config.h   |   41 +
> > > > > > > >  .../Library/JsonLib/jansson_private_config.h  |   19 +
> > > > > > > >  RedfishPkg/Library/JsonLib/load.c             | 1111
> > > +++++++++++++++++
> > > > > > > >  RedfishPkg/RedfishLibs.dsc.inc                |    3 +
> > > > > > > >  RedfishPkg/RedfishPkg.ci.yaml                 |   25 +
> > > > > > > >  RedfishPkg/RedfishPkg.dec                     |   25 +
> > > > > > > >  RedfishPkg/RedfishPkg.dsc                     |    3 +
> > > > > > > >  33 files changed, 4614 insertions(+)  create mode 100644
> > > > > > > > RedfishPkg/Include/Crt/assert.h  create mode
> > > > > > > > 100644 RedfishPkg/Include/Crt/errno.h  create mode 100644
> > > > > > > > RedfishPkg/Include/Crt/limits.h  create mode 100644
> > > > > > > > RedfishPkg/Include/Crt/math.h  create mode 100644
> > > > > > > > RedfishPkg/Include/Crt/stdarg.h  create mode 100644
> > > > > > > > RedfishPkg/Include/Crt/stddef.h  create mode 100644
> > > > > > > > RedfishPkg/Include/Crt/stdio.h  create mode 100644
> > > > > > > > RedfishPkg/Include/Crt/stdlib.h  create mode 100644
> > > > > > > > RedfishPkg/Include/Crt/string.h  create mode 100644
> > > > > > > > RedfishPkg/Include/Crt/sys/time.h  create mode 100644
> > > > > > > > RedfishPkg/Include/Crt/sys/types.h
> > > > > > > >  create mode 100644 RedfishPkg/Include/Crt/time.h  create
> > > > > > > > mode
> > > > > > > > 100644 RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h
> > > > > > > >  create mode 100644 RedfishPkg/Include/Library/CrtLib.h
> > > > > > > >  create mode 100644 RedfishPkg/Include/Library/JsonLib.h
> > > > > > > >  create mode 100644
> > > > > > > > RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c
> > > > > > > >  create mode 100644
> > > > > > > > RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
> > > > > > > >  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.c
> > > > > > > >  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.inf
> > > > > > > >  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.c
> > > > > > > >  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.inf
> > > > > > > >  create mode 100644 RedfishPkg/Library/JsonLib/Readme.rst
> > > > > > > >  create mode 160000 RedfishPkg/Library/JsonLib/jansson
> > > > > > > >  create mode 100644
> > > > > > > > RedfishPkg/Library/JsonLib/jansson_config.h
> > > > > > > >  create mode 100644
> > > > > > > > RedfishPkg/Library/JsonLib/jansson_private_config.h
> > > > > > > >  create mode 100644 RedfishPkg/Library/JsonLib/load.c
> > > > > > > >
> > > > > > > > --
> > > > > > > > 2.17.1
> > > > > >
> > > >
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69442): https://edk2.groups.io/g/devel/message/69442
Mute This Topic: https://groups.io/mt/79052636/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> -----Original Message-----
> From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> Sent: Thursday, December 24, 2020 2:50 PM
> To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> <abner.chang@hpe.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>; Laszlo
> Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> <nickle.wang@hpe.com>; O'Hanley, Peter (EXL) <peter.ohanley@hpe.com>
> Subject: RE: [edk2-devel] [PATCH v8 0/6] jansson edk2 port
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Abner
> > Chang
> > Sent: Wednesday, December 23, 2020 1:02 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > devel@edk2.groups.io
> > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>; Laszlo
> > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming
> > Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley@hpe.com>
> > Subject: Re: [edk2-devel] [PATCH v8 0/6] jansson edk2 port
> >
> > Hi Mike, v10 patch set sent.
> > BTW, why don’t you put those private libraries in UniTestFrameworkPkg
> > to under /PrivateLibrary which is similar to /PrivateInclude?
> 
> That is a good question.  We had not considered that before because it does
> not change the visibility of the library instance (we have to support listing
> these in platform DSC files).  Using PrivateInclude actually prevents a
> module/lib from other packages from using those include files because the
> include paths passed into the compiler do not contain private include paths.
> 
> From a self-documenting perspective using a PrivateLibrary directory name
> would make it obvious that the library is not intended to be used by any
> components from other packages.
> 
> Mike
Yes, that is what my thought. I will have /PrivateLibrary for RedfishCrtLib.

Thanks
Abner
> 
> >
> > Abner
> >
> > > -----Original Message-----
> > > From: Chang, Abner (HPS SW/FW Technologist)
> > > Sent: Wednesday, December 23, 2020 3:54 PM
> > > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > > devel@edk2.groups.io
> > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> Laszlo
> > > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Liming
> > > Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley@hpe.com>
> > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> > > > Sent: Wednesday, December 23, 2020 2:10 PM
> > > > To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> > > > devel@edk2.groups.io; Kinney, Michael D
> > > > <michael.d.kinney@intel.com>
> > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> > > > Laszlo Ersek <lersek@redhat.com>; Leif Lindholm
> > > > <leif@nuviainc.com>; Liming Gao <gaoliming@byosoft.com.cn>; Wang,
> > > > Nickle (HPS SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> > > <peter.ohanley@hpe.com>
> > > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > > >
> > > > Hi Abner,
> > > >
> > > > Response below.
> > > >
> > > > Mike
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Chang, Abner (HPS SW/FW Technologist)
> > > <abner.chang@hpe.com>
> > > > > Sent: Tuesday, December 22, 2020 8:40 PM
> > > > > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > > > > devel@edk2.groups.io
> > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> > > Laszlo
> > > > > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>;
> > > > > Liming Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > > > > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> > > > <peter.ohanley@hpe.com>
> > > > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> > > > > > Sent: Wednesday, December 23, 2020 2:14 AM
> > > > > > To: Chang, Abner (HPS SW/FW Technologist)
> > > > > > <abner.chang@hpe.com>; devel@edk2.groups.io; Kinney, Michael
> D
> > > > > > <michael.d.kinney@intel.com>
> > > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > > > <Bret.Barkelew@microsoft.com>; Andrew Fish <afish@apple.com>;
> > > > Laszlo
> > > > > > Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>;
> > > > > > Liming Gao <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > > > > > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> > > > <peter.ohanley@hpe.com>
> > > > > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > > > > >
> > > > > > Hi Abner,
> > > > > >
> > > > > > A few comments below.
> > > > > >
> > > > > > Best regards,
> > > > > >
> > > > > > Mike
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Chang, Abner (HPS SW/FW Technologist)
> > > > <abner.chang@hpe.com>
> > > > > > > Sent: Monday, December 21, 2020 12:17 AM
> > > > > > > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > > > > > > devel@edk2.groups.io
> > > > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > > > > <Bret.Barkelew@microsoft.com>; Andrew Fish
> > > > > > > <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Leif
> > > > > > > Lindholm <leif@nuviainc.com>; Liming Gao
> > > > > > > <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > > > > > > <nickle.wang@hpe.com>; O'Hanley, Peter
> > > > > > > (EXL)
> > > > > > <peter.ohanley@hpe.com>
> > > > > > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > > > > > >
> > > > > > > Mike, response in below. Also v9 is sent.
> > > > > > >
> > > > > > > Thanks for review this in detail.
> > > > > > > Abner
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Kinney, Michael D
> > > > > > > > [mailto:michael.d.kinney@intel.com]
> > > > > > > > Sent: Monday, December 21, 2020 3:43 AM
> > > > > > > > To: Chang, Abner (HPS SW/FW Technologist)
> > > > <abner.chang@hpe.com>;
> > > > > > > > devel@edk2.groups.io; Kinney, Michael D
> > > > > > > > <michael.d.kinney@intel.com>
> > > > > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> > > > > > > > <Bret.Barkelew@microsoft.com>; Andrew Fish
> > > > > > > > <afish@apple.com>;
> > > > > > Laszlo
> > > > > > > > Ersek <lersek@redhat.com>; Leif Lindholm
> > > > > > > > <leif@nuviainc.com>; Liming Gao
> > > > > > > > <gaoliming@byosoft.com.cn>; Wang, Nickle (HPS SW)
> > > > > > > > <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> > > > > > <peter.ohanley@hpe.com>
> > > > > > > > Subject: RE: [PATCH v8 0/6] jansson edk2 port
> > > > > > > >
> > > > > > > > Hi Abner,
> > > > > > > >
> > > > > > > > The include path in the [Include.Common.Private] section
> > > > > > > > should not be in the same file path as the [Include] section.
> > > > > > > > This allows private include files to be accessed.
> > > > > > > >
> > > > > > > > Instead, you should create a new top level package
> > > > > > > > directory called PrivateInclude and put all non-public
> > > > > > > > include content in that directory.  The
> > > > > > > > UnitTestFrameworkPkg is a good reference that demonstrates
> this design pattern.
> > > > > > > Yes, this looks better. All Crt header files and CrtLib.h
> > > > > > > will be moved to
> > > > > > under PrivateInclude.
> > > > > > > >
> > > > > > > > The library class name CrtLib is very generic and the
> > > > > > > > library class is in the public includes so it is exported
> > > > > > > > as a public interface from the
> > > > > > RedfishPkg.
> > > > > > > > This CrtLib implementation appears to be tuned to support
> > > > > > > > the dependencies for the jansson submodule.  I recommend
> > > > > > > > changing the library class name and the library instance
> > > > > > > > name so it is clear that this CrtLib is only for jansson.
> > > > > > > > Perhaps 'JanssonCrtLib'.
> > > > > > > The CrtLib is not used by jannson lib, libredfish as well
> > > > > > > (https://github.com/DMTF/libredfish). So JanssonCrtLib is
> > > > > > > not a good
> > > > > > naming, will keep that as it.
> > > > > > >
> > > > > > > >Also, does this library class (CrtLib.h) need to be
> > > > > > > >exported from RedfishPkg as a public interface?
> > > > > > > No, it will be moved to under PrivateInclude.
> > > > > > >
> > > > > > > If CrtLib.h is defined as a private library and CrtLib.h is
> > > > > > > in PrivateInclude which is not exposed to other packages,
> > > > > > > then why do we
> > > > > > care about the naming of CrtLib is too generic? Which is the
> > > > > > private library under RedfishPkg.
> > > > > > > Move those header files to under PrivateInclude seems clear
> > > > > > > to people that CrtLib is only for RedfishPkg. I think we
> > > > > > > don’t have to change the
> > > > > > naming in this case.
> > > > > >
> > > > > > In general I agree, but there is one place where the lib
> > > > > > mapping has wider scope, and that is in a platform DSC file.
> > > > > > The lib class name CrtLib has to be listed in the DSC file.
> > > > > > If more than one package defines a library class called
> > > > > > CrtLib, then the last mapping used will be used for all
> > > > > > modules if it is listed in the [LibraryClasses] section of the
> > > > > > DSC file.  The only was to handle this type of name collision
> > > > > > is to use a module scoped library mapping in the
> > > > > > <LibraryClasses> section of each individual module that uses a
> > > > > > CrtLib.  I am only trying to avoid the potential for future
> > > > > > name collisions
> > > > for a library class/instance that is specific to the JSON use case.
> > > > > >
> > > > > > We recognize that several modules in edk2 have this type of
> > > > > > gasket between an EDK II env and a standard C lib.  We may
> > > > > > find a we to implement a more generic gasket and retire the
> > > > > > use of the implementation specific ones.  This is another
> > > > > > reason to use an implementation specific name in this use case
> > > > > > and be able to introduce a
> > > > more generic name in the future.
> > > > > >
> > > > > > Another comment below to see if there is a way to avoid
> > > > > > introducing a CrtLib class.
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > * RedfishPkg/Include/Library/CrtLib.h
> > > > > > > >   + Remove reference to MDE_CPU_IA64.  This has been
> > > > > > > > retired from the
> > > > > > > > edk2 repo.
> > > > > > > Ok.
> > > > > > > >   + I do see one remaining reference to this in the
> > > > > > > > CryptoPkg that need to
> > > > > > be
> > > > > > > >     removed.
> > > > > > > >
> > > > > > > > * RedfishPkg/Include/Library/JsonLib.h
> > > > > > > >   + Some of the function descriptions are very brief and I can not
> tell
> > > > > > > >     how to use the service from the description and more
> > > > > > > > importantly,
> > > I
> > > > > > > >     would not know how to write a unit test to verify the
> > > > > > > > expected
> > > > > > behavior.
> > > > > > > >     Since these are a set of public APIs from this package that you
> > > > > > > >     expect modules/libs from outside of RedfishPkg to use, then
> all of
> > > > > > > >     these public APIs must be fully documented.
> > > > > > > Most of the API in JsonLib are wrappers of native jansson APIs.
> > > > > > > We can mention the URL to jansson API reference document in
> > > > > > > file
> > > header.
> > > > > > > INVALID URI REMOVED
> > > > > > 3A__jansson.readthedo
> > > > > > >
> > > > > >
> > > >
> > >
> cs.io_en_2.13_index.html&d=DwIGaQ&c=C5b8zRQO1miGmBeVZ2LFWg&r=_
> > > > > > SN6FZBN4
> > > > > > > Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=vd-
> > > > > > uPz864czoYA5C7YaPgqXfdPjv4c5kB
> > > > > > >
> > > >
> zTwV60NvNQ&s=acVzN3x4yND0jkg44bJ48ZkyPyugjwsYvUGdD4nJL74&e=
> > > > > > > I will review all function header again,  please check v9 patch later.
> > > > > > >
> > > > > > > >   + Are there any of these APIs that are not really needed
> > > > > > > > by
> > > > modules/libs
> > > > > > > >     outside the RedfishPkg?  It would be better to remove
> > > > > > > > APIs that
> > > are
> > > > > > > >     not needed outside RedfishPkg from this public include file.
> > > > > > > >   + A couple examples that stood out are:
> > > > > > > >     JsonDecreaseReference()
> > > > > > > >     JsonIncreaseReference()
> > > > > > > >     JsonObjectIterator()
> > > > > > > >     JsonObjectIteratorValue()
> > > > > > > >     JsonObjectIteratorNext()
> > > > > > > The native jansson APIs of all above wrappers are used in
> libredfish.
> > > > > > > So that is the potential use case for other JSON applications.
> > > > > >
> > > > > > Can you provide a pointer to an example of this use case?
> > > > > >
> > > > > > I am trying to make sure we are doing the right design of
> > > > > > RedfishPkg with respect to CrtLib for this use case.  Are
> > > > > > these are JSON apps that do not use JsonLib, but are expected
> > > > > > to build in an EDK II build environment?  Would these JSON
> > > > > > apps require more standard C lib services than the CrtLib used
> > > > > > to build JsonLib?  Where would those
> > > > additional C lib services come from?
> > > > > >
> > > > > > There is a standard C lib in edk2-libc.  Would that be
> > > > > > required to build the JSON apps.
> > > > > >
> > > > > > In other uses of submodules with C lib dependencies, a new
> > > > > > CrtLib like lib class was not added and instead the support
> > > > > > was added directly to the EDK II wrapper APIs. Some examples are:
> > > > > >   * MdeModulePkg\Library\BrotliCustomDecompressLib
> > > > > >   * MdeModulePkg\Universal\RegularExpressionDxe
> > > > > >
> > > > > > If there are use cases where the CrtLib will be added to an
> > > > > > INF outside the RedfishPkg, then I still think the name needs
> > > > > > to be changed to be specific to the JSON use case.
> > > > > CrtLib should not be added to INF outside RedfishPkg, but it
> > > > > will be added
> > > > to platform DSC file.
> > > > > >
> > > > > > If the use of CrtLib is only within the RedfishPkg, then the
> > > > > > Library Class can be declared in a
> > > > > > [LibraryClasses.Common.Private] section of the DEC file.  See
> > > > > > UnitTestFrameworkPkg for examples
> > > > > Because CrtLib, JsonLib and other RedfishPkg modules will be
> > > > > included in
> > > > other package dsc file (e.g. EmulatorPkg).
> > > > > What if in EmulatorPkg one CrtLib libraryclass maps to the
> > > > > private one under RedfishPkg, but another with the same name is
> > > > > the public library
> > > > provided by other package (e.g. MdeModulePkg) and used by modules
> > > > other than RedfishPkg?
> > > > > Is edk2 build tool that smart to link the private CrtLib for
> > > > > RedfishPkg modules and all other non RedfishPkg modules are
> > > > > linked with
> > > > the public one?
> > > > >
> > > > > Abner
> > > > >
> > > >
> > > > No.  Not that smart.
> > > >
> > > > There are global lib class -> lib instance mapping in
> > > > [LibraryClasses] sections of DSC files.  If the same lib class is
> > > > listed more than once, then the last mapping wins.
> > > >
> > > > If a module scoped <LibraryClasses> section is used, then then
> > > > last mapping in the scope of that module wins.
> > > >
> > > > If the same lib class name is declared by more than one package
> > > > used in a platform DSC, then the only remediation is to remove use
> > > > of the global [LibraryClasses] section and use a module scope
> > > > <LibraryClasses> section in every module that depends on that lib
> > > > class
> > > name.
> > > Ok, I will rename it to RedfishCrtLib.
> > >
> > > Abner
> > > >
> > > > > >
> > > > > > >
> > > > > > > >   + JsonDumpString()
> > > > > > > >     func header params do not match func prototype
> > > > > > > >     Does not describe who allocates the return buffer and who
> > > > > > > >     is responsible for freeing the buffer returned.
> > > > > > > >     What do Flags do?  What is the difference between this
> > > > > > > >     function and JsonToText()?
> > > > > > > In JsonToText, it only converts JSON array and object to text.
> > > > > > > We could just remove this for now because I don’t really
> > > > > > > remember the reason
> > > > > > to create this function, it has been a while. Seems to me it
> > > > > > is fine to remove this function.
> > > > > > >
> > > > > > > >   + JsonLoadBuffer() - Error param description missing
> > > > > > > >   + JsonArrayGetValue() does not describe the conditions NULL
> > > > > > > >     is returned.
> > > > > > > >   + JsonObjectGetKeys().  Return type is CHAR8**.  What
> > > > > > > >     function need to be called to free the array?
> > > > > > > All above addressed.
> > > > > > >
> > > > > > > >   + JsonValueGetAsciiString() and JsonValueGetUnicodeString()
> > > > > > > >     are asymmetric.  The Ascii one states that change will
> > > > > > > >     modify the original string.  The Unicode one says that the
> > > > > > > >     caller needs to free the returned string.  Any reason we
> > > > > > > >     can not make them symmetric?
> > > > > > > Jansson doesn't support Unicode string. The memory of the
> > > > > > > unicode string returned by JsonValueGetUnicodeString Is
> > > > > > > allocated by
> > > > > > UTF8StrToUCS2().
> > > > > > >  > Also, if Ascii one should
> > > > > > > >     not be modified, why isn’t the return type CONST?
> > > > > > > Yes. will do that.
> > > > > > >
> > > > > > > >
> > > > > > > > * RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
> > > > > > > >   + Change [Sources.common] to [Sources]
> > > > > > > done
> > > > > > > > * RedfishPkg/Library/CrtLib/CrtLib.inf
> > > > > > > >   + Does this lib instance really depend on MdeModulePkg?
> > > > > > > Yes, SortLib
> > > > > > > >   + Have you reviewed the module types this lib instance is
> > > > > > > >     compatible with?  Why does it need to support DXE_CORE?
> > > > > > > >     Are their JsonLib use cases for DXE_RUNTIME and SMM
> drivers?
> > > > > > > >     If there are SMM use cases, then why is MM excluded?
> > > > > > > Will just keep DXE_DRVER, UEFI_APPLICATION and UEFI_DRIVER.
> > > > These
> > > > > > are
> > > > > > > module types have potential use cases I can think of now.
> > > > > > > >
> > > > > > > > * RedfishPkg/Library/JsonLib
> > > > > > > >   + Readme.rst still has references to
> > > > > > > > JanssonJsonLibMapping.h
> > > > > > > done
> > > > > > > >   + Have you reviewed the module types this lib instance is
> > > > > > > >     compatible with?  Why does it need to support DXE_CORE?
> > > > > > > >     Are their JsonLib use cases for DXE_RUNTIME and SMM
> drivers?
> > > > > > > >     If there are SMM use cases, then why is MM excluded?
> > > > > > > Answered above
> > > > > > > >   + JsonLib.inf has a [BuildOptions] section that disables
> > > > > > > > some warnings
> > > > > > that
> > > > > > > >     make me concerned that this library may have some
> > > > > > > > issues with 32-bit builds.
> > > > > > > >     Have you fully validated all 32-bit CPU builds and
> > > > > > > > validated the
> > > > > > functionality
> > > > > > > >     of all services from the JsonLib for all ranges of input values?
> > > > > > > Yes, IA32 build is validated. But not validating on the
> > > > > > > functionalities IA32. I don't have that use case, we can get
> > > > > > > back to fix the
> > > > > > issue if any when  someone runs this on IA32.
> > > > > >
> > > > > > I did some local testing with VS2019.  I found that a much
> > > > > > smaller set of warning disables are required for IA32 and X64
> > > > > > and the set is different for
> > > > > > IA32 and X64.  Can you review what is needed and minimize the
> > > > > > required set of each supported arch?
> > > > > >
> > > > > >   MSFT:*_*_X64_CC_FLAGS = /wd4244 /wd4090 /wd4334
> > > > > > /DHAVE_CONFIG_H=1 /U_WIN32 /UWIN64 /U_MSC_VER
> > > > > >   MSFT:*_*_IA32_CC_FLAGS = /wd4244 /wd4090
> > > /DHAVE_CONFIG_H=1
> > > > > > /U_WIN32 /UWIN64 /U_MSC_VER
> > > > > >
> > > > > > > >
> > > > > > > > * RedfishLibs.dsc.inc
> > > > > > > >   It is better to add [LibraryClasses] statement to this file, so it
> > > > > > > >   can be include anywhere in a DSC file.  With current
> > > implementation
> > > > > > > >   if it is not included within a [LibraryClasses] section, it will
> > > > > > > >   generate a build error.
> > > > > > > I will separate this to another set of patch. Don’t mix up
> > > > > > > with jansson edk2
> > > > > > port.
> > > > > > >
> > > > > > > >You might also consider changing the name
> > > > > > > >   of this file in case you want to add more than just lib mappings
> > > > > > > >   to this file in the future.  See UnitTestFramworkPkg for
> examples.
> > > > > > > >
> > > > > > > > Best regards,
> > > > > > > >
> > > > > > > > Mike
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Abner Chang <abner.chang@hpe.com>
> > > > > > > > > Sent: Thursday, December 17, 2020 5:19 AM
> > > > > > > > > To: devel@edk2.groups.io
> > > > > > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret
> > > > > > > > > Barkelew <Bret.Barkelew@microsoft.com>; Andrew Fish
> > > > > > > > > <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>;
> > > > > > > > > Leif Lindholm <leif@nuviainc.com>; Kinney, Michael D
> > > > > > > > > <michael.d.kinney@intel.com>; Liming Gao
> > > > > > > > > <gaoliming@byosoft.com.cn>; Nickle Wang
> > > > <nickle.wang@hpe.com>;
> > > > > > > > Peter
> > > > > > > > > O'Hanley <peter.ohanley@hpe.com>
> > > > > > > > > Subject: [PATCH v8 0/6] jansson edk2 port
> > > > > > > > >
> > > > > > > > > In v8, - Assigne patch file order
> > > > > > > > >        - Add Acked-by tags In v7, - Remove C RTC header
> > > > > > > > > files to under
> > > > [Include.Common.Private]
> > > > > > > > >          in RedfishPkg.dec.
> > > > > > > > >        - address comments given by Mike Kinney.
> > > > > > > > > In v6, Remove JanssonJsonMapping.h In v5, move
> > > > > > > > > BaseUcs2Utf8Lib to under RedfishPkg.
> > > > > > > > > In v4,
> > > > > > > > >        - Address review comments
> > > > > > > > >        - Seperate CRT functions to a individule library CrtLib under
> > > > > > > > >          RedfishPkg.
> > > > > > > > >        - Seperate UCS2-UTF8 functions to a individule library
> > > > > > > > >          BaseUcs2Utf8Lib under MdeModulePkg.
> > > > > > > > >
> > > > > > > > > In v3, Add jansson library as the required submoudle in
> > > > > > > > >        CiSettings.py for CI test.
> > > > > > > > > In v2, JsonLib is moved to under RedfishPkg.
> > > > > > > > >
> > > > > > > > > edk2 JSON library is based on jansson open source
> > > > > > > > > (https://github.com/akheron/jansson) and wrapped as an
> > > > > > > > > edk2
> > > > library.
> > > > > > > > > edk2 JsonLib will be used by edk2 Redfish feature
> > > > > > > > > drivers (not contributed yet) and the edk2 port of
> > > > > > > > > libredfish library (not contributed yet) based on DMTF
> > > > > > > > > GitHub (https://github.com/DMTF/libredfish).
> > > > > > > > >
> > > > > > > > > Jansson is licensed under the MIT license(refer to
> > > > > > > > > ReadMe.rst under
> > > > > > edk2).
> > > > > > > > > It is used in production and its API is stable. In
> > > > > > > > > UEFI/EDKII environment, Redfish project consumes jansson
> > > > > > > > > to achieve JSON
> > > > > > > > operations.
> > > > > > > > >
> > > > > > > > > * Jansson version on edk2: 2.13.1
> > > > > > > > >
> > > > > > > > > * EDKII jansson library wrapper:
> > > > > > > > >    - JsonLib.h:
> > > > > > > > >      This is the denifitions of EDKII JSON APIs which are mapped
> to
> > > > > > > > >      jannson funcitons accordingly.
> > > > > > > > >
> > > > > > > > >    - JanssonJsonLibMapping.h:
> > > > > > > > >      This is the wrapper file to map funcitons and definitions
> used in
> > > > > > > > >      native jannson applications to edk2 JsonLib. This avoids the
> > > > > > > > >      modifications on native jannson applications to be built
> under
> > > > > > > > >      edk2 environment.
> > > > > > > > >
> > > > > > > > > *Known issue:
> > > > > > > > >   Build fail with jansson/src/load.c, overrride and add code in
> load.c
> > > > > > > > >   to conditionally use stdin according to HAVE_UNISTD_H
> macro.
> > > > > > > > >   The PR is submitted to jansson open source community.
> > > > > > > > >   https://github.com/akheron/jansson/pull/558
> > > > > > > > >
> > > > > > > > > Signed-off-by: Abner Chang <abner.chang@hpe.com>
> > > > > > > > >
> > > > > > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > > > > > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > > > > > > > > Cc: Andrew Fish <afish@apple.com>
> > > > > > > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > > > > > > Cc: Leif Lindholm <leif@nuviainc.com>
> > > > > > > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > > > > > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > > > > > > > Cc: Nickle Wang <nickle.wang@hpe.com>
> > > > > > > > > Cc: Peter O'Hanley <peter.ohanley@hpe.com>
> > > > > > > > >
> > > > > > > > > Abner Chang (6):
> > > > > > > > >   RedfishPkg/Ucs2Utf8lib: UCS2 to UFT8 manipulation library
> > > > > > > > >   edk2: jansson submodule for edk2 JSON library
> > > > > > > > >   RedfishPkg/CrtLib: C runtime library
> > > > > > > > >   RedfishPkg/library: EDK2 port of jansson library
> > > > > > > > >   RedfishPkg: Add EDK2 port of jansson library to build
> > > > > > > > >   .pytool: Add required submodule for JsonLib
> > > > > > > > >
> > > > > > > > >  .gitmodules                                   |    3 +
> > > > > > > > >  .pytool/CISettings.py                         |    2 +
> > > > > > > > >  ReadMe.rst                                    |    1 +
> > > > > > > > >  RedfishPkg/Include/Crt/assert.h               |   16 +
> > > > > > > > >  RedfishPkg/Include/Crt/errno.h                |   16 +
> > > > > > > > >  RedfishPkg/Include/Crt/limits.h               |   16 +
> > > > > > > > >  RedfishPkg/Include/Crt/math.h                 |   16 +
> > > > > > > > >  RedfishPkg/Include/Crt/stdarg.h               |   15 +
> > > > > > > > >  RedfishPkg/Include/Crt/stddef.h               |   16 +
> > > > > > > > >  RedfishPkg/Include/Crt/stdio.h                |   15 +
> > > > > > > > >  RedfishPkg/Include/Crt/stdlib.h               |   16 +
> > > > > > > > >  RedfishPkg/Include/Crt/string.h               |   16 +
> > > > > > > > >  RedfishPkg/Include/Crt/sys/time.h             |   15 +
> > > > > > > > >  RedfishPkg/Include/Crt/sys/types.h            |   15 +
> > > > > > > > >  RedfishPkg/Include/Crt/time.h                 |   15 +
> > > > > > > > >  RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h  |   61 +
> > > > > > > > >  RedfishPkg/Include/Library/CrtLib.h           |  191 +++
> > > > > > > > >  RedfishPkg/Include/Library/JsonLib.h          |  763
> +++++++++++
> > > > > > > > >  .../Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c |  421
> +++++++
> > > > > > > > >  .../BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf       |   31 +
> > > > > > > > >  RedfishPkg/Library/CrtLib/CrtLib.c            |  595 +++++++++
> > > > > > > > >  RedfishPkg/Library/CrtLib/CrtLib.inf          |   38 +
> > > > > > > > >  RedfishPkg/Library/JsonLib/JsonLib.c          |  964
> > > ++++++++++++++
> > > > > > > > >  RedfishPkg/Library/JsonLib/JsonLib.inf        |   89 ++
> > > > > > > > >  RedfishPkg/Library/JsonLib/Readme.rst         |   40 +
> > > > > > > > >  RedfishPkg/Library/JsonLib/jansson            |    1 +
> > > > > > > > >  RedfishPkg/Library/JsonLib/jansson_config.h   |   41 +
> > > > > > > > >  .../Library/JsonLib/jansson_private_config.h  |   19 +
> > > > > > > > >  RedfishPkg/Library/JsonLib/load.c             | 1111
> > > > +++++++++++++++++
> > > > > > > > >  RedfishPkg/RedfishLibs.dsc.inc                |    3 +
> > > > > > > > >  RedfishPkg/RedfishPkg.ci.yaml                 |   25 +
> > > > > > > > >  RedfishPkg/RedfishPkg.dec                     |   25 +
> > > > > > > > >  RedfishPkg/RedfishPkg.dsc                     |    3 +
> > > > > > > > >  33 files changed, 4614 insertions(+)  create mode
> > > > > > > > > 100644 RedfishPkg/Include/Crt/assert.h  create mode
> > > > > > > > > 100644 RedfishPkg/Include/Crt/errno.h  create mode
> > > > > > > > > 100644 RedfishPkg/Include/Crt/limits.h  create mode
> > > > > > > > > 100644 RedfishPkg/Include/Crt/math.h  create mode 100644
> > > > > > > > > RedfishPkg/Include/Crt/stdarg.h  create mode 100644
> > > > > > > > > RedfishPkg/Include/Crt/stddef.h  create mode 100644
> > > > > > > > > RedfishPkg/Include/Crt/stdio.h  create mode 100644
> > > > > > > > > RedfishPkg/Include/Crt/stdlib.h  create mode 100644
> > > > > > > > > RedfishPkg/Include/Crt/string.h  create mode 100644
> > > > > > > > > RedfishPkg/Include/Crt/sys/time.h  create mode 100644
> > > > > > > > > RedfishPkg/Include/Crt/sys/types.h
> > > > > > > > >  create mode 100644 RedfishPkg/Include/Crt/time.h
> > > > > > > > > create mode
> > > > > > > > > 100644 RedfishPkg/Include/Library/BaseUcs2Utf8Lib.h
> > > > > > > > >  create mode 100644 RedfishPkg/Include/Library/CrtLib.h
> > > > > > > > >  create mode 100644 RedfishPkg/Include/Library/JsonLib.h
> > > > > > > > >  create mode 100644
> > > > > > > > > RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.c
> > > > > > > > >  create mode 100644
> > > > > > > > > RedfishPkg/Library/BaseUcs2Utf8Lib/BaseUcs2Utf8Lib.inf
> > > > > > > > >  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.c
> > > > > > > > >  create mode 100644 RedfishPkg/Library/CrtLib/CrtLib.inf
> > > > > > > > >  create mode 100644 RedfishPkg/Library/JsonLib/JsonLib.c
> > > > > > > > >  create mode 100644
> > > > > > > > > RedfishPkg/Library/JsonLib/JsonLib.inf
> > > > > > > > >  create mode 100644
> > > > > > > > > RedfishPkg/Library/JsonLib/Readme.rst
> > > > > > > > >  create mode 160000 RedfishPkg/Library/JsonLib/jansson
> > > > > > > > >  create mode 100644
> > > > > > > > > RedfishPkg/Library/JsonLib/jansson_config.h
> > > > > > > > >  create mode 100644
> > > > > > > > > RedfishPkg/Library/JsonLib/jansson_private_config.h
> > > > > > > > >  create mode 100644 RedfishPkg/Library/JsonLib/load.c
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > 2.17.1
> > > > > > >
> > > > >
> >
> >
> >
> > 
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69444): https://edk2.groups.io/g/devel/message/69444
Mute This Topic: https://groups.io/mt/79052636/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-