Re: [edk2-devel] [PATCH] RedfishPkg/RedfishRestExDxe: return HTTP status code to caller.

["Mike Maslenkin" <mike.maslenkin@gmail.com>]

On Sat, Jul 22, 2023 at 11:18 AM Nickle Wang via groups.io
<nicklew=nvidia.com@groups.io> wrote:
>
> Return unsupported HTTP status code to caller so caller can handle
> HTTP error status code. Current implementation only return EFI error
> to caller. Without knowing the HTTP status code, caller has trouble
> to handle HTTP request failure.
>
> Signed-off-by: Nickle Wang <nicklew@nvidia.com>
> Cc: Abner Chang <abner.chang@amd.com>
> Cc: Igor Kulchytskyy <igork@ami.com>
> Cc: Nick Ramirez <nramirez@nvidia.com>
> ---
>  .../RedfishRestExDxe/RedfishRestExProtocol.c    | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/RedfishPkg/RedfishRestExDxe/RedfishRestExProtocol.c b/RedfishPkg/RedfishRestExDxe/RedfishRestExProtocol.c
> index 90973619f2bc..f11cee8542fb 100644
> --- a/RedfishPkg/RedfishRestExDxe/RedfishRestExProtocol.c
> +++ b/RedfishPkg/RedfishRestExDxe/RedfishRestExProtocol.c
> @@ -320,6 +320,18 @@ ReSendRequest:;
>      DEBUG ((DEBUG_ERROR, "This HTTP Status is not handled!\n"));
>      DumpHttpStatusCode (DEBUG_REDFISH_NETWORK, ResponseData->Response.StatusCode);
>      Status = EFI_UNSUPPORTED;
> +
> +    //
> +    // Deliver status code back to caller so caller can handle it.
> +    //
> +    ResponseMessage->Data.Response = AllocateZeroPool (sizeof (EFI_HTTP_RESPONSE_DATA));
> +    if (ResponseMessage->Data.Response == NULL) {
> +      Status = EFI_OUT_OF_RESOURCES;
> +      goto ON_EXIT;
> +    }
> +
> +    ResponseMessage->Data.Response->StatusCode = ResponseData->Response.StatusCode;
> +
>      goto ON_EXIT;
>    }
>
> @@ -444,11 +456,6 @@ ON_EXIT:
>    }
>
>    if (EFI_ERROR (Status)) {
> -    if (ResponseMessage->Data.Response != NULL) {
> -      FreePool (ResponseMessage->Data.Response);
> -      ResponseMessage->Data.Response = NULL;
> -    }
> -
This doesn't make code cleaner.
What about resource deallocation after  AllocateZeroPool (sizeof
(EFI_HTTP_RESPONSE_DATA)) for the common case?
I guess this resource deallocation was added for this and there are a
lot of 'goto ON_EXIT;' after original AllocateZeroPool call.
I would suggest to remove this `if (EFI_ERROR (Status)) {` block at all.
As I can see, the only callers of this function are
RedfishPkg//PrivateLibrary/RedfishLib/edk2libredfish/src/service.c and
edk2-redfish-client//RedfishClientPkg/PrivateLibrary/RedfishLib/edk2libredfish/src/service.c.
And both use the same pattern of an explicit response data
deallocation (RestConfigFreeHttpMessage() call)  in case of error
status returned.
I.e. caller is responsible for the response data deallocation as it owns it.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107142): https://edk2.groups.io/g/devel/message/107142
Mute This Topic: https://groups.io/mt/100292372/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-