Re: [PATCH] RedfishPkg: RedfishRestExDxe: PCD introduced to disable chunked reguest

["Nickle Wang" <nicklew@nvidia.com>]

Hi Igor,

It seems to me that AMI copyrights header is missing in these files. Could you please check this?

Thanks,
Nickle

-----Original Message-----
From: Chang, Abner <Abner.Chang@amd.com> 
Sent: Sunday, February 5, 2023 12:01 AM
To: devel@edk2.groups.io; igork@ami.com
Cc: Nickle Wang <nicklew@nvidia.com>
Subject: RE: [PATCH] RedfishPkg: RedfishRestExDxe: PCD introduced to disable chunked reguest

External email: Use caution opening links or attachments


[AMD Official Use Only - General]

Hi Igor, thanks for doing this.
The code which is wrapped by "if (FixedPcdGetBool(PcdRedfishRestExChunkRequestMode)" has the different tab spaces. Also, some new lines have the trailing spaces.
I update Nickle's email address for this mail thread temporarily, you can correct his email address in commit message later.
Another comment in line below,

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Igor 
> Kulchytskyy via groups.io
> Sent: Thursday, February 2, 2023 1:59 AM
> To: devel@edk2.groups.io
> Cc: Igor Kulchytskyy <igork@ami.com>; Chang, Abner 
> <Abner.Chang@amd.com>; Nickle Wang <nickle.wang@hpe.com>
> Subject: [edk2-devel] [PATCH] RedfishPkg: RedfishRestExDxe: PCD 
> introduced to disable chunked reguest
>
> Caution: This message originated from an External Source. Use proper 
> caution when opening attachments, clicking links, or responding.
>
>
> BIOS should be able to work with different BMC implementation.
> Some BMC does not support the chunked requests.
> So, this feature should be optional.
> Build time PCD was introduced to enable/disable chunked request.
>
> Cc: Abner Chang <abner.chang@amd.com>
> Cc: Nickle Wang <nickle.wang@hpe.com>
> Signed-off-by: Igor Kulchytskyy <igork@ami.com>
> ---
>  RedfishPkg/RedfishPkg.dec                           |   6 +
>  RedfishPkg/RedfishRestExDxe/RedfishRestExDxe.inf    |   1 +
>  RedfishPkg/RedfishRestExDxe/RedfishRestExProtocol.c | 158 
> +++++++++++-----
> ----
>  3 files changed, 96 insertions(+), 69 deletions(-)
>
> diff --git a/RedfishPkg/RedfishPkg.dec b/RedfishPkg/RedfishPkg.dec 
> index d2b189b..4b4706b 100644
> --- a/RedfishPkg/RedfishPkg.dec
> +++ b/RedfishPkg/RedfishPkg.dec
> @@ -97,3 +97,9 @@
>    # protocol instance.
>    #
>
> gEfiRedfishPkgTokenSpaceGuid.PcdRedfishDiscoverAccessModeInBand|FALSE|
> BOOLEAN|0x00001002
> +  #
> +  # This PCD indicates if the EFI REST EX sends chunk request to Redfish service.
> +  # Default is set to non chunk mode.
> +  #
> +
> +
> gEfiRedfishPkgTokenSpaceGuid.PcdRedfishRestExChunkRequestMode|FALSE|B
> O
> + OLEAN|0x00001003
> +
> \ No newline at end of file
> diff --git a/RedfishPkg/RedfishRestExDxe/RedfishRestExDxe.inf
> b/RedfishPkg/RedfishRestExDxe/RedfishRestExDxe.inf
> index 75437b0..26ce167 100644
> --- a/RedfishPkg/RedfishRestExDxe/RedfishRestExDxe.inf
> +++ b/RedfishPkg/RedfishRestExDxe/RedfishRestExDxe.inf
> @@ -57,6 +57,7 @@
>
>  [Pcd]
>    
> gEfiRedfishPkgTokenSpaceGuid.PcdRedfishRestExServiceAccessModeInBand
> ## CONSUMES
> +  gEfiRedfishPkgTokenSpaceGuid.PcdRedfishRestExChunkRequestMode   ##
> CONSUMES
>
>  [UserExtensions.TianoCore."ExtraFiles"]
>    RedfishRestExDxeExtra.uni
> diff --git a/RedfishPkg/RedfishRestExDxe/RedfishRestExProtocol.c
> b/RedfishPkg/RedfishRestExDxe/RedfishRestExProtocol.c
> index 4b61fc0..22dc5e1 100644
> --- a/RedfishPkg/RedfishRestExDxe/RedfishRestExProtocol.c
> +++ b/RedfishPkg/RedfishRestExDxe/RedfishRestExProtocol.c
> @@ -66,11 +66,13 @@ RedfishRestExSendReceive (
>    HTTP_IO_SEND_NON_CHUNK_PROCESS  SendNonChunkProcess;
>    EFI_HTTP_MESSAGE                ChunkTransferRequestMessage;
>
> -  Status               = EFI_SUCCESS;
> -  ResponseData         = NULL;
> -  IsGetChunkedTransfer = FALSE;
> -  SendChunkProcess     = HttpIoSendChunkNone;
> -  SendNonChunkProcess  = HttpIoSendNonChunkNone;
> +  Status                    = EFI_SUCCESS;
> +  ResponseData              = NULL;
> +  IsGetChunkedTransfer      = FALSE;
> +  SendChunkProcess          = HttpIoSendChunkNone;
> +  SendNonChunkProcess       = HttpIoSendNonChunkNone;
> +  ItsWrite                  = FALSE;
> +  PreservedRequestHeaders   = NULL;
>
>    //
>    // Validate the parameters
> @@ -94,78 +96,96 @@ RedfishRestExSendReceive (
>    DEBUG ((DEBUG_INFO, "\nRedfishRestExSendReceive():\n"));
>    DEBUG ((DEBUG_INFO, "*** Perform HTTP Request Method - %d, URL: 
> %s\n",
> RequestMessage->Data.Request->Method, RequestMessage->Data.Request-
> >Url));
>
> -  //
> -  // Add header "Expect" to server, only for URL write.
> -  //
> -  Status = RedfishHttpAddExpectation (This, RequestMessage, 
> &PreservedRequestHeaders, &ItsWrite);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -
> -  if (ItsWrite == TRUE) {
> -    if (RequestMessage->BodyLength > HTTP_IO_MAX_SEND_PAYLOAD) {
> +  if(FixedPcdGetBool(PcdRedfishRestExChunkRequestMode)){
>        //
> -      // Send chunked transfer.
> +      // Add header "Expect" to server, only for URL write.
>        //
> -      SendChunkProcess++;
> -      CopyMem ((VOID *)&ChunkTransferRequestMessage, (VOID
> *)RequestMessage, sizeof (EFI_HTTP_MESSAGE));
> -    } else {
> -      SendNonChunkProcess++;
> -    }
> +      Status = RedfishHttpAddExpectation (This, RequestMessage,
> &PreservedRequestHeaders, &ItsWrite);
> +      if (EFI_ERROR (Status)) {
> +        return Status;
> +      }
> +
> +      if (ItsWrite == TRUE) {
> +        if (RequestMessage->BodyLength > HTTP_IO_MAX_SEND_PAYLOAD) {
> +          //
> +          // Send chunked transfer.
> +          //
> +          SendChunkProcess++;
> +          CopyMem ((VOID *)&ChunkTransferRequestMessage, (VOID
> *)RequestMessage, sizeof (EFI_HTTP_MESSAGE));
> +        } else {
> +          SendNonChunkProcess++;
> +        }
> +      }
>    }
> -
> +
>  ReSendRequest:;
> -  //
> -  // Send out the request to REST service.
> -  //
> -  if (ItsWrite == TRUE) {
> -    //
> -    // This is write to URI
> -    //
> -    if (SendChunkProcess > HttpIoSendChunkNone) {
> +
> +  if(FixedPcdGetBool(PcdRedfishRestExChunkRequestMode)){
The behavior of Expect-Continue for chunk transfer should be: Send the Expect header first then check if the return HTTP status is Payload Too Large or not. Due to sending Expect is not only for checking the payload too large error and we don't have the code yet for checking HTTP error status yet, we would need another PCD for adding Expect header. Adding the Expect header according to PcdRedfishRestExAddingExpect in RedfishHttpAddExpectation(). With this, we can separate the logic of adding Expect and the Chunk Transfer. Later we can have the code to check HTTP return status for Expect and have the further actions (such as the error handler for the PUT on read only resource URI).
Thanks
Abner

>        //
> -      // This is chunk transfer for writing large payload.
> -      // Send request header first and then handle the
> -      // following request message body using chunk transfer.
> +      // Send the chunked request to REST service.
>        //
> -      do {
> -        Status = HttpIoSendChunkedTransfer (
> +      if (ItsWrite == TRUE) {
> +        //
> +        // This is write to URI
> +        //
> +        if (SendChunkProcess > HttpIoSendChunkNone) {
> +          //
> +          // This is chunk transfer for writing large payload.
> +          // Send request header first and then handle the
> +          // following request message body using chunk transfer.
> +          //
> +          do {
> +            Status = HttpIoSendChunkedTransfer (
> +                       &(Instance->HttpIo),
> +                       &SendChunkProcess,
> +                       &ChunkTransferRequestMessage
> +                       );
> +            if (EFI_ERROR (Status)) {
> +              goto ON_EXIT;
> +            }
> +          } while (SendChunkProcess == HttpIoSendChunkContent ||
> SendChunkProcess == HttpIoSendChunkEndChunk);
> +        } else {
> +          //
> +          // This is the non-chunk transfer, send request header first and then
> +          // handle the following request message body using chunk transfer.
> +          //
> +          Status = HttpIoSendRequest (
> +                     &(Instance->HttpIo),
> +                     (SendNonChunkProcess == HttpIoSendNonChunkContent) ? NULL :
> RequestMessage->Data.Request,
> +                     (SendNonChunkProcess == HttpIoSendNonChunkContent) ? 0 :
> RequestMessage->HeaderCount,
> +                     (SendNonChunkProcess == HttpIoSendNonChunkContent) ? NULL :
> RequestMessage->Headers,
> +                     (SendNonChunkProcess ==
> HttpIoSendNonChunkHeaderZeroContent) ? 0 : RequestMessage->BodyLength,
> +                     (SendNonChunkProcess ==
> HttpIoSendNonChunkHeaderZeroContent) ? NULL : RequestMessage->Body
> +                     );
> +        }
> +      } else {
> +        //
> +        // This is read from URI.
> +        //
> +        Status = HttpIoSendRequest (
>                     &(Instance->HttpIo),
> -                   &SendChunkProcess,
> -                   &ChunkTransferRequestMessage
> +                   RequestMessage->Data.Request,
> +                   RequestMessage->HeaderCount,
> +                   RequestMessage->Headers,
> +                   RequestMessage->BodyLength,
> +                   RequestMessage->Body
>                     );
> -        if (EFI_ERROR (Status)) {
> -          goto ON_EXIT;
> -        }
> -      } while (SendChunkProcess == HttpIoSendChunkContent ||
> SendChunkProcess == HttpIoSendChunkEndChunk);
> -    } else {
> +      }
> +  }
> +  else{
>        //
> -      // This is the non-chunk transfer, send request header first and then
> -      // handle the following request message body using chunk transfer.
> +      // This is normal request to URI.
>        //
>        Status = HttpIoSendRequest (
>                   &(Instance->HttpIo),
> -                 (SendNonChunkProcess == HttpIoSendNonChunkContent) ? NULL :
> RequestMessage->Data.Request,
> -                 (SendNonChunkProcess == HttpIoSendNonChunkContent) ? 0 :
> RequestMessage->HeaderCount,
> -                 (SendNonChunkProcess == HttpIoSendNonChunkContent) ? NULL :
> RequestMessage->Headers,
> -                 (SendNonChunkProcess == HttpIoSendNonChunkHeaderZeroContent) ?
> 0 : RequestMessage->BodyLength,
> -                 (SendNonChunkProcess == HttpIoSendNonChunkHeaderZeroContent) ?
> NULL : RequestMessage->Body
> +                 RequestMessage->Data.Request,
> +                 RequestMessage->HeaderCount,
> +                 RequestMessage->Headers,
> +                 RequestMessage->BodyLength,
> +                 RequestMessage->Body
>                   );
> -    }
> -  } else {
> -    //
> -    // This is read from URI.
> -    //
> -    Status = HttpIoSendRequest (
> -               &(Instance->HttpIo),
> -               RequestMessage->Data.Request,
> -               RequestMessage->HeaderCount,
> -               RequestMessage->Headers,
> -               RequestMessage->BodyLength,
> -               RequestMessage->Body
> -               );
>    }
> -
> +
>    if (EFI_ERROR (Status)) {
>      goto ON_EXIT;
>    }
> @@ -213,7 +233,7 @@ ReSendRequest:;
>    //
>    // Restore the headers if it ever changed in RedfishHttpAddExpectation().
>    //
> -  if (RequestMessage->Headers != PreservedRequestHeaders) {
> +  if (FixedPcdGetBool(PcdRedfishRestExChunkRequestMode) &&
> + RequestMessage->Headers != PreservedRequestHeaders) {
>      FreePool (RequestMessage->Headers);
>      RequestMessage->Headers = PreservedRequestHeaders; // Restore 
> headers before we adding "Expect".
>      RequestMessage->HeaderCount--;                     // Minus one header count for
> "Expect".
> @@ -223,8 +243,8 @@ ReSendRequest:;
>    if (ResponseData->Response.StatusCode == HTTP_STATUS_200_OK) {
>      DEBUG ((DEBUG_INFO, "HTTP_STATUS_200_OK\n"));
>
> -    if (SendChunkProcess == HttpIoSendChunkHeaderZeroContent) {
> -      DEBUG ((DEBUG_INFO, "This is chunk transfer, start to send all chunks."));
> +    if (FixedPcdGetBool(PcdRedfishRestExChunkRequestMode) &&
> SendChunkProcess == HttpIoSendChunkHeaderZeroContent) {
> +      DEBUG ((DEBUG_INFO, "This is chunk transfer, start to send all 
> + chunks.", ResponseData->Response.StatusCode));
>        SendChunkProcess++;
>        goto ReSendRequest;
>      }
> @@ -240,14 +260,14 @@ ReSendRequest:;
>      goto ON_EXIT;
>    } else if (ResponseData->Response.StatusCode ==
> HTTP_STATUS_400_BAD_REQUEST) {
>      DEBUG ((DEBUG_INFO, "HTTP_STATUS_400_BAD_REQUEST\n"));
> -    if (SendChunkProcess == HttpIoSendChunkHeaderZeroContent) {
> +    if (FixedPcdGetBool(PcdRedfishRestExChunkRequestMode) && 
> + SendChunkProcess == HttpIoSendChunkHeaderZeroContent) {
>        DEBUG ((DEBUG_INFO, "Bad request may caused by zero length 
> chunk. Try to send all chunks...\n"));
>        SendChunkProcess++;
>        goto ReSendRequest;
>      }
>    } else if (ResponseData->Response.StatusCode ==
> HTTP_STATUS_100_CONTINUE) {
>      DEBUG ((DEBUG_INFO, "HTTP_STATUS_100_CONTINUE\n"));
> -    if (SendChunkProcess == HttpIoSendChunkHeaderZeroContent) {
> +    if (FixedPcdGetBool(PcdRedfishRestExChunkRequestMode) && 
> + SendChunkProcess == HttpIoSendChunkHeaderZeroContent) {
>        //
>        // We get HTTP_STATUS_100_CONTINUE to send the body using chunk 
> transfer.
>        //
> @@ -256,7 +276,7 @@ ReSendRequest:;
>        goto ReSendRequest;
>      }
>
> -    if (SendNonChunkProcess == HttpIoSendNonChunkHeaderZeroContent) {
> +    if (FixedPcdGetBool(PcdRedfishRestExChunkRequestMode) && 
> + SendNonChunkProcess == HttpIoSendNonChunkHeaderZeroContent) {
>        DEBUG ((DEBUG_INFO, "HTTP_STATUS_100_CONTINUE for non chunk 
> transfer...\n"));
>        SendNonChunkProcess++;
>        goto ReSendRequest;
> --
> 2.6.1.windows.1
> -The information contained in this message may be confidential and 
> proprietary to American Megatrends (AMI). This communication is 
> intended to be read only by the individual or entity to whom it is 
> addressed or by their designee. If the reader of this message is not 
> the intended recipient, you are on notice that any distribution of 
> this message, in any form, is strictly prohibited. Please promptly 
> notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
>
>
> 
>