Re: [edk2-devel] [PATCH] RedfishPkg/JsonLib: Fix build errors

["Abner Chang" <abner.chang@hpe.com>]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Leif Lindholm
> Sent: Tuesday, January 26, 2021 7:31 PM
> To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> <abner.chang@hpe.com>
> Cc: Wang, Nickle (HPS SW) <nickle.wang@hpe.com>; Michael D Kinney
> <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] [PATCH] RedfishPkg/JsonLib: Fix build errors
> 
> Hi Abner,
> 
> On Mon, Jan 25, 2021 at 12:31:54 +0800, Abner Chang wrote:
> > This patch fixes the build errors when build JsonLib with EDK2 Redfish
> > feature driver.
> >
> > - Add JsonLoadString function to load a NULL terminated-string JSON
> > - json_string_value() in JsonValueGetAsciiString () is removed by
> > accident.
> >
> > Signed-off-by: Abner Chang <abner.chang@hpe.com>
> >
> > Cc: Leif Lindholm <leif@nuviainc.com>
> > Cc: Nickle Wang <nickle.wang@hpe.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > ---
> >  RedfishPkg/Library/JsonLib/JsonLib.inf |  5 +++--
> >  RedfishPkg/Include/Library/JsonLib.h   | 21 ++++++++++++++++++
> >  RedfishPkg/Library/JsonLib/JsonLib.c   | 30
> ++++++++++++++++++++++++--
> >  3 files changed, 52 insertions(+), 4 deletions(-)
> >
> > diff --git a/RedfishPkg/Library/JsonLib/JsonLib.inf
> > b/RedfishPkg/Library/JsonLib/JsonLib.inf
> > index 48b094a78a..9d52a622e1 100644
> > --- a/RedfishPkg/Library/JsonLib/JsonLib.inf
> > +++ b/RedfishPkg/Library/JsonLib/JsonLib.inf
> > @@ -75,12 +75,13 @@
> >    #   C4244: conversion from type1 to type2, possible loss of data
> >    #   C4334: 32-bit shift implicitly converted to 64-bit
> >    #   C4204: nonstandard extension used: non-constant aggregate initializer
> > +  #   C4706: assignment within conditional expression
> >    #
> >    # Define macro HAVE_CONFIG_H to include jansson_private_config.h to
> build.
> >    # Undefined _WIN32, WIN64, _MSC_VER macros
> >    # On GCC, no error on the unused-function and unused-but-set-variable.
> >    #
> > -  MSFT:*_*_X64_CC_FLAGS = /wd4204 /wd4244 /wd4090 /wd4334
> > /DHAVE_CONFIG_H=1 /U_WIN32 /UWIN64 /U_MSC_VER
> > -  MSFT:*_*_IA32_CC_FLAGS = /wd4204 /wd4244 /wd4090
> /DHAVE_CONFIG_H=1
> > /U_WIN32 /UWIN64 /U_MSC_VER
> > +  MSFT:*_*_X64_CC_FLAGS = /wd4204 /wd4244 /wd4090 /wd4334
> /wd4706
> > + /DHAVE_CONFIG_H=1 /U_WIN32 /UWIN64 /U_MSC_VER
> > + MSFT:*_*_IA32_CC_FLAGS = /wd4204 /wd4244 /wd4090 /wd4706
> > + /DHAVE_CONFIG_H=1 /U_WIN32 /UWIN64 /U_MSC_VER
> 
> Urgh, please don't do this.
> C4706 is what gives you a warning for accidentally assigning instead of
> comparing. It's our last defence against the jeopardy-comparing hordes that
> think
>   if (NULL == Ptr)
> is a sensible way of writing C.
> 
> What is the actual problem being encountered?
That is because we use the macro defined in open source header file, RedfishPkg/Library/JsonLib/jansson/src/jansson.h

#define json_object_foreach(object, key, value)                                          \
    for (key = json_object_iter_key(json_object_iter(object));                           \
         key && (value = json_object_iter_value(json_object_key_to_iter(key)));          \
         key = json_object_iter_key(                                                     \
             json_object_iter_next(object, json_object_key_to_iter(key))))

