From: "Nickle Wang via groups.io" <nicklew=nvidia.com@groups.io>
To: Mike Maslenkin <mike.maslenkin@gmail.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: Abner Chang <abner.chang@amd.com>,
Igor Kulchytskyy <igork@ami.com>,
Nick Ramirez <nramirez@nvidia.com>
Subject: Re: [edk2-devel] [PATCH] RedfishPkg/RedfishRestExDxe: return HTTP status code to caller.
Date: Thu, 3 Aug 2023 06:06:06 +0000 [thread overview]
Message-ID: <MW4PR12MB7031910EA789EE4E93EF7F63D908A@MW4PR12MB7031.namprd12.prod.outlook.com> (raw)
In-Reply-To: <CAL77WPC6icX9KbHUeRHfoTXojPR4Cbk8t=t2VMnFSYfLSYkChA@mail.gmail.com>
Hi Make,
> I would suggest to remove this `if (EFI_ERROR (Status)) {` block at all.
Are you suggesting to not release "ResponseMessage->Body"[1] while error happens because caller will handle this too?
[1]: Line 452 - 454 at: https://github.com/tianocore/edk2/pull/4672/files#diff-18a31a76c495b9a145b0ffa6a68fe5de887cbdddc925c5eb3a759e81694f59c9L452
Thanks,
Nickle
> -----Original Message-----
> From: Mike Maslenkin <mike.maslenkin@gmail.com>
> Sent: Saturday, July 22, 2023 8:09 PM
> To: devel@edk2.groups.io; Nickle Wang <nicklew@nvidia.com>
> Cc: Abner Chang <abner.chang@amd.com>; Igor Kulchytskyy <igork@ami.com>;
> Nick Ramirez <nramirez@nvidia.com>
> Subject: Re: [edk2-devel] [PATCH] RedfishPkg/RedfishRestExDxe: return HTTP
> status code to caller.
>
> External email: Use caution opening links or attachments
>
>
> 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 (#107517): https://edk2.groups.io/g/devel/message/107517
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]
-=-=-=-=-=-=-=-=-=-=-=-
next prev parent reply other threads:[~2023-08-03 6:06 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-22 8:18 [edk2-devel] [PATCH] RedfishPkg/RedfishRestExDxe: return HTTP status code to caller Nickle Wang via groups.io
2023-07-22 12:09 ` Mike Maslenkin
2023-08-03 6:06 ` Nickle Wang via groups.io [this message]
2023-08-03 10:16 ` Mike Maslenkin
2023-09-18 13:15 ` Nickle Wang via groups.io
2023-09-19 13:53 ` Mike Maslenkin
2023-09-19 14:05 ` Nickle Wang via groups.io
2023-07-24 2:05 ` Chang, Abner via groups.io
2023-07-24 2:06 ` Igor Kulchytskyy via groups.io
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MW4PR12MB7031910EA789EE4E93EF7F63D908A@MW4PR12MB7031.namprd12.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox