Re: [edk2-devel] [PATCH] NetworkPkg/DxeHttpLib: Migrate HTTP header manipulation APIs

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

Hi Maciej,
Patch v2 sent.

Thanks
Abner

> -----Original Message-----
> From: Chang, Abner (HPS SW/FW Technologist)
> Sent: Wednesday, January 6, 2021 1:28 PM
> To: Rabeda, Maciej <maciej.rabeda@linux.intel.com>; devel@edk2.groups.io
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Fan
> Wang <fan.wang@intel.com>; Jiewen Yao <jiewen.yao@intel.com>; Wang,
> Nickle (HPS SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley@hpe.com>
> Subject: RE: [edk2-devel] [PATCH] NetworkPkg/DxeHttpLib: Migrate HTTP
> header manipulation APIs
> 
> 
> 
> > -----Original Message-----
> > From: Rabeda, Maciej [mailto:maciej.rabeda@linux.intel.com]
> > Sent: Tuesday, January 5, 2021 10:49 PM
> > To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> > <abner.chang@hpe.com>
> > Cc: Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>;
> > Fan Wang <fan.wang@intel.com>; Jiewen Yao <jiewen.yao@intel.com>;
> > Wang, Nickle (HPS SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> > <peter.ohanley@hpe.com>
> > Subject: Re: [edk2-devel] [PATCH] NetworkPkg/DxeHttpLib: Migrate HTTP
> > header manipulation APIs
> >
> > Hi Abner,
> >
> > Sorry for the late response, I was on vacation.
> > One quick thing: I can see that the functions are simply being moved
> > from one place to the other and renamed to HttpIo___().
> > The only issue I have is that this patch does not remove HttpBoot___()
> > functions from HttpDxe/HttpBootSupport.c Upon removing those, I will
> > give my approval :)
> I was originally thought to have a Bugzilla for HttpBootSupport and let owner
> to deal with that. :-) Will send the patch for this.
> 
> Thanks
> Abner
> >
> > Thanks,
> > Maciej
> >
> > On 18-Dec-20 03:39, Abner Chang wrote:
> > > Move HTTP header manipulation functions to DxeHttpLib from
> > > HttpBootSupport.c. These general functions are used by both Http
> > > BOOT and RedfishLib (patches will be sent later).
> > >
> > > Signed-off-by: Abner Chang <abner.chang@hpe.com>
> > >
> > > Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> > > Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> > > Cc: Siyuan Fu <siyuan.fu@intel.com>
> > > Cc: Fan Wang <fan.wang@intel.com>
> > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > Cc: Nickle Wang <nickle.wang@hpe.com>
> > > Cc: Peter O'Hanley <peter.ohanley@hpe.com>
> > > ---
> > >   NetworkPkg/HttpBootDxe/HttpBootSupport.h   |  53 --------
> > >   NetworkPkg/Include/Library/HttpLib.h       |  53 ++++++++
> > >   NetworkPkg/HttpBootDxe/HttpBootClient.c    |  10 +-
> > >   NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c | 135
> > ++++++++++++++++++++-
> > >   4 files changed, 192 insertions(+), 59 deletions(-)
> > >
> > > diff --git a/NetworkPkg/HttpBootDxe/HttpBootSupport.h
> > > b/NetworkPkg/HttpBootDxe/HttpBootSupport.h
> > > index 1a2d32dd5a..c2ac1b785a 100644
> > > --- a/NetworkPkg/HttpBootDxe/HttpBootSupport.h
> > > +++ b/NetworkPkg/HttpBootDxe/HttpBootSupport.h
> > > @@ -87,59 +87,6 @@ HttpBootPrintErrorMessage (
> > >     EFI_HTTP_STATUS_CODE            StatusCode
> > >     );
> > >
> > > -//
> > > -// A wrapper structure to hold the HTTP headers.
> > > -//
> > > -typedef struct {
> > > -  UINTN                       MaxHeaderCount;
> > > -  UINTN                       HeaderCount;
> > > -  EFI_HTTP_HEADER             *Headers;
> > > -} HTTP_IO_HEADER;
> > > -
> > > -/**
> > > -  Create a HTTP_IO_HEADER to hold the HTTP header items.
> > > -
> > > -  @param[in]  MaxHeaderCount         The maximum number of HTTP
> > header in this holder.
> > > -
> > > -  @return    A pointer of the HTTP header holder or NULL if failed.
> > > -
> > > -**/
> > > -HTTP_IO_HEADER *
> > > -HttpBootCreateHeader (
> > > -  IN  UINTN                MaxHeaderCount
> > > -  );
> > > -
> > > -/**
> > > -  Destroy the HTTP_IO_HEADER and release the resources.
> > > -
> > > -  @param[in]  HttpIoHeader       Point to the HTTP header holder to be
> > destroyed.
> > > -
> > > -**/
> > > -VOID
> > > -HttpBootFreeHeader (
> > > -  IN  HTTP_IO_HEADER       *HttpIoHeader
> > > -  );
> > > -
> > > -/**
> > > -  Set or update a HTTP header with the field name and corresponding
> > value.
> > > -
> > > -  @param[in]  HttpIoHeader       Point to the HTTP header holder.
> > > -  @param[in]  FieldName          Null terminated string which describes a
> field
> > name.
> > > -  @param[in]  FieldValue         Null terminated string which describes the
> > corresponding field value.
> > > -
> > > -  @retval  EFI_SUCCESS           The HTTP header has been set or updated.
> > > -  @retval  EFI_INVALID_PARAMETER Any input parameter is invalid.
> > > -  @retval  EFI_OUT_OF_RESOURCES  Insufficient resource to complete
> > > the
> > operation.
> > > -  @retval  Other                 Unexpected error happened.
> > > -
> > > -**/
> > > -EFI_STATUS
> > > -HttpBootSetHeader (
> > > -  IN  HTTP_IO_HEADER       *HttpIoHeader,
> > > -  IN  CHAR8                *FieldName,
> > > -  IN  CHAR8                *FieldValue
> > > -  );
> > > -
> > >   /**
> > >     Retrieve the host address using the EFI_DNS6_PROTOCOL.
> > >
> > > diff --git a/NetworkPkg/Include/Library/HttpLib.h
> > > b/NetworkPkg/Include/Library/HttpLib.h
> > > index a906126b3d..2c3367fb01 100644
> > > --- a/NetworkPkg/Include/Library/HttpLib.h
> > > +++ b/NetworkPkg/Include/Library/HttpLib.h
> > > @@ -476,6 +476,59 @@ HttpIsValidHttpHeader (
> > >     IN  CHAR8            *FieldName
> > >     );
> > >
> > > +//
> > > +// A wrapper structure to hold the HTTP headers.
> > > +//
> > > +typedef struct {
> > > +  UINTN                       MaxHeaderCount;
> > > +  UINTN                       HeaderCount;
> > > +  EFI_HTTP_HEADER             *Headers;
> > > +} HTTP_IO_HEADER;
> > > +
> > > +
> > > +/**
> > > +  Create a HTTP_IO_HEADER to hold the HTTP header items.
> > > +
> > > +  @param[in]  MaxHeaderCount         The maximun number of HTTP
> > header in this holder.
> > > +
> > > +  @return    A pointer of the HTTP header holder or NULL if failed.
> > > +
> > > +**/
> > > +HTTP_IO_HEADER *
> > > +HttpIoCreateHeader (
> > > +  UINTN                     MaxHeaderCount
> > > +  );
> > > +
> > > +/**
> > > +  Destroy the HTTP_IO_HEADER and release the resources.
> > > +
> > > +  @param[in]  HttpIoHeader       Point to the HTTP header holder to be
> > destroyed.
> > > +
> > > +**/
> > > +VOID
> > > +HttpIoFreeHeader (
> > > +  IN  HTTP_IO_HEADER       *HttpIoHeader
> > > +  );
> > > +
> > > +/**
> > > +  Set or update a HTTP header with the field name and corresponding
> > value.
> > > +
> > > +  @param[in]  HttpIoHeader       Point to the HTTP header holder.
> > > +  @param[in]  FieldName          Null terminated string which describes a
> > field name.
> > > +  @param[in]  FieldValue         Null terminated string which describes the
> > corresponding field value.
> > > +
> > > +  @retval  EFI_SUCCESS           The HTTP header has been set or updated.
> > > +  @retval  EFI_INVALID_PARAMETER Any input parameter is invalid.
> > > +  @retval  EFI_OUT_OF_RESOURCES  Insufficient resource to complete
> > > + the
> > operation.
> > > +  @retval  Other                 Unexpected error happened.
> > > +
> > > +**/
> > > +EFI_STATUS
> > > +HttpIoSetHeader (
> > > +  IN  HTTP_IO_HEADER       *HttpIoHeader,
> > > +  IN  CHAR8                *FieldName,
> > > +  IN  CHAR8                *FieldValue
> > > +  );
> > >
> > >   #endif
> > >
> > > diff --git a/NetworkPkg/HttpBootDxe/HttpBootClient.c
> > > b/NetworkPkg/HttpBootDxe/HttpBootClient.c
> > > index 30ac15889b..8f21f7766e 100644
> > > --- a/NetworkPkg/HttpBootDxe/HttpBootClient.c
> > > +++ b/NetworkPkg/HttpBootDxe/HttpBootClient.c
> > > @@ -977,7 +977,7 @@ HttpBootGetBootFile (
> > >     //       Accept
> > >     //       User-Agent
> > >     //
> > > -  HttpIoHeader = HttpBootCreateHeader (3);
> > > +  HttpIoHeader = HttpIoCreateHeader (3);
> > >     if (HttpIoHeader == NULL) {
> > >       Status = EFI_OUT_OF_RESOURCES;
> > >       goto ERROR_2;
> > > @@ -995,7 +995,7 @@ HttpBootGetBootFile (
> > >     if (EFI_ERROR (Status)) {
> > >       goto ERROR_3;
> > >     }
> > > -  Status = HttpBootSetHeader (
> > > +  Status = HttpIoSetHeader (
> > >                HttpIoHeader,
> > >                HTTP_HEADER_HOST,
> > >                HostName
> > > @@ -1008,7 +1008,7 @@ HttpBootGetBootFile (
> > >     //
> > >     // Add HTTP header field 2: Accept
> > >     //
> > > -  Status = HttpBootSetHeader (
> > > +  Status = HttpIoSetHeader (
> > >                HttpIoHeader,
> > >                HTTP_HEADER_ACCEPT,
> > >                "*/*"
> > > @@ -1020,7 +1020,7 @@ HttpBootGetBootFile (
> > >     //
> > >     // Add HTTP header field 3: User-Agent
> > >     //
> > > -  Status = HttpBootSetHeader (
> > > +  Status = HttpIoSetHeader (
> > >                HttpIoHeader,
> > >                HTTP_HEADER_USER_AGENT,
> > >                HTTP_USER_AGENT_EFI_HTTP_BOOT @@ -1291,7 +1291,7 @@
> > > ERROR_4:
> > >       FreePool (RequestData);
> > >     }
> > >   ERROR_3:
> > > -  HttpBootFreeHeader (HttpIoHeader);
> > > +  HttpIoFreeHeader (HttpIoHeader);
> > >   ERROR_2:
> > >     if (Cache != NULL) {
> > >       FreePool (Cache);
> > > diff --git a/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c
> > > b/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c
> > > index daec1e0226..7354e0170f 100644
> > > --- a/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c
> > > +++ b/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c
> > > @@ -3,7 +3,7 @@
> > >     It provides the helper routines to parse the HTTP message byte stream.
> > >
> > >   Copyright (c) 2015 - 2019, Intel Corporation. All rights
> > > reserved.<BR>
> > > -(C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> > > +(C) Copyright 2016 - 2020  Hewlett Packard Enterprise Development
> > > +LP<BR>
> > >   SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > >   **/
> > > @@ -2095,3 +2095,136 @@ HttpIsValidHttpHeader (
> > >     return TRUE;
> > >   }
> > >
> > > +
> > > +/**
> > > +  Create a HTTP_IO_HEADER to hold the HTTP header items.
> > > +
> > > +  @param[in]  MaxHeaderCount         The maximun number of HTTP
> > header in this holder.
> > > +
> > > +  @return    A pointer of the HTTP header holder or NULL if failed.
> > > +
> > > +**/
> > > +HTTP_IO_HEADER *
> > > +HttpIoCreateHeader (
> > > +  UINTN                     MaxHeaderCount
> > > +  )
> > > +{
> > > +  HTTP_IO_HEADER        *HttpIoHeader;
> > > +
> > > +  if (MaxHeaderCount == 0) {
> > > +    return NULL;
> > > +  }
> > > +
> > > +  HttpIoHeader = AllocateZeroPool (sizeof (HTTP_IO_HEADER) +
> > > + MaxHeaderCount * sizeof (EFI_HTTP_HEADER));  if (HttpIoHeader ==
> > NULL) {
> > > +    return NULL;
> > > +  }
> > > +
> > > +  HttpIoHeader->MaxHeaderCount = MaxHeaderCount;
> > > + HttpIoHeader->Headers = (EFI_HTTP_HEADER *) (HttpIoHeader + 1);
> > > +
> > > +  return HttpIoHeader;
> > > +}
> > > +
> > > +/**
> > > +  Destroy the HTTP_IO_HEADER and release the resources.
> > > +
> > > +  @param[in]  HttpIoHeader       Point to the HTTP header holder to be
> > destroyed.
> > > +
> > > +**/
> > > +VOID
> > > +HttpIoFreeHeader (
> > > +  IN  HTTP_IO_HEADER       *HttpIoHeader
> > > +  )
> > > +{
> > > +  UINTN      Index;
> > > +
> > > +  if (HttpIoHeader != NULL) {
> > > +    if (HttpIoHeader->HeaderCount != 0) {
> > > +      for (Index = 0; Index < HttpIoHeader->HeaderCount; Index++) {
> > > +        FreePool (HttpIoHeader->Headers[Index].FieldName);
> > > +        ZeroMem (HttpIoHeader->Headers[Index].FieldValue,
> > > + AsciiStrSize
> > (HttpIoHeader->Headers[Index].FieldValue));
> > > +        FreePool (HttpIoHeader->Headers[Index].FieldValue);
> > > +      }
> > > +    }
> > > +    FreePool (HttpIoHeader);
> > > +  }
> > > +}
> > > +
> > > +/**
> > > +  Set or update a HTTP header with the field name and corresponding
> > value.
> > > +
> > > +  @param[in]  HttpIoHeader       Point to the HTTP header holder.
> > > +  @param[in]  FieldName          Null terminated string which describes a
> > field name.
> > > +  @param[in]  FieldValue         Null terminated string which describes the
> > corresponding field value.
> > > +
> > > +  @retval  EFI_SUCCESS           The HTTP header has been set or updated.
> > > +  @retval  EFI_INVALID_PARAMETER Any input parameter is invalid.
> > > +  @retval  EFI_OUT_OF_RESOURCES  Insufficient resource to complete
> > > + the
> > operation.
> > > +  @retval  Other                 Unexpected error happened.
> > > +
> > > +**/
> > > +EFI_STATUS
> > > +HttpIoSetHeader (
> > > +  IN  HTTP_IO_HEADER       *HttpIoHeader,
> > > +  IN  CHAR8                *FieldName,
> > > +  IN  CHAR8                *FieldValue
> > > +  )
> > > +{
> > > +  EFI_HTTP_HEADER       *Header;
> > > +  UINTN                 StrSize;
> > > +  CHAR8                 *NewFieldValue;
> > > +
> > > +  if (HttpIoHeader == NULL || FieldName == NULL || FieldValue == NULL)
> {
> > > +    return EFI_INVALID_PARAMETER;
> > > +  }
> > > +
> > > +  Header = HttpFindHeader (HttpIoHeader->HeaderCount,
> > > + HttpIoHeader->Headers, FieldName);  if (Header == NULL) {
> > > +    //
> > > +    // Add a new header.
> > > +    //
> > > +    if (HttpIoHeader->HeaderCount >= HttpIoHeader->MaxHeaderCount)
> {
> > > +      return EFI_OUT_OF_RESOURCES;
> > > +    }
> > > +    Header = &HttpIoHeader->Headers[HttpIoHeader->HeaderCount];
> > > +
> > > +    StrSize = AsciiStrSize (FieldName);
> > > +    Header->FieldName = AllocatePool (StrSize);
> > > +    if (Header->FieldName == NULL) {
> > > +      return EFI_OUT_OF_RESOURCES;
> > > +    }
> > > +    CopyMem (Header->FieldName, FieldName, StrSize);
> > > +    Header->FieldName[StrSize -1] = '\0';
> > > +
> > > +    StrSize = AsciiStrSize (FieldValue);
> > > +    Header->FieldValue = AllocatePool (StrSize);
> > > +    if (Header->FieldValue == NULL) {
> > > +      FreePool (Header->FieldName);
> > > +      return EFI_OUT_OF_RESOURCES;
> > > +    }
> > > +    CopyMem (Header->FieldValue, FieldValue, StrSize);
> > > +    Header->FieldValue[StrSize -1] = '\0';
> > > +
> > > +    HttpIoHeader->HeaderCount++;
> > > +  } else {
> > > +    //
> > > +    // Update an existing one.
> > > +    //
> > > +    StrSize = AsciiStrSize (FieldValue);
> > > +    NewFieldValue = AllocatePool (StrSize);
> > > +    if (NewFieldValue == NULL) {
> > > +      return EFI_OUT_OF_RESOURCES;
> > > +    }
> > > +    CopyMem (NewFieldValue, FieldValue, StrSize);
> > > +    NewFieldValue[StrSize -1] = '\0';
> > > +
> > > +    if (Header->FieldValue != NULL) {
> > > +      FreePool (Header->FieldValue);
> > > +    }
> > > +    Header->FieldValue = NewFieldValue;  }
> > > +
> > > +  return EFI_SUCCESS;
> > > +}
> > > +