[edk2-devel] [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand

Tue Aug 18 18:33:44 UTC 2020

Hi Rabeda,
Thank you for reviewing.

> -----Original Message-----
> From: Rabeda, Maciej <maciej.rabeda at linux.intel.com>
> Sent: Tuesday, August 18, 2020 9:54 AM
> To: Vladimir Olovyannikov <vladimir.olovyannikov at broadcom.com>; Laszlo
> Ersek <lersek at redhat.com>; Gao, Zhichao <zhichao.gao at intel.com>;
> devel at edk2.groups.io
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud at arm.com>; Wu, Jiaxin
> <jiaxin.wu at intel.com>; Fu, Siyuan <siyuan.fu at intel.com>; Ni, Ray
> <ray.ni at intel.com>; Gao, Liming <liming.gao at intel.com>; Nd
> <nd at arm.com>
> 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 <lersek at redhat.com>
> >> Sent: Monday, August 17, 2020 1:44 PM
> >> To: Vladimir Olovyannikov <vladimir.olovyannikov at broadcom.com>;
> >> Rabeda, Maciej <maciej.rabeda at linux.intel.com>; Gao, Zhichao
> >> <zhichao.gao at intel.com>; devel at edk2.groups.io
> >> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud at arm.com>; Wu,
> Jiaxin
> >> <jiaxin.wu at intel.com>; Fu, Siyuan <siyuan.fu at intel.com>; Ni, Ray
> >> <ray.ni at intel.com>; Gao, Liming <liming.gao at intel.com>; Nd
> >> <nd at arm.com>
> >> Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add
> >> HttpDynamicCommand
> >>
> >> On 08/17/20 20:29, Vladimir Olovyannikov wrote:
> >>>> -----Original Message-----
> >>>> From: Laszlo Ersek <lersek at redhat.com>
> >>>> Sent: Monday, August 17, 2020 11:01 AM
> >>>> To: Rabeda, Maciej <maciej.rabeda at linux.intel.com>; Vladimir
> >>>> Olovyannikov <vladimir.olovyannikov at broadcom.com>; Gao, Zhichao
> >>>> <zhichao.gao at intel.com>; devel at edk2.groups.io
> >>>> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud at arm.com>; Wu,
> >> Jiaxin
> >>>> <jiaxin.wu at intel.com>; Fu, Siyuan <siyuan.fu at intel.com>; Ni, Ray
> >>>> <ray.ni at intel.com>; Gao, Liming <liming.gao at intel.com>; Nd
> >>>> <nd at arm.com>
> >>>> 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 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 (#64398): https://edk2.groups.io/g/devel/message/64398
Mute This Topic: https://groups.io/mt/75826968/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-