Http Fixes (Take Two)

[edk2-devel] [PATCH v3 0/5] Http Fixes (Take Two)

Posted by Oliver Steffen 3 years, 10 months ago

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2720

This set of patches fixes booting from HTTP/1.0 servers.
It also improves the interaction with HTTP/1.1 servers by recognizing
the 'Connection: close' header field, which fixes a problem with
servers that close the connection after a 404-error is encountered.

It also prevents triggering the TCP issue described in
https://bugzilla.tianocore.org/show_bug.cgi?id=3735
when booting from HTTP/1.0 servers.

PR: https://github.com/tianocore/edk2/pull/2564

Oliver Steffen (5):
  NetworkPkg/HttpDxe: Decofigure Tcp4 before reconfiguring
  NetworkPkg/HttpDxe: Decofigure Tcp6 before reconfiguring
  NetworkPkg/HttpDxe: Add ConnectionClose flag fo HTTP_PROTOCOL
  NetworkPkg/HttpDxe: Detect 'Connection: close' header
  NetworkPkg/HttpDxe: Detect HTTP/1.0 servers

 NetworkPkg/HttpDxe/HttpProto.h |  2 ++
 NetworkPkg/HttpDxe/HttpImpl.c  | 25 ++++++++++++++++++++++++-
 NetworkPkg/HttpDxe/HttpProto.c | 24 ++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 1 deletion(-)

-- 
2.35.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#87833): https://edk2.groups.io/g/devel/message/87833
Mute This Topic: https://groups.io/mt/89951844/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

回复: [edk2-devel] [PATCH v3 0/5] Http Fixes (Take Two)

Posted by gaoliming 3 years, 10 months ago

Maciej, Jiaxin and Siyuan:
  Can you help review this fix?

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Oliver Steffen
> 发送时间: 2022年3月22日 21:30
> 收件人: devel@edk2.groups.io
> 抄送: maciej.rabeda@linux.intel.com; jiaxin.wu@intel.com;
> siyuan.fu@intel.com; kraxel@redhat.com; Oliver Steffen
> <osteffen@redhat.com>
> 主题: [edk2-devel] [PATCH v3 0/5] Http Fixes (Take Two)
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2720
> 
> This set of patches fixes booting from HTTP/1.0 servers.
> It also improves the interaction with HTTP/1.1 servers by recognizing
> the 'Connection: close' header field, which fixes a problem with
> servers that close the connection after a 404-error is encountered.
> 
> It also prevents triggering the TCP issue described in
> https://bugzilla.tianocore.org/show_bug.cgi?id=3735
> when booting from HTTP/1.0 servers.
> 
> PR: https://github.com/tianocore/edk2/pull/2564
> 
> Oliver Steffen (5):
>   NetworkPkg/HttpDxe: Decofigure Tcp4 before reconfiguring
>   NetworkPkg/HttpDxe: Decofigure Tcp6 before reconfiguring
>   NetworkPkg/HttpDxe: Add ConnectionClose flag fo HTTP_PROTOCOL
>   NetworkPkg/HttpDxe: Detect 'Connection: close' header
>   NetworkPkg/HttpDxe: Detect HTTP/1.0 servers
> 
>  NetworkPkg/HttpDxe/HttpProto.h |  2 ++
>  NetworkPkg/HttpDxe/HttpImpl.c  | 25 ++++++++++++++++++++++++-
>  NetworkPkg/HttpDxe/HttpProto.c | 24 ++++++++++++++++++++++++
>  3 files changed, 50 insertions(+), 1 deletion(-)
> 
> --
> 2.35.1
> 
> 
> 
> 
> 





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#87889): https://edk2.groups.io/g/devel/message/87889
Mute This Topic: https://groups.io/mt/89966934/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: 回复: [edk2-devel] [PATCH v3 0/5] Http Fixes (Take Two)

Posted by Gerd Hoffmann 3 years, 10 months ago

On Wed, Mar 23, 2022 at 09:19:09AM +0800, gaoliming wrote:
> Maciej, Jiaxin and Siyuan:
>   Can you help review this fix?

Ping.  Anything blocking the merge of these bugfixes?

thanks,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88516): https://edk2.groups.io/g/devel/message/88516
Mute This Topic: https://groups.io/mt/89966934/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: 回复: [edk2-devel] [PATCH v3 0/5] Http Fixes (Take Two)

Posted by Maciej Rabeda 3 years, 10 months ago

Hi Gerd,

Sorry, I can spare very little time for reviews these days...

I am alright with the patch with two cosmetic changes.