> 
> Beyond that, this will probably be an issue for all architectures, so why add
> explicit (identical) workarounds for IA32/X64?
I didn't catch this build error on GCC. You may know why?

Abner

> 
> Secondly, I understand these changes were added for a single reason "fix
> build failures" - but these are two distinct changes, so should be two distinct
> patches.
> 
> /
>     Leif
> 
> >    GCC:*_*_*_CC_FLAGS = -Wno-unused-function
> > -Wno-unused-but-set-variable
> >
> > diff --git a/RedfishPkg/Include/Library/JsonLib.h
> > b/RedfishPkg/Include/Library/JsonLib.h
> > index 3c10f67d27..82ca4bad60 100644
> > --- a/RedfishPkg/Include/Library/JsonLib.h
> > +++ b/RedfishPkg/Include/Library/JsonLib.h
> > @@ -664,6 +664,27 @@ JsonDumpString (
> >    IN    UINTN               Flags
> >    );
> >
> > +/**
> > +  Convert a string to JSON object.
> > +  The function is used to convert a NULL terminated UTF8 encoded
> > +string to a JSON
> > +  value. Only object and array represented strings can be converted
> > +successfully,
> > +  since they are the only valid root values of a JSON text for UEFI usage.
> > +
> > +  Real number and number with exponent part are not supportted by UEFI.
> > +
> > +  Caller needs to cleanup the root value by calling JsonValueFree().
> > +
> > +  @param[in]   String        The NULL terminated UTF8 encoded string to
> convert
> > +
> > +  @retval      Array JSON value or object JSON value, or NULL when any
> error occurs.
> > +
> > +**/
> > +EDKII_JSON_VALUE
> > +EFIAPI
> > +JsonLoadString (
> > +  IN   CONST CHAR8*    String
> > +  );
> > +
> >  /**
> >    Load JSON from a buffer.
> >
> > diff --git a/RedfishPkg/Library/JsonLib/JsonLib.c
> > b/RedfishPkg/Library/JsonLib/JsonLib.c
> > index 34ff381aee..1762c6f5af 100644
> > --- a/RedfishPkg/Library/JsonLib/JsonLib.c
> > +++ b/RedfishPkg/Library/JsonLib/JsonLib.c
> > @@ -430,10 +430,10 @@ JsonValueGetAsciiString (
> >    IN    EDKII_JSON_VALUE    Json
> >    )
> >  {
> > -  CHAR8          *AsciiStr;
> > +  CONST CHAR8    *AsciiStr;
> >    UINTN          Index;
> >
> > -  AsciiStr = (CHAR8 *)  ((json_t *) Json);
> > +  AsciiStr = json_string_value ((json_t *) Json);
> >    if (AsciiStr == NULL) {
> >      return NULL;
> >    }
> > @@ -819,6 +819,32 @@ JsonDumpString (
> >      return json_dumps((json_t *)JsonValue, Flags);  }
> >
> > +/**
> > +  Convert a string to JSON object.
> > +  The function is used to convert a NULL terminated UTF8 encoded
> > +string to a JSON
> > +  value. Only object and array represented strings can be converted
> > +successfully,
> > +  since they are the only valid root values of a JSON text for UEFI usage.
> > +
> > +  Real number and number with exponent part are not supportted by UEFI.
> > +
> > +  Caller needs to cleanup the root value by calling JsonValueFree().
> > +
> > +  @param[in]   String        The NULL terminated UTF8 encoded string to
> convert
> > +
> > +  @retval      Array JSON value or object JSON value, or NULL when any
> error occurs.
> > +
> > +**/
> > +EDKII_JSON_VALUE
> > +EFIAPI
> > +JsonLoadString (
> > +  IN    CONST CHAR8*    String
> > +  )
> > +{
> > +  json_error_t    JsonError;
> > +
> > +  return (EDKII_JSON_VALUE) json_loads ((const char *)String, 0,
> > +&JsonError); }
> > +
> >  /**
> >    Load JSON from a buffer.
> >
> > --
> > 2.17.1
> >
> >
> >
> >
> >
> >
> 
> 
> 
>