Re: [edk2-devel] [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand
> -Original Message- > From: Rabeda, Maciej > Sent: Wednesday, August 19, 2020 10:44 AM > To: Laszlo Ersek ; Vladimir Olovyannikov > ; Gao, Zhichao > ; devel@edk2.groups.io > Cc: Samer El-Haj-Mahmoud ; Wu, Jiaxin > ; Fu, Siyuan ; Ni, Ray > ; Gao, Liming ; Nd > > Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add > HttpDynamicCommand > > @Laszlo, > > As for type casting, I don't have strong opinions. > Space after typecast (for me) always seemed to be visually more consistent > with the rest of the code. > In my regular projects written in C-based languages, I would do it the way > you are describing. > > @Vladimir > I went through the rest of the code. > There are more coding standard problems, but in terms of HTTP usage the > code seems to be OK. OK, great, thanks. I am reviewing it and will submit the patch shortly. Thanks to Laszlo, I hope to be able to run CI locally and detect hidden issues. Indeed, I have to switch between Linux and UEFI code, so I missed several code style issues. > > Thanks, > Maciej > > PS. Rabeda is my last name :) Oops, thanks for letting me know, Maciej. It is just a bit unusual to have the last name first :) Thank you, Vladimir > > On 19-Aug-20 11:47, Laszlo Ersek wrote: > > On 08/18/20 20:33, Vladimir Olovyannikov wrote: > >>> -Original Message- > >>> From: Rabeda, Maciej > >>> Sent: Tuesday, August 18, 2020 9:54 AM > >>> To: Vladimir Olovyannikov ; > >>> Laszlo Ersek ; Gao, Zhichao > >>> ; devel@edk2.groups.io > >>> Cc: Samer El-Haj-Mahmoud ; Wu, > Jiaxin > >>> ; Fu, Siyuan ; Ni, Ray > >>> ; Gao, Liming ; Nd > >>> > >>> Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add > >>> HttpDynamicCommand > >>> > >>> Hi Vladimir, > >>> > >>> I am inprog of going through the patch. There are some coding > >>> standard violations. > >>> For reference: > >>> https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification > >>> / > > [...] > > > >> OK, I will look through all files again, probably something slipped > >> from my attention as I work with Linux code as well. > > Now that we have ECC checking enabled in CI, submitting a personal > > pull request could help. Additionally, the CI workload should be > > possible to run locally, according to ".pytool/Readme.md". > > > > (I'm planning to learn how to run that locally myself.) > > > >>> HttpApp.c: > >>> Line 48: Space after type cast. > >> Sorry, I don't understand here. You mean, Something = > >> (CAST)SomethingElse should actually be "Something = (CAST) > SomethingElse? > >> I don't see this say in the Tftp.c: > >> if (AsciiStrnCmp ((CHAR8 *)Option->OptionStr, "tsize", 5) == 0) ... > >> or in TftpApp.c > >> Status = (EFI_STATUS)RunTftp (ImageHandle, SystemTable); Am I missing > >> anything? > > > > In core edk2 packages, casts are frequently written like this: > > > >(TYPE) Expression > > > > (Importantly, Expression itself is not parenthesized.) > > > > In my opinion, this is a bad practice. That's because the typecast has > > one of the strongest operator bindings in the C language, and visually > > distancing (TYPE) from "Expression" suggests the opposite -- it's > > counter-intuitive. > > > > I strongly prefer > > > >(TYPE)Expression > > > > but other maintainers may have a different preference. > > > > Thanks > > Laszlo > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64459): https://edk2.groups.io/g/devel/message/64459 Mute This Topic: https://groups.io/mt/75826968/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand
@Laszlo, As for type casting, I don't have strong opinions. Space after typecast (for me) always seemed to be visually more consistent with the rest of the code. In my regular projects written in C-based languages, I would do it the way you are describing. @Vladimir I went through the rest of the code. There are more coding standard problems, but in terms of HTTP usage the code seems to be OK. Thanks, Maciej PS. Rabeda is my last name :) On 19-Aug-20 11:47, Laszlo Ersek wrote: On 08/18/20 20:33, Vladimir Olovyannikov wrote: -Original Message- From: Rabeda, Maciej Sent: Tuesday, August 18, 2020 9:54 AM To: Vladimir Olovyannikov ; Laszlo Ersek ; Gao, Zhichao ; devel@edk2.groups.io Cc: Samer El-Haj-Mahmoud ; Wu, Jiaxin ; Fu, Siyuan ; Ni, Ray ; Gao, Liming ; Nd Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand Hi Vladimir, I am inprog of going through the patch. There are some coding standard violations. For reference: https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/ [...] OK, I will look through all files again, probably something slipped from my attention as I work with Linux code as well. Now that we have ECC checking enabled in CI, submitting a personal pull request could help. Additionally, the CI workload should be possible to run locally, according to ".pytool/Readme.md". (I'm planning to learn how to run that locally myself.) HttpApp.c: Line 48: Space after type cast. Sorry, I don't understand here. You mean, Something = (CAST)SomethingElse should actually be "Something = (CAST) SomethingElse? I don't see this say in the Tftp.c: if (AsciiStrnCmp ((CHAR8 *)Option->OptionStr, "tsize", 5) == 0) ... or in TftpApp.c Status = (EFI_STATUS)RunTftp (ImageHandle, SystemTable); Am I missing anything? In core edk2 packages, casts are frequently written like this: (TYPE) Expression (Importantly, Expression itself is not parenthesized.) In my opinion, this is a bad practice. That's because the typecast has one of the strongest operator bindings in the C language, and visually distancing (TYPE) from "Expression" suggests the opposite -- it's counter-intuitive. I strongly prefer (TYPE)Expression but other maintainers may have a different preference. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64456): https://edk2.groups.io/g/devel/message/64456 Mute This Topic: https://groups.io/mt/75826968/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand
On 08/18/20 20:33, Vladimir Olovyannikov wrote: >> -Original Message- >> From: Rabeda, Maciej >> Sent: Tuesday, August 18, 2020 9:54 AM >> To: Vladimir Olovyannikov ; Laszlo >> Ersek ; Gao, Zhichao ; >> devel@edk2.groups.io >> Cc: Samer El-Haj-Mahmoud ; Wu, Jiaxin >> ; Fu, Siyuan ; Ni, Ray >> ; Gao, Liming ; Nd >> >> Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add >> HttpDynamicCommand >> >> Hi Vladimir, >> >> I am inprog of going through the patch. There are some coding standard >> violations. >> For reference: >> https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/ [...] > OK, I will look through all files again, probably something slipped from my > attention as I work with Linux code as well. Now that we have ECC checking enabled in CI, submitting a personal pull request could help. Additionally, the CI workload should be possible to run locally, according to ".pytool/Readme.md". (I'm planning to learn how to run that locally myself.) > >> HttpApp.c: >> Line 48: Space after type cast. > Sorry, I don't understand here. You mean, Something = (CAST)SomethingElse > should actually be "Something = (CAST) SomethingElse? > I don't see this say in the Tftp.c: > if (AsciiStrnCmp ((CHAR8 *)Option->OptionStr, "tsize", 5) == 0) ... > or in TftpApp.c > Status = (EFI_STATUS)RunTftp (ImageHandle, SystemTable); > Am I missing anything? In core edk2 packages, casts are frequently written like this: (TYPE) Expression (Importantly, Expression itself is not parenthesized.) In my opinion, this is a bad practice. That's because the typecast has one of the strongest operator bindings in the C language, and visually distancing (TYPE) from "Expression" suggests the opposite -- it's counter-intuitive. I strongly prefer (TYPE)Expression but other maintainers may have a different preference. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64423): https://edk2.groups.io/g/devel/message/64423 Mute This Topic: https://groups.io/mt/75826968/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand
Hi Rabeda,
Thank you for reviewing.
> -Original Message-
> From: Rabeda, Maciej
> Sent: Tuesday, August 18, 2020 9:54 AM
> To: Vladimir Olovyannikov ; Laszlo
> Ersek ; Gao, Zhichao ;
> devel@edk2.groups.io
> Cc: Samer El-Haj-Mahmoud ; Wu, Jiaxin
> ; Fu, Siyuan ; Ni, Ray
> ; Gao, Liming ; Nd
>
> Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add
> HttpDynamicCommand
>
> Hi Vladimir,
>
> I am inprog of going through the patch. There are some coding standard
> violations.
> For reference:
> https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/
>
> Http.c:
> Lines 110-111: Spacing before open bracket and != operator Line 133:
> Unnecessary brackets around EFI_SUCCESS Line 144+: Function argument
> alignment, closed bracket spacing Line 357: End of function call ');' in a
> multiline call should be in the next line, aligned with argument list Line
> 360: In
> a multiline if/while statement, '{' should be moved to a new line Line
> 361,
> 380, 388, ... : ShellPrintHiiEx calls: I suggest defining a macro to
> improve
> readability. First 3 parameters and mHttpHiiHandle seem to be commonly
> used in your calls.
> Line 740: Keep unary operation in a single line Lines 891-901, 946-1028:
> Tab
> alignment Lines 988-995: Assignments to a struct line by line. Could you
> align
> them in the following manner?
> SpecificStruct.Field1=10;
> SpecificStruct.Field2=20;
>
OK, I will look through all files again, probably something slipped from my
attention as I work with Linux code as well.
> HttpApp.c:
> Line 48: Space after type cast.
Sorry, I don't understand here. You mean, Something = (CAST)SomethingElse
should actually be "Something = (CAST) SomethingElse?
I don't see this say in the Tftp.c:
if (AsciiStrnCmp ((CHAR8 *)Option->OptionStr, "tsize", 5) == 0) ...
or in TftpApp.c
Status = (EFI_STATUS)RunTftp (ImageHandle, SystemTable);
Am I missing anything?
>
> List above does not exhaust the things that can be found there.
> May I ask you to read through the code again and catch what you and I
> might
> have missed?
>
> Additionally:
> Http.c, TrimSpaces:
> 1. It takes CHAR16** as an argument, yet it does not seem to modify the
> value of the pointer for other functions outside its scope. I think it
> would be
> better to just make it CHAR16*.
> 2. First while loop is highly inefficient. If I had a string like "
> asdf.txt", it would
> call CopyMem for each space, each time moving the asdf.txt one char closer
> to the beginning.
> It would be way better to achieve the same functionality by iterating from
> front and back of the string looking for a char that is not a space nor a
> tab.
> Having their indices, you can easily call CopyMem once to move the string
> forward and ZeroMem the rest.
>
Definitely, thanks. This TrimSpaces() was copy/paste from some other edk2
code.
I should've looked into it.
> Thanks,
> Maciej
Thank you,
Vladimir
>
> On 18-Aug-20 00:52, Vladimir Olovyannikov wrote:
> >> -Original Message-
> >> From: Laszlo Ersek
> >> Sent: Monday, August 17, 2020 1:44 PM
> >> To: Vladimir Olovyannikov ;
> >> Rabeda, Maciej ; Gao, Zhichao
> >> ; devel@edk2.groups.io
> >> Cc: Samer El-Haj-Mahmoud ; Wu,
> Jiaxin
> >> ; Fu, Siyuan ; Ni, Ray
> >> ; Gao, Liming ; Nd
> >>
> >> Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add
> >> HttpDynamicCommand
> >>
> >> On 08/17/20 20:29, Vladimir Olovyannikov wrote:
> -Original Message-
> From: Laszlo Ersek
> Sent: Monday, August 17, 2020 11:01 AM
> To: Rabeda, Maciej ; Vladimir
> Olovyannikov ; Gao, Zhichao
> ; devel@edk2.groups.io
> Cc: Samer El-Haj-Mahmoud ; Wu,
> >> Jiaxin
> ; Fu, Siyuan ; Ni, Ray
> ; Gao, Liming ; Nd
>
> Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add
> HttpDynamicCommand
>
> On 08/17/20 19:15, Rabeda, Maciej wrote:
> > Hi Vladimir,
> >
> > I cannot apply the patch via 'git am'.
> > Is your git configured in a manner described here?
> > https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unk
> > em pt -git-guide-for-edk2-contributors-and-maintainers
> >>> Hi Rabeda,
> >>> Yes, I followed the page. I did not run the SetupGit.py though.
> >
> > Laszlo,
> >
> > Were you able to apply this patch from .eml file?
> Yes, but I had to use some tricks (implemented by my colleague
> Paolo in a python script) to undo the damage caused by the "quoted-
> printable"
> content-transfer-encoding on the posting.
>
> Our recommended Content-Transfer-Encoding (that is,
> "sendemail.transferEncoding") is "8bit", or even "base64".
> quoted-printable is practically impossible to get functional, with
> the hard CRLF line endings in edk2.
>
> "BaseTools/Scripts/SetupGit.py" does set "8bit".
> >>> Thank you for notice Laszlo,
> >>> So, should I run this script and re-send the patch as v6?
> >> I think that would
Re: [edk2-devel] [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand
Hi Vladimir,
I am inprog of going through the patch. There are some coding standard
violations.
For reference:
https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/
Http.c:
Lines 110-111: Spacing before open bracket and != operator
Line 133: Unnecessary brackets around EFI_SUCCESS
Line 144+: Function argument alignment, closed bracket spacing
Line 357: End of function call ');' in a multiline call should be in the
next line, aligned with argument list
Line 360: In a multiline if/while statement, '{' should be moved to a
new line
Line 361, 380, 388, ... : ShellPrintHiiEx calls: I suggest defining a
macro to improve readability. First 3 parameters and mHttpHiiHandle seem
to be commonly used in your calls.
Line 740: Keep unary operation in a single line
Lines 891-901, 946-1028: Tab alignment
Lines 988-995: Assignments to a struct line by line. Could you align
them in the following manner?
SpecificStruct.Field1 = 10;
SpecificStruct.Field2 = 20;
HttpApp.c:
Line 48: Space after type cast.
List above does not exhaust the things that can be found there.
May I ask you to read through the code again and catch what you and I
might have missed?
Additionally:
Http.c, TrimSpaces:
1. It takes CHAR16** as an argument, yet it does not seem to modify the
value of the pointer for other functions outside its scope. I think it
would be better to just make it CHAR16*.
2. First while loop is highly inefficient. If I had a string like "
asdf.txt", it would call CopyMem for each space, each time moving the
asdf.txt one char closer to the beginning.
It would be way better to achieve the same functionality by iterating
from front and back of the string looking for a char that is not a space
nor a tab.
Having their indices, you can easily call CopyMem once to move the
string forward and ZeroMem the rest.
Thanks,
Maciej
On 18-Aug-20 00:52, Vladimir Olovyannikov wrote:
-Original Message-
From: Laszlo Ersek
Sent: Monday, August 17, 2020 1:44 PM
To: Vladimir Olovyannikov ;
Rabeda, Maciej ; Gao, Zhichao
; devel@edk2.groups.io
Cc: Samer El-Haj-Mahmoud ; Wu, Jiaxin
; Fu, Siyuan ; Ni, Ray
; Gao, Liming ; Nd
Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add
HttpDynamicCommand
On 08/17/20 20:29, Vladimir Olovyannikov wrote:
-Original Message-
From: Laszlo Ersek
Sent: Monday, August 17, 2020 11:01 AM
To: Rabeda, Maciej ; Vladimir
Olovyannikov ; Gao, Zhichao
; devel@edk2.groups.io
Cc: Samer El-Haj-Mahmoud ; Wu,
Jiaxin
; Fu, Siyuan ; Ni, Ray
; Gao, Liming ; Nd
Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add
HttpDynamicCommand
On 08/17/20 19:15, Rabeda, Maciej wrote:
Hi Vladimir,
I cannot apply the patch via 'git am'.
Is your git configured in a manner described here?
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkem
pt -git-guide-for-edk2-contributors-and-maintainers
Hi Rabeda,
Yes, I followed the page. I did not run the SetupGit.py though.
Laszlo,
Were you able to apply this patch from .eml file?
Yes, but I had to use some tricks (implemented by my colleague Paolo
in a python script) to undo the damage caused by the "quoted-printable"
content-transfer-encoding on the posting.
Our recommended Content-Transfer-Encoding (that is,
"sendemail.transferEncoding") is "8bit", or even "base64".
quoted-printable is practically impossible to get functional, with
the hard CRLF line endings in edk2.
"BaseTools/Scripts/SetupGit.py" does set "8bit".
Thank you for notice Laszlo,
So, should I run this script and re-send the patch as v6?
I think that would be useful, yes -- and if you have made no changes since
v5,
you can also post it as "v5 RESEND". If you've implemented some changes
meanwhile, then please post it as v6 indeed.
Thanks Laszlo,
I will post v6.
Vladimir
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#64393): https://edk2.groups.io/g/devel/message/64393
Mute This Topic: https://groups.io/mt/75826968/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand
> -Original Message- > From: Laszlo Ersek > Sent: Monday, August 17, 2020 1:44 PM > To: Vladimir Olovyannikov ; > Rabeda, Maciej ; Gao, Zhichao > ; devel@edk2.groups.io > Cc: Samer El-Haj-Mahmoud ; Wu, Jiaxin > ; Fu, Siyuan ; Ni, Ray > ; Gao, Liming ; Nd > > Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add > HttpDynamicCommand > > On 08/17/20 20:29, Vladimir Olovyannikov wrote: > >> -Original Message- > >> From: Laszlo Ersek > >> Sent: Monday, August 17, 2020 11:01 AM > >> To: Rabeda, Maciej ; Vladimir > >> Olovyannikov ; Gao, Zhichao > >> ; devel@edk2.groups.io > >> Cc: Samer El-Haj-Mahmoud ; Wu, > Jiaxin > >> ; Fu, Siyuan ; Ni, Ray > >> ; Gao, Liming ; Nd > >> > >> Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add > >> HttpDynamicCommand > >> > >> On 08/17/20 19:15, Rabeda, Maciej wrote: > >>> Hi Vladimir, > >>> > >>> I cannot apply the patch via 'git am'. > >>> Is your git configured in a manner described here? > >>> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkem > >>> pt -git-guide-for-edk2-contributors-and-maintainers > > Hi Rabeda, > > Yes, I followed the page. I did not run the SetupGit.py though. > >>> > >>> > >>> Laszlo, > >>> > >>> Were you able to apply this patch from .eml file? > >> > >> Yes, but I had to use some tricks (implemented by my colleague Paolo > >> in a python script) to undo the damage caused by the "quoted-printable" > >> content-transfer-encoding on the posting. > >> > >> Our recommended Content-Transfer-Encoding (that is, > >> "sendemail.transferEncoding") is "8bit", or even "base64". > >> quoted-printable is practically impossible to get functional, with > >> the hard CRLF line endings in edk2. > >> > >> "BaseTools/Scripts/SetupGit.py" does set "8bit". > > Thank you for notice Laszlo, > > So, should I run this script and re-send the patch as v6? > > I think that would be useful, yes -- and if you have made no changes since > v5, > you can also post it as "v5 RESEND". If you've implemented some changes > meanwhile, then please post it as v6 indeed. Thanks Laszlo, I will post v6. Vladimir > > Thanks > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64337): https://edk2.groups.io/g/devel/message/64337 Mute This Topic: https://groups.io/mt/75826968/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand
On 08/17/20 20:29, Vladimir Olovyannikov wrote: >> -Original Message- >> From: Laszlo Ersek >> Sent: Monday, August 17, 2020 11:01 AM >> To: Rabeda, Maciej ; Vladimir Olovyannikov >> ; Gao, Zhichao >> ; devel@edk2.groups.io >> Cc: Samer El-Haj-Mahmoud ; Wu, Jiaxin >> ; Fu, Siyuan ; Ni, Ray >> ; Gao, Liming ; Nd >> >> Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add >> HttpDynamicCommand >> >> On 08/17/20 19:15, Rabeda, Maciej wrote: >>> Hi Vladimir, >>> >>> I cannot apply the patch via 'git am'. >>> Is your git configured in a manner described here? >>> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt >>> -git-guide-for-edk2-contributors-and-maintainers > Hi Rabeda, > Yes, I followed the page. I did not run the SetupGit.py though. >>> >>> >>> Laszlo, >>> >>> Were you able to apply this patch from .eml file? >> >> Yes, but I had to use some tricks (implemented by my colleague Paolo in a >> python script) to undo the damage caused by the "quoted-printable" >> content-transfer-encoding on the posting. >> >> Our recommended Content-Transfer-Encoding (that is, >> "sendemail.transferEncoding") is "8bit", or even "base64". >> quoted-printable is practically impossible to get functional, with the >> hard >> CRLF line endings in edk2. >> >> "BaseTools/Scripts/SetupGit.py" does set "8bit". > Thank you for notice Laszlo, > So, should I run this script and re-send the patch as v6? I think that would be useful, yes -- and if you have made no changes since v5, you can also post it as "v5 RESEND". If you've implemented some changes meanwhile, then please post it as v6 indeed. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64333): https://edk2.groups.io/g/devel/message/64333 Mute This Topic: https://groups.io/mt/75826968/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand
> -Original Message- > From: Laszlo Ersek > Sent: Monday, August 17, 2020 11:01 AM > To: Rabeda, Maciej ; Vladimir Olovyannikov > ; Gao, Zhichao > ; devel@edk2.groups.io > Cc: Samer El-Haj-Mahmoud ; Wu, Jiaxin > ; Fu, Siyuan ; Ni, Ray > ; Gao, Liming ; Nd > > Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add > HttpDynamicCommand > > On 08/17/20 19:15, Rabeda, Maciej wrote: > > Hi Vladimir, > > > > I cannot apply the patch via 'git am'. > > Is your git configured in a manner described here? > > https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt > > -git-guide-for-edk2-contributors-and-maintainers Hi Rabeda, Yes, I followed the page. I did not run the SetupGit.py though. > > > > > > Laszlo, > > > > Were you able to apply this patch from .eml file? > > Yes, but I had to use some tricks (implemented by my colleague Paolo in a > python script) to undo the damage caused by the "quoted-printable" > content-transfer-encoding on the posting. > > Our recommended Content-Transfer-Encoding (that is, > "sendemail.transferEncoding") is "8bit", or even "base64". > quoted-printable is practically impossible to get functional, with the > hard > CRLF line endings in edk2. > > "BaseTools/Scripts/SetupGit.py" does set "8bit". Thank you for notice Laszlo, So, should I run this script and re-send the patch as v6? Thank you, Vladimir > > Thanks > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64331): https://edk2.groups.io/g/devel/message/64331 Mute This Topic: https://groups.io/mt/75826968/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand
On 08/17/20 19:15, Rabeda, Maciej wrote: > Hi Vladimir, > > I cannot apply the patch via 'git am'. > Is your git configured in a manner described here? > https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers > > > Laszlo, > > Were you able to apply this patch from .eml file? Yes, but I had to use some tricks (implemented by my colleague Paolo in a python script) to undo the damage caused by the "quoted-printable" content-transfer-encoding on the posting. Our recommended Content-Transfer-Encoding (that is, "sendemail.transferEncoding") is "8bit", or even "base64". quoted-printable is practically impossible to get functional, with the hard CRLF line endings in edk2. "BaseTools/Scripts/SetupGit.py" does set "8bit". Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64328): https://edk2.groups.io/g/devel/message/64328 Mute This Topic: https://groups.io/mt/75826968/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand
Hi Vladimir, I cannot apply the patch via 'git am'. Is your git configured in a manner described here? https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers Laszlo, Were you able to apply this patch from .eml file? Thanks, Maciej On 17-Aug-20 17:46, Vladimir Olovyannikov wrote: Hi Gao, Thanks for reviewing. As you suggest, I will postpone the next patchset submission until NetworkPkg maintainer(s) review. -Original Message- From: Gao, Zhichao Sent: Sunday, August 16, 2020 6:48 PM To: Vladimir Olovyannikov ; devel@edk2.groups.io Cc: Laszlo Ersek ; Samer El-Haj-Mahmoud ; Maciej Rabeda ; Wu, Jiaxin ; Fu, Siyuan ; Ni, Ray ; Gao, Liming ; Nd Subject: RE: [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand Sorry for the delay reply. See below: -Original Message- From: Vladimir Olovyannikov Sent: Tuesday, July 28, 2020 12:49 AM To: devel@edk2.groups.io Cc: Laszlo Ersek ; Vladimir Olovyannikov ; Samer El-Haj-Mahmoud haj-mahm...@arm.com>; Gao, Zhichao ; Maciej Rabeda ; Wu, Jiaxin ; Fu, Siyuan ; Ni, Ray ; Gao, Liming ; Nd Subject: [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand Introduce an http client utilizing EDK2 HTTP protocol, to allow fast image downloading from http/https servers. HTTP download speed is usually faster than tftp. The client is based on the same approach as tftp dynamic command, and uses the same UEFI Shell command line parameters. This makes it easy integrating http into existing UEFI Shell scripts. Note that to enable HTTP download, feature Pcd gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections must be set to TRUE. BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2860 Signed-off-by: Vladimir Olovyannikov CC: Samer El-Haj-Mahmoud CC: Laszlo Ersek Cc: Zhichao Gao Cc: Maciej Rabeda Cc: Jiaxin Wu Cc: Siyuan Fu Cc: Ray Ni Cc: Liming Gao Cc: Nd --- CryptoPkg/Library/OpensslLib/openssl |2 +- .../DynamicCommand/HttpDynamicCommand/Http.c | 1715 + .../DynamicCommand/HttpDynamicCommand/Http.h | 89 + .../HttpDynamicCommand/Http.uni | 117 ++ .../HttpDynamicCommand/HttpApp.c | 53 + .../HttpDynamicCommand/HttpApp.inf| 58 + .../HttpDynamicCommand/HttpDynamicCommand.c | 134 ++ .../HttpDynamicCommand/HttpDynamicCommand.inf | 63 + ShellPkg/Include/Guid/ShellLibHiiGuid.h |5 + ShellPkg/ShellPkg.dec |1 + ShellPkg/ShellPkg.dsc |5 + 11 files changed, 2241 insertions(+), 1 deletion(-) create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.h create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.uni create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.c create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.inf create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand .c create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand .inf diff --git a/CryptoPkg/Library/OpensslLib/openssl b/CryptoPkg/Library/OpensslLib/openssl index e2e09d9fba11..c3656cc594da 16 --- a/CryptoPkg/Library/OpensslLib/openssl +++ b/CryptoPkg/Library/OpensslLib/openssl @@ -1 +1 @@ -Subproject commit e2e09d9fba1187f8d6aafaa34d4172f56f1ffb72 +Subproject commit c3656cc594daac8167721dde7220f0e59ae146fc diff --git a/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c b/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c new file mode 100644 index ..0565b07c3570 --- /dev/null +++ b/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c @@ -0,0 +1,1715 @@ +/** @file + The implementation for the 'http' Shell command. + + Copyright (c) 2015, ARM Ltd. All rights reserved. + Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved. + (C) Copyright 2015 Hewlett Packard Enterprise Development LP + Copyright (c) 2020, Broadcom. All rights reserved. + + SPDX-License-Identifier: BSD-2-Clause-Patent +**/ + +#include "Http.h" + +#define IP4_CONFIG2_INTERFACE_INFO_NAME_LENGTH 32 +EFI_HII_HANDLE mHttpHiiHandle; + +/* + Constant strings and definitions related to the message + indicating the amount of progress in the dowloading of a HTTP file. +*/ + +// Number of steps in the progression slider +#define HTTP_PROGRESS_SLIDER_STEPS \ + ((sizeof (HTTP_PROGR_FRAME) / sizeof (CHAR16)) - 3) + +// Size in number of characters plus one (final zero) of the message to +// indicate the progress of an HTTP download. The format is "[(progress slider: +// 40 characters)] (nb of KBytes downloaded so far: 7 characters) Kb". There +// are thus the number of characters in HTTP_PROGR_FRAME[] plus 11 characters +// (2 // spaces, "Kb" and seven characters for the number of KBytes). +#define HTTP_PROGRESS_MESSAGE_SIZE
Re: [edk2-devel] [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand
Hi Gao, Thanks for reviewing. As you suggest, I will postpone the next patchset submission until NetworkPkg maintainer(s) review. > -Original Message- > From: Gao, Zhichao > Sent: Sunday, August 16, 2020 6:48 PM > To: Vladimir Olovyannikov ; > devel@edk2.groups.io > Cc: Laszlo Ersek ; Samer El-Haj-Mahmoud haj-mahm...@arm.com>; Maciej Rabeda > ; Wu, Jiaxin ; Fu, > Siyuan ; Ni, Ray ; Gao, Liming > ; Nd > Subject: RE: [PATCH v5 1/1] ShellPkg/DynamicCommand: add > HttpDynamicCommand > > Sorry for the delay reply. > > See below: > > > -Original Message- > > From: Vladimir Olovyannikov > > Sent: Tuesday, July 28, 2020 12:49 AM > > To: devel@edk2.groups.io > > Cc: Laszlo Ersek ; Vladimir Olovyannikov > > ; Samer El-Haj-Mahmoud > > haj-mahm...@arm.com>; Gao, Zhichao ; Maciej > > Rabeda ; Wu, Jiaxin > ; Fu, > > Siyuan ; Ni, Ray ; Gao, Liming > > ; Nd > > Subject: [PATCH v5 1/1] ShellPkg/DynamicCommand: add > HttpDynamicCommand > > > > Introduce an http client utilizing EDK2 HTTP protocol, to > > allow fast image downloading from http/https servers. > > HTTP download speed is usually faster than tftp. > > The client is based on the same approach as tftp dynamic command, and > > uses the same UEFI Shell command line parameters. This makes it easy > > integrating http into existing UEFI Shell scripts. > > Note that to enable HTTP download, feature Pcd > > gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections must > > be set to TRUE. > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2860 > > > > Signed-off-by: Vladimir Olovyannikov > > > CC: Samer El-Haj-Mahmoud > > CC: Laszlo Ersek > > Cc: Zhichao Gao > > Cc: Maciej Rabeda > > Cc: Jiaxin Wu > > Cc: Siyuan Fu > > Cc: Ray Ni > > Cc: Liming Gao > > Cc: Nd > > --- > > CryptoPkg/Library/OpensslLib/openssl |2 +- > > .../DynamicCommand/HttpDynamicCommand/Http.c | 1715 > > + > > .../DynamicCommand/HttpDynamicCommand/Http.h | 89 + > > .../HttpDynamicCommand/Http.uni | 117 ++ > > .../HttpDynamicCommand/HttpApp.c | 53 + > > .../HttpDynamicCommand/HttpApp.inf| 58 + > > .../HttpDynamicCommand/HttpDynamicCommand.c | 134 ++ > > .../HttpDynamicCommand/HttpDynamicCommand.inf | 63 + > > ShellPkg/Include/Guid/ShellLibHiiGuid.h |5 + > > ShellPkg/ShellPkg.dec |1 + > > ShellPkg/ShellPkg.dsc |5 + > > 11 files changed, 2241 insertions(+), 1 deletion(-) > > create mode 100644 > ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c > > create mode 100644 > > ShellPkg/DynamicCommand/HttpDynamicCommand/Http.h > > create mode 100644 > > ShellPkg/DynamicCommand/HttpDynamicCommand/Http.uni > > create mode 100644 > > ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.c > > create mode 100644 > > ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.inf > > create mode 100644 > > > ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand > .c > > create mode 100644 > > > ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand > .inf > > > > diff --git a/CryptoPkg/Library/OpensslLib/openssl > > b/CryptoPkg/Library/OpensslLib/openssl > > index e2e09d9fba11..c3656cc594da 16 > > --- a/CryptoPkg/Library/OpensslLib/openssl > > +++ b/CryptoPkg/Library/OpensslLib/openssl > > @@ -1 +1 @@ > > -Subproject commit e2e09d9fba1187f8d6aafaa34d4172f56f1ffb72 > > +Subproject commit c3656cc594daac8167721dde7220f0e59ae146fc > > diff --git a/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c > > b/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c > > new file mode 100644 > > index ..0565b07c3570 > > --- /dev/null > > +++ b/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c > > @@ -0,0 +1,1715 @@ > > +/** @file > > > > + The implementation for the 'http' Shell command. > > > > + > > > > + Copyright (c) 2015, ARM Ltd. All rights reserved. > > > > + Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved. > > > > + (C) Copyright 2015 Hewlett Packard Enterprise Development LP > > > > + Copyright (c) 2020, Broadcom. All rights reserved. > > > > + > > > > + SPDX-License-Identifier: BSD-2-Clause-Patent > > > > +**/ > > > > + > > > > +#include "Http.h" > > > > + > > > > +#define IP4_CONFIG2_INTERFACE_INFO_NAME_LENGTH 32 > > > > +EFI_HII_HANDLE mHttpHiiHandle; > > > > + > > > > +/* > > > > + Constant strings and definitions related to the message > > > > + indicating the amount of progress in the dowloading of a HTTP file. > > > > +*/ > > > > + > > > > +// Number of steps in the progression slider > > > > +#define HTTP_PROGRESS_SLIDER_STEPS \ > > > > + ((sizeof (HTTP_PROGR_FRAME) / sizeof (CHAR16)) - 3) > > > > + > > > > +// Size in number of characters plus one (final zero) of the message to > > > > +// indicate the progress of an HTTP download. The format is "[(progress > slider: > > > > +// 40 characters)] (nb of KBytes downloaded so far: 7 characters) Kb". > There > > > > +// are
Re: [edk2-devel] [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand
Sorry for the delay reply. See below: > -Original Message- > From: Vladimir Olovyannikov > Sent: Tuesday, July 28, 2020 12:49 AM > To: devel@edk2.groups.io > Cc: Laszlo Ersek ; Vladimir Olovyannikov > ; Samer El-Haj-Mahmoud haj-mahm...@arm.com>; Gao, Zhichao ; Maciej > Rabeda ; Wu, Jiaxin ; Fu, > Siyuan ; Ni, Ray ; Gao, Liming > ; Nd > Subject: [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand > > Introduce an http client utilizing EDK2 HTTP protocol, to > allow fast image downloading from http/https servers. > HTTP download speed is usually faster than tftp. > The client is based on the same approach as tftp dynamic command, and > uses the same UEFI Shell command line parameters. This makes it easy > integrating http into existing UEFI Shell scripts. > Note that to enable HTTP download, feature Pcd > gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections must > be set to TRUE. > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2860 > > Signed-off-by: Vladimir Olovyannikov > CC: Samer El-Haj-Mahmoud > CC: Laszlo Ersek > Cc: Zhichao Gao > Cc: Maciej Rabeda > Cc: Jiaxin Wu > Cc: Siyuan Fu > Cc: Ray Ni > Cc: Liming Gao > Cc: Nd > --- > CryptoPkg/Library/OpensslLib/openssl |2 +- > .../DynamicCommand/HttpDynamicCommand/Http.c | 1715 > + > .../DynamicCommand/HttpDynamicCommand/Http.h | 89 + > .../HttpDynamicCommand/Http.uni | 117 ++ > .../HttpDynamicCommand/HttpApp.c | 53 + > .../HttpDynamicCommand/HttpApp.inf| 58 + > .../HttpDynamicCommand/HttpDynamicCommand.c | 134 ++ > .../HttpDynamicCommand/HttpDynamicCommand.inf | 63 + > ShellPkg/Include/Guid/ShellLibHiiGuid.h |5 + > ShellPkg/ShellPkg.dec |1 + > ShellPkg/ShellPkg.dsc |5 + > 11 files changed, 2241 insertions(+), 1 deletion(-) > create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c > create mode 100644 > ShellPkg/DynamicCommand/HttpDynamicCommand/Http.h > create mode 100644 > ShellPkg/DynamicCommand/HttpDynamicCommand/Http.uni > create mode 100644 > ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.c > create mode 100644 > ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.inf > create mode 100644 > ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.c > create mode 100644 > ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf > > diff --git a/CryptoPkg/Library/OpensslLib/openssl > b/CryptoPkg/Library/OpensslLib/openssl > index e2e09d9fba11..c3656cc594da 16 > --- a/CryptoPkg/Library/OpensslLib/openssl > +++ b/CryptoPkg/Library/OpensslLib/openssl > @@ -1 +1 @@ > -Subproject commit e2e09d9fba1187f8d6aafaa34d4172f56f1ffb72 > +Subproject commit c3656cc594daac8167721dde7220f0e59ae146fc > diff --git a/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c > b/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c > new file mode 100644 > index ..0565b07c3570 > --- /dev/null > +++ b/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c > @@ -0,0 +1,1715 @@ > +/** @file > > + The implementation for the 'http' Shell command. > > + > > + Copyright (c) 2015, ARM Ltd. All rights reserved. > > + Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved. > > + (C) Copyright 2015 Hewlett Packard Enterprise Development LP > > + Copyright (c) 2020, Broadcom. All rights reserved. > > + > > + SPDX-License-Identifier: BSD-2-Clause-Patent > > +**/ > > + > > +#include "Http.h" > > + > > +#define IP4_CONFIG2_INTERFACE_INFO_NAME_LENGTH 32 > > +EFI_HII_HANDLE mHttpHiiHandle; > > + > > +/* > > + Constant strings and definitions related to the message > > + indicating the amount of progress in the dowloading of a HTTP file. > > +*/ > > + > > +// Number of steps in the progression slider > > +#define HTTP_PROGRESS_SLIDER_STEPS \ > > + ((sizeof (HTTP_PROGR_FRAME) / sizeof (CHAR16)) - 3) > > + > > +// Size in number of characters plus one (final zero) of the message to > > +// indicate the progress of an HTTP download. The format is "[(progress > slider: > > +// 40 characters)] (nb of KBytes downloaded so far: 7 characters) Kb". There > > +// are thus the number of characters in HTTP_PROGR_FRAME[] plus 11 > characters > > +// (2 // spaces, "Kb" and seven characters for the number of KBytes). > > +#define HTTP_PROGRESS_MESSAGE_SIZE \ > > + ((sizeof (HTTP_PROGR_FRAME) / sizeof (CHAR16)) + 12) > > + > > +// > > +// Buffer size. Note that larger buffer does not mean better speed! > > +// > > +#define DEFAULT_BUF_SIZE SIZE_32KB > > +#define MAX_BUF_SIZE SIZE_4MB > > + > > +#define MIN_PARAM_COUNT 2 > > +#define MAX_PARAM_COUNT 4 > > + > > +#define TIMER_MAX_TIMEOUT_S 10 > > + > > +// File name to use when URI ends with "/" > > +#define DEFAULT_HTML_FILE L"index.html" > > +#define DEFAULT_HTTP_PROTOL"http" > > + > > +// String to delete the
Re: [edk2-devel] [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand
Hi Laszlo,
Thank you for the comments.
I agree with them, and as you suggest, I will address them along with
comments from maintainers.
Thank you,
Vladimir
> -Original Message-
> From: devel@edk2.groups.io On Behalf Of Laszlo
> Ersek
> Sent: Monday, July 27, 2020 3:08 PM
> To: devel@edk2.groups.io; vladimir.olovyanni...@broadcom.com
> Cc: Samer El-Haj-Mahmoud ; Zhichao
> Gao ; Maciej Rabeda
> ; Jiaxin Wu ; Siyuan
> Fu ; Ray Ni ; Liming Gao
> ; Nd
> Subject: Re: [edk2-devel] [PATCH v5 1/1] ShellPkg/DynamicCommand: add
> HttpDynamicCommand
>
> Just some quick remarks after a comparison with v3:
>
> On 07/27/20 18:48, Vladimir Olovyannikov via groups.io wrote:
> > Introduce an http client utilizing EDK2 HTTP protocol, to allow fast
> > image downloading from http/https servers.
> > HTTP download speed is usually faster than tftp.
> > The client is based on the same approach as tftp dynamic command, and
> > uses the same UEFI Shell command line parameters. This makes it easy
> > integrating http into existing UEFI Shell scripts.
> > Note that to enable HTTP download, feature Pcd
> > gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections must be set to
> > TRUE.
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2860
> >
> > Signed-off-by: Vladimir Olovyannikov
> >
> > CC: Samer El-Haj-Mahmoud
> > CC: Laszlo Ersek
>
> (1) These "CC" lines are not formatted correctly -- they might do the job
> as
> far as git-send-email is concerned, but they don't satisfy
> "PatchCheck.py":
>
> > ShellPkg/DynamicCommand: add HttpDynamicCommand The commit
> message
> > format is not valid:
> > * 'CC' should be 'Cc'
> > * 'CC' should be 'Cc'
> > https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-F
> > ormat
>
> The exit status is 255, so this would break the CI run again.
>
> Please run "PatchCheck.py" locally before posting, and/or submit a
> personal
> CI build.
>
> [...]
>
> > +if (UserNicName != NULL) {
> > + if (StrCmp (NicName, UserNicName) != 0) {
> > +Status = EFI_NOT_FOUND;
>
> Change is new since v4, but not documented in the v5 changelog.
>
> (The code may be OK, but please help reviewers with the v(n) -> v(n+1)
> changelogs.)
>
> [...]
>
> > +if (ShellCommandLineGetFlag (CheckPackage, L"-m")) {
> > + Context.Flags |= DL_FLAG_TIME;
> > +}
>
> (2) The "-m" flag has not been removed from here
>
> [...]
>
> > +// Download Flags
> > +#define DL_FLAG_TIME BIT0 // Show elapsed time.
>
> (3) and here
>
> > +".SH SYNOPSIS\r\n"
> > +" \r\n"
> > +"HTTP [-i interface] [-l port] [-t timeout] [-s size] [-m] [-k]\r\n"
>
> (4) and here
>
> [...]
>
> > +" -m Measure and report download time (in seconds).
> > \r\n"
>
> (5) and here.
>
> I suggest waiting for ShellPkg owner feedback before posting v6.
>
> Thanks!
> Laszlo
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#63704): https://edk2.groups.io/g/devel/message/63704
Mute This Topic: https://groups.io/mt/75826968/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand
Maciej, Jiaxin and Siyuan: As you are network exporters, could you give the comments for this change? Thanks Liming -Original Message- From: Vladimir Olovyannikov Sent: 2020年7月28日 0:49 To: devel@edk2.groups.io Cc: Laszlo Ersek ; Vladimir Olovyannikov ; Samer El-Haj-Mahmoud ; Gao, Zhichao ; Maciej Rabeda ; Wu, Jiaxin ; Fu, Siyuan ; Ni, Ray ; Gao, Liming ; Nd Subject: [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand Introduce an http client utilizing EDK2 HTTP protocol, to allow fast image downloading from http/https servers. HTTP download speed is usually faster than tftp. The client is based on the same approach as tftp dynamic command, and uses the same UEFI Shell command line parameters. This makes it easy integrating http into existing UEFI Shell scripts. Note that to enable HTTP download, feature Pcd gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections must be set to TRUE. BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2860 Signed-off-by: Vladimir Olovyannikov CC: Samer El-Haj-Mahmoud CC: Laszlo Ersek Cc: Zhichao Gao Cc: Maciej Rabeda Cc: Jiaxin Wu Cc: Siyuan Fu Cc: Ray Ni Cc: Liming Gao Cc: Nd --- CryptoPkg/Library/OpensslLib/openssl |2 +- .../DynamicCommand/HttpDynamicCommand/Http.c | 1715 + .../DynamicCommand/HttpDynamicCommand/Http.h | 89 + .../HttpDynamicCommand/Http.uni | 117 ++ .../HttpDynamicCommand/HttpApp.c | 53 + .../HttpDynamicCommand/HttpApp.inf| 58 + .../HttpDynamicCommand/HttpDynamicCommand.c | 134 ++ .../HttpDynamicCommand/HttpDynamicCommand.inf | 63 + ShellPkg/Include/Guid/ShellLibHiiGuid.h |5 + ShellPkg/ShellPkg.dec |1 + ShellPkg/ShellPkg.dsc |5 + 11 files changed, 2241 insertions(+), 1 deletion(-) create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.h create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.uni create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.c create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.inf create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.c create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf diff --git a/CryptoPkg/Library/OpensslLib/openssl b/CryptoPkg/Library/OpensslLib/openssl index e2e09d9fba11..c3656cc594da 16 --- a/CryptoPkg/Library/OpensslLib/openssl +++ b/CryptoPkg/Library/OpensslLib/openssl @@ -1 +1 @@ -Subproject commit e2e09d9fba1187f8d6aafaa34d4172f56f1ffb72 +Subproject commit c3656cc594daac8167721dde7220f0e59ae146fc diff --git a/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c b/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c new file mode 100644 index ..0565b07c3570 --- /dev/null +++ b/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c @@ -0,0 +1,1715 @@ +/** @file + The implementation for the 'http' Shell command. + + Copyright (c) 2015, ARM Ltd. All rights reserved. + Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved. + (C) Copyright 2015 Hewlett Packard Enterprise Development LP + Copyright (c) 2020, Broadcom. All rights reserved. + + SPDX-License-Identifier: BSD-2-Clause-Patent +**/ + +#include "Http.h" + +#define IP4_CONFIG2_INTERFACE_INFO_NAME_LENGTH 32 +EFI_HII_HANDLE mHttpHiiHandle; + +/* + Constant strings and definitions related to the message + indicating the amount of progress in the dowloading of a HTTP file. +*/ + +// Number of steps in the progression slider +#define HTTP_PROGRESS_SLIDER_STEPS \ + ((sizeof (HTTP_PROGR_FRAME) / sizeof (CHAR16)) - 3) + +// Size in number of characters plus one (final zero) of the message to +// indicate the progress of an HTTP download. The format is "[(progress slider: +// 40 characters)] (nb of KBytes downloaded so far: 7 characters) Kb". There +// are thus the number of characters in HTTP_PROGR_FRAME[] plus 11 characters +// (2 // spaces, "Kb" and seven characters for the number of KBytes). +#define HTTP_PROGRESS_MESSAGE_SIZE \ + ((sizeof (HTTP_PROGR_FRAME) / sizeof (CHAR16)) + 12) + +// +// Buffer size. Note that larger buffer does not mean better speed! +// +#define DEFAULT_BUF_SIZE SIZE_32KB +#define MAX_BUF_SIZE SIZE_4MB + +#define MIN_PARAM_COUNT 2 +#define MAX_PARAM_COUNT 4 + +#define TIMER_MAX_TIMEOUT_S 10 + +// File name to use when URI ends with "/" +#define DEFAULT_HTML_FILE L"index.html" +#define DEFAULT_HTTP_PROTOL"http" + +// String to delete the HTTP progress message to be able to update it : +// (HTTP_PROGRESS_MESSAGE_SIZE-1) '\b' +#define HTTP_PROGRESS_DEL \ + L"\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\ +\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b" + +#define HTTP_KB L"\b\b\b\b\b\b\b\b\b\b"
Re: [edk2-devel] [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand
Just some quick remarks after a comparison with v3:
On 07/27/20 18:48, Vladimir Olovyannikov via groups.io wrote:
> Introduce an http client utilizing EDK2 HTTP protocol, to
> allow fast image downloading from http/https servers.
> HTTP download speed is usually faster than tftp.
> The client is based on the same approach as tftp dynamic command, and
> uses the same UEFI Shell command line parameters. This makes it easy
> integrating http into existing UEFI Shell scripts.
> Note that to enable HTTP download, feature Pcd
> gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections must
> be set to TRUE.
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2860
>
> Signed-off-by: Vladimir Olovyannikov
> CC: Samer El-Haj-Mahmoud
> CC: Laszlo Ersek
(1) These "CC" lines are not formatted correctly -- they might do the
job as far as git-send-email is concerned, but they don't satisfy
"PatchCheck.py":
> ShellPkg/DynamicCommand: add HttpDynamicCommand
> The commit message format is not valid:
> * 'CC' should be 'Cc'
> * 'CC' should be 'Cc'
> https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format
The exit status is 255, so this would break the CI run again.
Please run "PatchCheck.py" locally before posting, and/or submit a
personal CI build.
[...]
> +if (UserNicName != NULL) {
> + if (StrCmp (NicName, UserNicName) != 0) {
> +Status = EFI_NOT_FOUND;
Change is new since v4, but not documented in the v5 changelog.
(The code may be OK, but please help reviewers with the v(n) -> v(n+1)
changelogs.)
[...]
> +if (ShellCommandLineGetFlag (CheckPackage, L"-m")) {
> + Context.Flags |= DL_FLAG_TIME;
> +}
(2) The "-m" flag has not been removed from here
[...]
> +// Download Flags
> +#define DL_FLAG_TIME BIT0 // Show elapsed time.
(3) and here
> +".SH SYNOPSIS\r\n"
> +" \r\n"
> +"HTTP [-i interface] [-l port] [-t timeout] [-s size] [-m] [-k]\r\n"
(4) and here
[...]
> +" -m Measure and report download time (in seconds). \r\n"
(5) and here.
I suggest waiting for ShellPkg owner feedback before posting v6.
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#63363): https://edk2.groups.io/g/devel/message/63363
Mute This Topic: https://groups.io/mt/75826968/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand
Introduce an http client utilizing EDK2 HTTP protocol, to
allow fast image downloading from http/https servers.
HTTP download speed is usually faster than tftp.
The client is based on the same approach as tftp dynamic command, and
uses the same UEFI Shell command line parameters. This makes it easy
integrating http into existing UEFI Shell scripts.
Note that to enable HTTP download, feature Pcd
gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections must
be set to TRUE.
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2860
Signed-off-by: Vladimir Olovyannikov
CC: Samer El-Haj-Mahmoud
CC: Laszlo Ersek
Cc: Zhichao Gao
Cc: Maciej Rabeda
Cc: Jiaxin Wu
Cc: Siyuan Fu
Cc: Ray Ni
Cc: Liming Gao
Cc: Nd
---
CryptoPkg/Library/OpensslLib/openssl |2 +-
.../DynamicCommand/HttpDynamicCommand/Http.c | 1715 +
.../DynamicCommand/HttpDynamicCommand/Http.h | 89 +
.../HttpDynamicCommand/Http.uni | 117 ++
.../HttpDynamicCommand/HttpApp.c | 53 +
.../HttpDynamicCommand/HttpApp.inf| 58 +
.../HttpDynamicCommand/HttpDynamicCommand.c | 134 ++
.../HttpDynamicCommand/HttpDynamicCommand.inf | 63 +
ShellPkg/Include/Guid/ShellLibHiiGuid.h |5 +
ShellPkg/ShellPkg.dec |1 +
ShellPkg/ShellPkg.dsc |5 +
11 files changed, 2241 insertions(+), 1 deletion(-)
create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c
create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.h
create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.uni
create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.c
create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.inf
create mode 100644
ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.c
create mode 100644
ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf
diff --git a/CryptoPkg/Library/OpensslLib/openssl
b/CryptoPkg/Library/OpensslLib/openssl
index e2e09d9fba11..c3656cc594da 16
--- a/CryptoPkg/Library/OpensslLib/openssl
+++ b/CryptoPkg/Library/OpensslLib/openssl
@@ -1 +1 @@
-Subproject commit e2e09d9fba1187f8d6aafaa34d4172f56f1ffb72
+Subproject commit c3656cc594daac8167721dde7220f0e59ae146fc
diff --git a/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c
b/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c
new file mode 100644
index ..0565b07c3570
--- /dev/null
+++ b/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c
@@ -0,0 +1,1715 @@
+/** @file
+ The implementation for the 'http' Shell command.
+
+ Copyright (c) 2015, ARM Ltd. All rights reserved.
+ Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.
+ (C) Copyright 2015 Hewlett Packard Enterprise Development LP
+ Copyright (c) 2020, Broadcom. All rights reserved.
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include "Http.h"
+
+#define IP4_CONFIG2_INTERFACE_INFO_NAME_LENGTH 32
+EFI_HII_HANDLE mHttpHiiHandle;
+
+/*
+ Constant strings and definitions related to the message
+ indicating the amount of progress in the dowloading of a HTTP file.
+*/
+
+// Number of steps in the progression slider
+#define HTTP_PROGRESS_SLIDER_STEPS \
+ ((sizeof (HTTP_PROGR_FRAME) / sizeof (CHAR16)) - 3)
+
+// Size in number of characters plus one (final zero) of the message to
+// indicate the progress of an HTTP download. The format is "[(progress slider:
+// 40 characters)] (nb of KBytes downloaded so far: 7 characters) Kb". There
+// are thus the number of characters in HTTP_PROGR_FRAME[] plus 11 characters
+// (2 // spaces, "Kb" and seven characters for the number of KBytes).
+#define HTTP_PROGRESS_MESSAGE_SIZE \
+ ((sizeof (HTTP_PROGR_FRAME) / sizeof (CHAR16)) + 12)
+
+//
+// Buffer size. Note that larger buffer does not mean better speed!
+//
+#define DEFAULT_BUF_SIZE SIZE_32KB
+#define MAX_BUF_SIZE SIZE_4MB
+
+#define MIN_PARAM_COUNT 2
+#define MAX_PARAM_COUNT 4
+
+#define TIMER_MAX_TIMEOUT_S 10
+
+// File name to use when URI ends with "/"
+#define DEFAULT_HTML_FILE L"index.html"
+#define DEFAULT_HTTP_PROTOL"http"
+
+// String to delete the HTTP progress message to be able to update it :
+// (HTTP_PROGRESS_MESSAGE_SIZE-1) '\b'
+#define HTTP_PROGRESS_DEL \
+ L"\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\
+\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"
+
+#define HTTP_KB L"\b\b\b\b\b\b\b\b\b\b"
+// Frame for the progression slider
+#define HTTP_PROGR_FRAME L"[]"
+
+// String descriptions for server errors
+STATIC CONST CHAR16 *ErrStatusDesc[] =
+{
+ L"400 Bad Request",
+ L"401 Unauthorized",
+ L"402 Payment required",
+ L"403 Forbidden",
+ L"404 Not Found",
+ L"405 Method not allowed",
+ L"406 Not acceptable",
+ L"407 Proxy authentication required",
+ L"408 Request time out",
+ L"409 Conflict",
+ L"410 Gone",
+ L"411 Length required",
+