1. In HttpResponseWorker():

     if (AsciiStrnCmp (HttpHeaders, "HTTP/1.0", AsciiStrLen 
("HTTP/1.0")) == 0) {
       DEBUG ((DEBUG_VERBOSE, "HTTP: Server version is 1.0. Setting 
Connection close.\n"));
       HttpInstance->ConnectionClose = TRUE;
     }

I'd change AsciiStrLen ("HTTP/1.0") to sizeof("HTTP/1.0") - 1. No need 
to call a AsciiStrLen every time this flow is executed, it is easily a 
compile-time thing.

2. In HttpResponseWorker(), index -> Index, coding standard.

I can merge this patch with changes above one I get an ACK from you.

Thanks,
Maciej

On 7 kwi 2022 11:57, Gerd Hoffmann wrote:
> On Wed, Mar 23, 2022 at 09:19:09AM +0800, gaoliming wrote:
>> Maciej, Jiaxin and Siyuan:
>>    Can you help review this fix?
> Ping.  Anything blocking the merge of these bugfixes?
>
> thanks,
>    Gerd
>

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88526): https://edk2.groups.io/g/devel/message/88526
Mute This Topic: https://groups.io/mt/89966934/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: 回复: [edk2-devel] [PATCH v3 0/5] Http Fixes (Take Two)

Posted by Oliver Steffen 3 years, 10 months ago

On Thu, Apr 7, 2022 at 2:46 PM Rabeda, Maciej
<maciej.rabeda@linux.intel.com> wrote:

> 1. In HttpResponseWorker():
>
>      if (AsciiStrnCmp (HttpHeaders, "HTTP/1.0", AsciiStrLen
> ("HTTP/1.0")) == 0) {
>        DEBUG ((DEBUG_VERBOSE, "HTTP: Server version is 1.0. Setting
> Connection close.\n"));
>        HttpInstance->ConnectionClose = TRUE;
>      }
>
> I'd change AsciiStrLen ("HTTP/1.0") to sizeof("HTTP/1.0") - 1. No need
> to call a AsciiStrLen every time this flow is executed, it is easily a
> compile-time thing.

Yes, of course.

> 2. In HttpResponseWorker(), index -> Index, coding standard.

Sorry, I missed that one.

> I can merge this patch with changes above one I get an ACK from you.

Sounds good to me. Thank you!

-- Oliver



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88528): https://edk2.groups.io/g/devel/message/88528
Mute This Topic: https://groups.io/mt/89966934/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: 回复: [edk2-devel] [PATCH v3 0/5] Http Fixes (Take Two)

Posted by Maciej Rabeda 3 years, 10 months ago

Alright.

Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>

Merge soon.

On 7 kwi 2022 14:55, Oliver Steffen wrote:
> On Thu, Apr 7, 2022 at 2:46 PM Rabeda, Maciej
> <maciej.rabeda@linux.intel.com> wrote:
>
>> 1. In HttpResponseWorker():
>>
>>       if (AsciiStrnCmp (HttpHeaders, "HTTP/1.0", AsciiStrLen
>> ("HTTP/1.0")) == 0) {
>>         DEBUG ((DEBUG_VERBOSE, "HTTP: Server version is 1.0. Setting
>> Connection close.\n"));
>>         HttpInstance->ConnectionClose = TRUE;
>>       }
>>
>> I'd change AsciiStrLen ("HTTP/1.0") to sizeof("HTTP/1.0") - 1. No need
>> to call a AsciiStrLen every time this flow is executed, it is easily a
>> compile-time thing.
> Yes, of course.
>
>> 2. In HttpResponseWorker(), index -> Index, coding standard.
> Sorry, I missed that one.
>
>> I can merge this patch with changes above one I get an ACK from you.
> Sounds good to me. Thank you!
>
> -- Oliver
>
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88612): https://edk2.groups.io/g/devel/message/88612
Mute This Topic: https://groups.io/mt/89966934/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: 回复: [edk2-devel] [PATCH v3 0/5] Http Fixes (Take Two)

Posted by Maciej Rabeda 3 years, 10 months ago

Patchset merged: https://github.com/tianocore/edk2/pull/2756

On 8 kwi 2022 11:07, Rabeda, Maciej wrote:
> Alright.
>
> Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
>
> Merge soon.
>
> On 7 kwi 2022 14:55, Oliver Steffen wrote:
>> On Thu, Apr 7, 2022 at 2:46 PM Rabeda, Maciej
>> <maciej.rabeda@linux.intel.com> wrote:
>>
>>> 1. In HttpResponseWorker():
>>>
>>>       if (AsciiStrnCmp (HttpHeaders, "HTTP/1.0", AsciiStrLen
>>> ("HTTP/1.0")) == 0) {
>>>         DEBUG ((DEBUG_VERBOSE, "HTTP: Server version is 1.0. Setting
>>> Connection close.\n"));
>>>         HttpInstance->ConnectionClose = TRUE;
>>>       }
>>>
>>> I'd change AsciiStrLen ("HTTP/1.0") to sizeof("HTTP/1.0") - 1. No need
>>> to call a AsciiStrLen every time this flow is executed, it is easily a
>>> compile-time thing.
>> Yes, of course.
>>
>>> 2. In HttpResponseWorker(), index -> Index, coding standard.
>> Sorry, I missed that one.
>>
>>> I can merge this patch with changes above one I get an ACK from you.
>> Sounds good to me. Thank you!
>>
>> -- Oliver
>>
>>
>>
>> 
>>
>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88707): https://edk2.groups.io/g/devel/message/88707
Mute This Topic: https://groups.io/mt/89966934/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-