Hi Jiaxin, After an attempt of HTTPBoot, we keep the State as HTTP_STATE_TCP_CONNECTED. With your suggestion, I see that alternate attempts of HTTPBoot is failing. In Wireshark, I see that we send a RST following a GET request for each failed alternate boot attempts. So, we might not want to have that condition in place.
Hi Siyuan, Do you have any additional feedback on this patch? Regards, Nagaraj. -----Original Message----- From: Wu, Jiaxin [mailto:jiaxin...@intel.com] Sent: Monday, May 09, 2016 7:28 AM To: Hegde, Nagaraj P <nagaraj-p.he...@hpe.com>; edk2-devel@lists.01.org Cc: Fu, Siyuan <siyuan...@intel.com>; Ye, Ting <ting...@intel.com>; El-Haj-Mahmoud, Samer <samer.el-haj-mahm...@hpe.com> Subject: RE: [PATCH v2 1/1] NetworkPkg:HttpDxe: Code changes to support HTTP PUT/POST operations Hi Nagaraj, In every EfiHttpRequest() for PUT/POST, HttpInitTcp() will be called even the TCP state is established (get from TCP4/6 GetModeData each time) and return EFI_SUCCESS. To avoid this repeating function call and improve the performance, we can check the current Http state, if required, then call it: if (HttpInstance->State != HTTP_STATE_TCP_CONNECTED) { Status = HttpInitTcp (HttpInstance, Wrap, Configure); if (EFI_ERROR (Status)) { goto Error2; } } Others is good to me. Reviewed-By: Wu Jiaxin <jiaxin...@intel.com> Thanks. Jiaxin > -----Original Message----- > From: Nagaraj Hegde [mailto:nagaraj-p.he...@hpe.com] > Sent: Friday, May 6, 2016 6:20 PM > To: edk2-devel@lists.01.org > Cc: Wu, Jiaxin <jiaxin...@intel.com>; Fu, Siyuan > <siyuan...@intel.com>; Ye, Ting <ting...@intel.com>; > samer.el-haj-mahm...@hpe.com > Subject: [PATCH v2 1/1] NetworkPkg:HttpDxe: Code changes to support > HTTP PUT/POST operations > > v2: Add comments for the code changes. > > Code changes enables HttpDxe to handle PUT/POST operations. > EfiHttpRequest assumes "Request" and "HttpMsg->Headers" can never be > NULL. Also, HttpResponseWorker assumes HTTP Reponse will contain > headers. We could have response which could contain only a string > (HTTP 100 > Continue) and no headers. Code changes tries to do-away from these > assumptions, which would enable HttpDxe to support PUT/POST operations. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Hegde, Nagaraj P nagaraj-p.he...@hpe.com > --- > NetworkPkg/HttpDxe/HttpDriver.c | 3 + > NetworkPkg/HttpDxe/HttpImpl.c | 445 ++++++++++++-------- > NetworkPkg/HttpDxe/HttpProto.h | 1 + > 3 files changed, 264 insertions(+), 185 deletions(-) > > diff --git a/NetworkPkg/HttpDxe/HttpDriver.c > b/NetworkPkg/HttpDxe/HttpDriver.c index 2518f4e..da6d087 100644 > --- a/NetworkPkg/HttpDxe/HttpDriver.c > +++ b/NetworkPkg/HttpDxe/HttpDriver.c > @@ -2,6 +2,7 @@ > The driver binding and service binding protocol for HttpDxe driver. > > Copyright (c) 2015, Intel Corporation. All rights reserved.<BR> > + (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR> > > This program and the accompanying materials > are licensed and made available under the terms and conditions of > the BSD License @@ -939,6 +940,8 @@ HttpServiceBindingCreateChild ( > > HttpInstance->Signature = HTTP_PROTOCOL_SIGNATURE; > HttpInstance->Service = HttpService; > + HttpInstance->Method = HttpMethodMax; > + > CopyMem (&HttpInstance->Http, &mEfiHttpTemplate, sizeof > (HttpInstance->Http)); > NetMapInit (&HttpInstance->TxTokens); > NetMapInit (&HttpInstance->RxTokens); diff --git > a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c index > 7a236f4..9bc0f8a 100644 > --- a/NetworkPkg/HttpDxe/HttpImpl.c > +++ b/NetworkPkg/HttpDxe/HttpImpl.c > @@ -248,151 +248,185 @@ EfiHttpRequest ( > HTTP_TOKEN_WRAP *Wrap; > CHAR8 *FileUrl; > UINTN RequestMsgSize; > - > + > + // > + // Initializations > + // > + Url = NULL; > + HostName = NULL; > + RequestMsg = NULL; > + HostNameStr = NULL; > + Wrap = NULL; > + FileUrl = NULL; > + > if ((This == NULL) || (Token == NULL)) { > return EFI_INVALID_PARAMETER; > } > > HttpMsg = Token->Message; > - if ((HttpMsg == NULL) || (HttpMsg->Headers == NULL)) { > + if (HttpMsg == NULL) { > return EFI_INVALID_PARAMETER; > } > > - // > - // Current implementation does not support POST/PUT method. > - // If future version supports these two methods, Request could be > NULL for a special case that to send large amounts > - // of data. For this case, the implementation need check whether > previous call to Request() has been completed or not. > - // > - // > Request = HttpMsg->Data.Request; > - if ((Request == NULL) || (Request->Url == NULL)) { > - return EFI_INVALID_PARAMETER; > - } > > // > - // Only support GET and HEAD method in current implementation. > + // Only support GET, HEAD, PUT and POST method in current > implementation. > // > - if ((Request->Method != HttpMethodGet) && (Request->Method != > HttpMethodHead)) { > + if ((Request != NULL) && (Request->Method != HttpMethodGet) && > + (Request->Method != HttpMethodHead) && (Request->Method != > + HttpMethodPut) && (Request->Method != HttpMethodPost)) { > return EFI_UNSUPPORTED; > } > > HttpInstance = HTTP_INSTANCE_FROM_PROTOCOL (This); > ASSERT (HttpInstance != NULL); > > + // > + // Capture the method into HttpInstance. > + // > + if (Request != NULL) { > + HttpInstance->Method = Request->Method; } > + > if (HttpInstance->State < HTTP_STATE_HTTP_CONFIGED) { > return EFI_NOT_STARTED; > } > > - // > - // Check whether the token already existed. > - // > - if (EFI_ERROR (NetMapIterate (&HttpInstance->TxTokens, > HttpTokenExist, > Token))) { > - return EFI_ACCESS_DENIED; > - } > - > - HostName = NULL; > - Wrap = NULL; > - HostNameStr = NULL; > - > - // > - // Parse the URI of the remote host. > - // > - Url = HttpInstance->Url; > - UrlLen = StrLen (Request->Url) + 1; > - if (UrlLen > HTTP_URL_BUFFER_LEN) { > - Url = AllocateZeroPool (UrlLen); > - if (Url == NULL) { > - return EFI_OUT_OF_RESOURCES; > + if (Request == NULL) { > + // > + // Request would be NULL only for PUT/POST operation (in the > + current > implementation) > + // > + if ((HttpInstance->Method != HttpMethodPut) && (HttpInstance- > >Method != HttpMethodPost)) { > + return EFI_INVALID_PARAMETER; > + } > + > + // > + // For PUT/POST, we need to have the TCP already configured. Bail > + out if > it is not! > + // > + if (HttpInstance->State < HTTP_STATE_TCP_CONFIGED) { > + return EFI_INVALID_PARAMETER; > + } > + > + // > + // We need to have the Message Body for sending the HTTP message > across in these cases. > + // > + if (HttpMsg->Body == NULL || HttpMsg->BodyLength == 0) { > + return EFI_INVALID_PARAMETER; > } > - FreePool (HttpInstance->Url); > - HttpInstance->Url = Url; > - } > - > > - UnicodeStrToAsciiStr (Request->Url, Url); > - UrlParser = NULL; > - Status = HttpParseUrl (Url, (UINT32) AsciiStrLen (Url), FALSE, > &UrlParser); > - if (EFI_ERROR (Status)) { > - goto Error1; > - } > - > - RequestMsg = NULL; > - HostName = NULL; > - Status = HttpUrlGetHostName (Url, UrlParser, &HostName); > - if (EFI_ERROR (Status)) { > - goto Error1; > - } > - > - Status = HttpUrlGetPort (Url, UrlParser, &RemotePort); > - if (EFI_ERROR (Status)) { > - RemotePort = HTTP_DEFAULT_PORT; > - } > - // > - // If Configure is TRUE, it indicates the first time to call > Request(); > - // If ReConfigure is TRUE, it indicates the request URL is not same > - // with the previous call to Request(); > - // > - Configure = TRUE; > - ReConfigure = TRUE; > - > - if (HttpInstance->RemoteHost == NULL) { > + // > + // Use existing TCP instance to transmit the packet. > + // > + Configure = FALSE; > + ReConfigure = FALSE; > + } else { > + // > + // Check whether the token already existed. > + // > + if (EFI_ERROR (NetMapIterate (&HttpInstance->TxTokens, > HttpTokenExist, Token))) { > + return EFI_ACCESS_DENIED; > + } > + > + // > + // Parse the URI of the remote host. > + // > + Url = HttpInstance->Url; > + UrlLen = StrLen (Request->Url) + 1; > + if (UrlLen > HTTP_URL_BUFFER_LEN) { > + Url = AllocateZeroPool (UrlLen); > + if (Url == NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + FreePool (HttpInstance->Url); > + HttpInstance->Url = Url; > + } > + > + > + UnicodeStrToAsciiStr (Request->Url, Url); > + UrlParser = NULL; > + Status = HttpParseUrl (Url, (UINT32) AsciiStrLen (Url), FALSE, > &UrlParser); > + if (EFI_ERROR (Status)) { > + goto Error1; > + } > + > + HostName = NULL; > + Status = HttpUrlGetHostName (Url, UrlParser, &HostName); > + if (EFI_ERROR (Status)) { > + goto Error1; > + } > + > + Status = HttpUrlGetPort (Url, UrlParser, &RemotePort); > + if (EFI_ERROR (Status)) { > + RemotePort = HTTP_DEFAULT_PORT; > + } > // > - // Request() is called the first time. > + // If Configure is TRUE, it indicates the first time to call Request(); > + // If ReConfigure is TRUE, it indicates the request URL is not same > + // with the previous call to Request(); > // > - ReConfigure = FALSE; > - } else { > - if ((HttpInstance->RemotePort == RemotePort) && > - (AsciiStrCmp (HttpInstance->RemoteHost, HostName) == 0)) { > + Configure = TRUE; > + ReConfigure = TRUE; > + > + if (HttpInstance->RemoteHost == NULL) { > // > - // Host Name and port number of the request URL are the same with > previous call to Request(). > - // Check whether previous TCP packet sent out. > + // Request() is called the first time. > // > - if (EFI_ERROR (NetMapIterate (&HttpInstance->TxTokens, > HttpTcpNotReady, NULL))) { > + ReConfigure = FALSE; > + } else { > + if ((HttpInstance->RemotePort == RemotePort) && > + (AsciiStrCmp (HttpInstance->RemoteHost, HostName) == 0)) { > // > - // Wrap the HTTP token in HTTP_TOKEN_WRAP > + // Host Name and port number of the request URL are the same > + with > previous call to Request(). > + // Check whether previous TCP packet sent out. > // > - Wrap = AllocateZeroPool (sizeof (HTTP_TOKEN_WRAP)); > - if (Wrap == NULL) { > - Status = EFI_OUT_OF_RESOURCES; > - goto Error1; > - } > - > - Wrap->HttpToken = Token; > - Wrap->HttpInstance = HttpInstance; > - > - Status = HttpCreateTcpTxEvent (Wrap); > - if (EFI_ERROR (Status)) { > - goto Error1; > - } > - > - Status = NetMapInsertTail (&HttpInstance->TxTokens, Token, Wrap); > - if (EFI_ERROR (Status)) { > - goto Error1; > - } > - > - Wrap->TcpWrap.Method = Request->Method; > > - FreePool (HostName); > - > - // > - // Queue the HTTP token and return. > - // > - return EFI_SUCCESS; > + if (EFI_ERROR (NetMapIterate (&HttpInstance->TxTokens, > HttpTcpNotReady, NULL))) { > + // > + // Wrap the HTTP token in HTTP_TOKEN_WRAP > + // > + Wrap = AllocateZeroPool (sizeof (HTTP_TOKEN_WRAP)); > + if (Wrap == NULL) { > + Status = EFI_OUT_OF_RESOURCES; > + goto Error1; > + } > + > + Wrap->HttpToken = Token; > + Wrap->HttpInstance = HttpInstance; > + > + Status = HttpCreateTcpTxEvent (Wrap); > + if (EFI_ERROR (Status)) { > + goto Error1; > + } > + > + Status = NetMapInsertTail (&HttpInstance->TxTokens, Token, Wrap); > + if (EFI_ERROR (Status)) { > + goto Error1; > + } > + > + Wrap->TcpWrap.Method = Request->Method; > + > + FreePool (HostName); > + > + // > + // Queue the HTTP token and return. > + // > + return EFI_SUCCESS; > + } else { > + // > + // Use existing TCP instance to transmit the packet. > + // > + Configure = FALSE; > + ReConfigure = FALSE; > + } > } else { > // > - // Use existing TCP instance to transmit the packet. > + // Need close existing TCP instance and create a new TCP > + instance for > data transmit. > // > - Configure = FALSE; > - ReConfigure = FALSE; > - } > - } else { > - // > - // Need close existing TCP instance and create a new TCP instance for > data transmit. > - // > - if (HttpInstance->RemoteHost != NULL) { > - FreePool (HttpInstance->RemoteHost); > - HttpInstance->RemoteHost = NULL; > - HttpInstance->RemotePort = 0; > + if (HttpInstance->RemoteHost != NULL) { > + FreePool (HttpInstance->RemoteHost); > + HttpInstance->RemoteHost = NULL; > + HttpInstance->RemotePort = 0; > + } > } > } > } > @@ -461,7 +495,9 @@ EfiHttpRequest ( > > Wrap->HttpToken = Token; > Wrap->HttpInstance = HttpInstance; > - Wrap->TcpWrap.Method = Request->Method; > + if (Request != NULL) { > + Wrap->TcpWrap.Method = Request->Method; } > > Status = HttpInitTcp (HttpInstance, Wrap, Configure); > if (EFI_ERROR (Status)) { > @@ -482,7 +518,7 @@ EfiHttpRequest ( > // Create request message. > // > FileUrl = Url; > - if (*FileUrl != '/') { > + if (Url != NULL && *FileUrl != '/') { > // > // Convert the absolute-URI to the absolute-path > // > @@ -506,9 +542,17 @@ EfiHttpRequest ( > goto Error3; > } > > - Status = NetMapInsertTail (&HttpInstance->TxTokens, Token, Wrap); > - if (EFI_ERROR (Status)) { > - goto Error4; > + // > + // Every request we insert a TxToken and a response call would > + remove > the TxToken. > + // In cases of PUT/POST, after an initial request-response pair, we > + would do a // continuous request without a response call. So, in > + such cases, where Request // structure is NULL, we would not insert a > TxToken. > + // > + if (Request != NULL) { > + Status = NetMapInsertTail (&HttpInstance->TxTokens, Token, Wrap); > + if (EFI_ERROR (Status)) { > + goto Error4; > + } > } > > // > @@ -533,7 +577,13 @@ EfiHttpRequest ( > return EFI_SUCCESS; > > Error5: > - NetMapRemoveTail (&HttpInstance->TxTokens, NULL); > + // > + // We would have inserted a TxToken only if Request structure is > + not > NULL. > + // Hence check before we do a remove in this error case. > + // > + if (Request != NULL) { > + NetMapRemoveTail (&HttpInstance->TxTokens, NULL); > + } > > Error4: > if (RequestMsg != NULL) { > @@ -970,97 +1020,122 @@ HttpResponseWorker ( > goto Error; > } > > - Tmp = Tmp + AsciiStrLen (HTTP_CRLF_STR); > - SizeofHeaders = SizeofHeaders - (Tmp - HttpHeaders); > - HeaderTmp = AllocateZeroPool (SizeofHeaders); > - if (HeaderTmp == NULL) { > - goto Error; > - } > - > - CopyMem (HeaderTmp, Tmp, SizeofHeaders); > - FreePool (HttpHeaders); > - HttpHeaders = HeaderTmp; > - > // > - // Check whether the EFI_HTTP_UTILITIES_PROTOCOL is available. > + // We could have response with just a HTTP message and no > + headers. For > Example, > + // "100 Continue". In such cases, we would not want to > + unnecessarily call > a Parse > + // method. A "\r\n" following Tmp string again would indicate an end. > Compare and > + // set SizeofHeaders to 0. > // > - if (mHttpUtilities == NULL) { > - Status = EFI_NOT_READY; > - goto Error; > - } > - > - // > - // Parse the HTTP header into array of key/value pairs. > - // > - Status = mHttpUtilities->Parse ( > - mHttpUtilities, > - HttpHeaders, > - SizeofHeaders, > - &HttpMsg->Headers, > - &HttpMsg->HeaderCount > - ); > - if (EFI_ERROR (Status)) { > - goto Error; > + Tmp = Tmp + AsciiStrLen (HTTP_CRLF_STR); > + if (CompareMem (Tmp, HTTP_CRLF_STR, AsciiStrLen (HTTP_CRLF_STR)) > == 0) { > + SizeofHeaders = 0; > + } else { > + SizeofHeaders = SizeofHeaders - (Tmp - HttpHeaders); > } > > - FreePool (HttpHeaders); > - HttpHeaders = NULL; > - > HttpMsg->Data.Response->StatusCode = HttpMappingToStatusCode > (StatusCode); > HttpInstance->StatusCode = StatusCode; > - // > - // Init message-body parser by header information. > - // > + > Status = EFI_NOT_READY; > ValueInItem = NULL; > - NetMapRemoveHead (&HttpInstance->TxTokens, (VOID**) > &ValueInItem); > - if (ValueInItem == NULL) { > - goto Error; > - } > > // > - // The first Tx Token not transmitted yet, insert back and return error. > + // In cases of PUT/POST, after an initial request-response pair, > + we would > do a > + // continuous request without a response call. So, we would not > + do an > insert of > + // TxToken. After we have sent the complete file, we will call a > + response > to get > + // a final response from server. In such a case, we would not > + have any > TxTokens. > + // Hence, check that case before doing a NetMapRemoveHead. > // > - if (!ValueInItem->TcpWrap.IsTxDone) { > - goto Error2; > - } > - > - Status = HttpInitMsgParser ( > - ValueInItem->TcpWrap.Method, > - HttpMsg->Data.Response->StatusCode, > - HttpMsg->HeaderCount, > - HttpMsg->Headers, > - HttpBodyParserCallback, > - (VOID *) ValueInItem, > - &HttpInstance->MsgParser > - ); > - if (EFI_ERROR (Status)) { > - goto Error2; > + if (!NetMapIsEmpty (&HttpInstance->TxTokens)) { > + NetMapRemoveHead (&HttpInstance->TxTokens, (VOID**) > &ValueInItem); > + if (ValueInItem == NULL) { > + goto Error; > + } > + > + // > + // The first Tx Token not transmitted yet, insert back and return > error. > + // > + if (!ValueInItem->TcpWrap.IsTxDone) { > + goto Error2; > + } > } > > - // > - // Check whether we received a complete HTTP message. > - // > - if (HttpInstance->CacheBody != NULL) { > - Status = HttpParseMessageBody (HttpInstance->MsgParser, > HttpInstance->CacheLen, HttpInstance->CacheBody); > + if (SizeofHeaders != 0) { > + HeaderTmp = AllocateZeroPool (SizeofHeaders); > + if (HeaderTmp == NULL) { > + goto Error; > + } > + > + CopyMem (HeaderTmp, Tmp, SizeofHeaders); > + FreePool (HttpHeaders); > + HttpHeaders = HeaderTmp; > + > + // > + // Check whether the EFI_HTTP_UTILITIES_PROTOCOL is available. > + // > + if (mHttpUtilities == NULL) { > + Status = EFI_NOT_READY; > + goto Error; > + } > + > + // > + // Parse the HTTP header into array of key/value pairs. > + // > + Status = mHttpUtilities->Parse ( > + mHttpUtilities, > + HttpHeaders, > + SizeofHeaders, > + &HttpMsg->Headers, > + &HttpMsg->HeaderCount > + ); > + if (EFI_ERROR (Status)) { > + goto Error; > + } > + > + FreePool (HttpHeaders); > + HttpHeaders = NULL; > + > + > + // > + // Init message-body parser by header information. > + // > + Status = HttpInitMsgParser ( > + HttpInstance->Method, > + HttpMsg->Data.Response->StatusCode, > + HttpMsg->HeaderCount, > + HttpMsg->Headers, > + HttpBodyParserCallback, > + (VOID *) ValueInItem, > + &HttpInstance->MsgParser > + ); > if (EFI_ERROR (Status)) { > goto Error2; > } > > - if (HttpIsMessageComplete (HttpInstance->MsgParser)) { > - // > - // Free the MsgParse since we already have a full HTTP message. > - // > - HttpFreeMsgParser (HttpInstance->MsgParser); > - HttpInstance->MsgParser = NULL; > + // > + // Check whether we received a complete HTTP message. > + // > + if (HttpInstance->CacheBody != NULL) { > + Status = HttpParseMessageBody (HttpInstance->MsgParser, > HttpInstance->CacheLen, HttpInstance->CacheBody); > + if (EFI_ERROR (Status)) { > + goto Error2; > + } > + > + if (HttpIsMessageComplete (HttpInstance->MsgParser)) { > + // > + // Free the MsgParse since we already have a full HTTP message. > + // > + HttpFreeMsgParser (HttpInstance->MsgParser); > + HttpInstance->MsgParser = NULL; > + } > } > } > > - if ((HttpMsg->Body == NULL) || (HttpMsg->BodyLength == 0)) { > + if ((HttpMsg->Body == NULL) || (HttpMsg->BodyLength == 0)) { > Status = EFI_SUCCESS; > goto Exit; > } > - } > + } > > // > // Receive the response body. > diff --git a/NetworkPkg/HttpDxe/HttpProto.h > b/NetworkPkg/HttpDxe/HttpProto.h index 8b47fe0..571e669 100644 > --- a/NetworkPkg/HttpDxe/HttpProto.h > +++ b/NetworkPkg/HttpDxe/HttpProto.h > @@ -91,6 +91,7 @@ typedef struct _HTTP_PROTOCOL { > LIST_ENTRY Link; // Link to all HTTP instance from > the service. > BOOLEAN InDestroy; > INTN State; > + EFI_HTTP_METHOD Method; > > UINTN StatusCode; > > -- > 2.6.2.windows.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
