From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lj1-f180.google.com (mail-lj1-f180.google.com [209.85.208.180]) by mx.groups.io with SMTP id smtpd.web10.72574.1597775629351172913 for ; Tue, 18 Aug 2020 11:33:50 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@broadcom.com header.s=google header.b=R/fvxEm3; spf=permerror, err=parse error for token &{10 18 %{i}._ip.%{h}._ehlo.%{d}._spf.vali.email}: invalid domain name (domain: broadcom.com, ip: 209.85.208.180, mailfrom: vladimir.olovyannikov@broadcom.com) Received: by mail-lj1-f180.google.com with SMTP id t23so22571640ljc.3 for ; Tue, 18 Aug 2020 11:33:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=from:references:in-reply-to:mime-version:thread-index:date :message-id:subject:to:cc; bh=kzmveo/qFwzxG3QhBFKPUi2uq3RK+BqUjmII4Mfy9jg=; b=R/fvxEm3mo4sSoSjnXZOWk61Ut4rgo3i5ra6rARlVEH2szaM+02Hx2cJk0oS3u3tbd 97cPROuzl4EeVkGGQiFw7+xG16Y5ftI88m00P78rDAy/9jAXSiDb2G1azZ44HUNih5cu 4uJMKIgolCz+hpPtEOIR+tHrCk1Gqv0SPjYps= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:references:in-reply-to:mime-version :thread-index:date:message-id:subject:to:cc; bh=kzmveo/qFwzxG3QhBFKPUi2uq3RK+BqUjmII4Mfy9jg=; b=WN7hdrDQFpQ0x1XZX3ke/PXNt8o2bPR9l6GQUTnG7XoMItzWVk50QmNLykgcmDMpRT ZTXvobPfbgPuSl8Ul2nLtAv+vAbFAMnC89ahDpYaXOJlELMz72ZmMOsqpRqmG3WxJT3c xrCYQX6HG36og8zqEEC/UzNrZzLyomoeJn0UZYt/ssa/tc2BP9KbUcVZxgDkSCcpMfNE WC0tWbc7JdgEXNOxpLV6/WGDrfRhnHJHPaL1rFK1cYe9aQHSHZD/XOsZ019UpRW3LO8F 7661eh/Thpoo5r2PrZ8CuRg+R7AIdudP33HkOcb2nx5bx1JEY7YpY4ZAJl6U0tyML+kS PslQ== X-Gm-Message-State: AOAM531xEKj5m/uQ+waC/+QNHxb95uA/4iG1g/LiAGxM6wmG0oP8ceG3 4wUcjo9QZjxAWyhAJCNe+ap3cc4xpa8njF9oQ+W8LA== X-Google-Smtp-Source: ABdhPJxdXZkCmQb51zooIne21j5fT4jRaX7V//ct79gVhsbfjZGRtZxZ+nrm3yH8qH0ECT+R1zh/Xhi5xglhgFYFndU= X-Received: by 2002:a2e:b53b:: with SMTP id z27mr10565316ljm.441.1597775627347; Tue, 18 Aug 2020 11:33:47 -0700 (PDT) From: "Vladimir Olovyannikov" 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> In-Reply-To: MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQILIYHslp9C8beoIeXBb+C1khe8nAJZRV/xAGCCW/QCSmia/gFipDoLAhIip3QCjNqSBgK2ARQXAj2BhEYBjU0/KqhIoONg Date: Tue, 18 Aug 2020 11:33:44 -0700 Message-ID: <01083c792c35bba8472e4da75ee15867@mail.gmail.com> Subject: Re: [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand To: "Rabeda, Maciej" , Laszlo Ersek , "Gao, Zhichao" , devel@edk2.groups.io Cc: Samer El-Haj-Mahmoud , "Wu, Jiaxin" , "Fu, Siyuan" , "Ni, Ray" , "Gao, Liming" , Nd Content-Type: text/plain; charset="UTF-8" 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 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