[edk2-devel] [PATCH v8 0/6] jansson edk2 port
Michael D Kinney
michael.d.kinney at intel.com
Sun Dec 20 19:42:33 UTC 2020
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 at hpe.com>
> Sent: Thursday, December 17, 2020 5:19 AM
> To: devel at edk2.groups.io
> Cc: Sean Brogan <sean.brogan at microsoft.com>; Bret Barkelew <Bret.Barkelew at microsoft.com>; Andrew Fish <afish at apple.com>;
> Laszlo Ersek <lersek at redhat.com>; Leif Lindholm <leif at nuviainc.com>; Kinney, Michael D <michael.d.kinney at intel.com>;
> Liming Gao <gaoliming at byosoft.com.cn>; Nickle Wang <nickle.wang at hpe.com>; Peter O'Hanley <peter.ohanley at 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 at hpe.com>
>
> Cc: Sean Brogan <sean.brogan at microsoft.com>
> Cc: Bret Barkelew <Bret.Barkelew at microsoft.com>
> Cc: Andrew Fish <afish at apple.com>
> Cc: Laszlo Ersek <lersek at redhat.com>
> Cc: Leif Lindholm <leif at nuviainc.com>
> Cc: Michael D Kinney <michael.d.kinney at intel.com>
> Cc: Liming Gao <gaoliming at byosoft.com.cn>
> Cc: Nickle Wang <nickle.wang at hpe.com>
> Cc: Peter O'Hanley <peter.ohanley at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list