[edk2-devel] [PATCH v8 0/6] jansson edk2 port
Abner Chang
abner.chang at hpe.com
Wed Dec 23 03:42:50 UTC 2020
> -----Original Message-----
> From: Kinney, Michael D [mailto:michael.d.kinney at intel.com]
> Sent: Wednesday, December 23, 2020 2:14 AM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang at hpe.com>;
> devel at edk2.groups.io; Kinney, Michael D <michael.d.kinney at intel.com>
> 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>; Liming Gao
> <gaoliming at byosoft.com.cn>; Wang, Nickle (HPS SW)
> <nickle.wang at hpe.com>; O'Hanley, Peter (EXL) <peter.ohanley at 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 at hpe.com>
> > Sent: Monday, December 21, 2020 12:17 AM
> > To: Kinney, Michael D <michael.d.kinney at intel.com>;
> > 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>; Liming
> > Gao <gaoliming at byosoft.com.cn>; Wang, Nickle (HPS SW)
> > <nickle.wang at hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley at 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 at intel.com]
> > > Sent: Monday, December 21, 2020 3:43 AM
> > > To: Chang, Abner (HPS SW/FW Technologist) <abner.chang at hpe.com>;
> > > devel at edk2.groups.io; Kinney, Michael D <michael.d.kinney at intel.com>
> > > 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>; Liming
> > > Gao <gaoliming at byosoft.com.cn>; Wang, Nickle (HPS SW)
> > > <nickle.wang at hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley at 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 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 (#69394): https://edk2.groups.io/g/devel/message/69394
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