From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by mx.groups.io with SMTP id smtpd.web11.70549.1597769668715740917 for ; Tue, 18 Aug 2020 09:54:28 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: linux.intel.com, ip: 192.55.52.93, mailfrom: maciej.rabeda@linux.intel.com) IronPort-SDR: Agr/zzgTykm6tnEl3y/nzdk3QXn22nnD2Z82DeqnZ3uGCu1UzfkbNgljruqpwNGpjbCQ0m86sB zsbXSblCqRbQ== X-IronPort-AV: E=McAfee;i="6000,8403,9717"; a="152578979" X-IronPort-AV: E=Sophos;i="5.76,328,1592895600"; d="scan'208";a="152578979" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Aug 2020 09:54:27 -0700 IronPort-SDR: YwMFrEBMD9cksDcZCb3KGd1gUxvLdqArxt/e1TAU4O5SgfFLTEyGA1qbTIkOFF8oZyEPjvXgt+ mAXprjtz0pcg== X-IronPort-AV: E=Sophos;i="5.76,328,1592895600"; d="scan'208";a="497432600" Received: from mrabeda-mobl.ger.corp.intel.com (HELO [10.249.137.110]) ([10.249.137.110]) by fmsmga005-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Aug 2020 09:54:25 -0700 Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand 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 References: <20200727164830.25829-1-vladimir.olovyannikov@broadcom.com> <20200727164830.25829-2-vladimir.olovyannikov@broadcom.com> <25b5ce9498095e68650b8817e4523714@mail.gmail.com> <2a710789-bfdd-dd80-386a-04fbd46d4835@redhat.com> From: "Maciej Rabeda" Message-ID: Date: Tue, 18 Aug 2020 18:54:23 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: pl 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