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

["Leif Lindholm" <leif@nuviainc.com>]

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?

Beyond that, this will probably be an issue for all architectures, so
why add explicit (identical) workarounds for IA32/X64?

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
> 
> 
> 
> 
> 
>