Re: [PATCH 2/2] OvmfPkg: enable HttpDynamicCommand

["Laszlo Ersek" <lersek@redhat.com>]

On 07/22/20 22:02, Vladimir Olovyannikov wrote:
> Hi Laszlo,
> 
> Thank you for reviewing the patchset.

Thank *you* for submitting the set, at my request! :)

>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Wednesday, July 22, 2020 12:52 PM
>> To: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>;
>> devel@edk2.groups.io
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Leif Lindholm
>> <leif@nuviainc.com>
>> Subject: Re: [PATCH 2/2] OvmfPkg: enable HttpDynamicCommand
>>
>> On 07/21/20 19:23, Vladimir Olovyannikov wrote:
>>> Enable HttpDynamicCommand (Shell command "http") for OvmfPkg
>> platforms
>>>
>>> Signed-off-by: Vladimir Olovyannikov
>>> <vladimir.olovyannikov@broadcom.com>
>>> ---
>>>  OvmfPkg/OvmfPkgIa32.dsc    | 6 ++++++
>>>  OvmfPkg/OvmfPkgIa32X64.dsc | 6 ++++++
>>>  OvmfPkg/OvmfPkgX64.dsc     | 6 ++++++
>>>  OvmfPkg/OvmfXen.dsc        | 6 ++++++
>>>  4 files changed, 24 insertions(+)
>>>
>>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc index
>>> 9178ffeb71cb..aed3a73c0172 100644
>>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>>> @@ -883,6 +883,12 @@ [Components]
>>>      <PcdsFixedAtBuild>
>>>        gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>>>    }
>>> +
>>> +
>> ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand
>> .inf {
>>> +    <PcdsFixedAtBuild>
>>> +      gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>>> +  }
>>> +
>>>
>> OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellComma
>> nd.inf {
>>>      <PcdsFixedAtBuild>
>>>        gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc
>> b/OvmfPkg/OvmfPkgIa32X64.dsc
>>> index a665f78f0dc7..6c07326cc0fe 100644
>>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>>> @@ -897,6 +897,12 @@ [Components.X64]
>>>      <PcdsFixedAtBuild>
>>>        gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>>>    }
>>> +
>>> +
>> ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand
>> .inf {
>>> +    <PcdsFixedAtBuild>
>>> +      gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>>> +  }
>>> +
>>>
>> OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellComma
>> nd.inf {
>>>      <PcdsFixedAtBuild>
>>>        gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index
>>> 17f345acf4ee..d5e5bcf8e66c 100644
>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>> @@ -893,6 +893,12 @@ [Components]
>>>      <PcdsFixedAtBuild>
>>>        gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>>>    }
>>> +
>>> +
>> ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand
>> .inf {
>>> +    <PcdsFixedAtBuild>
>>> +      gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>>> +  }
>>> +
>>>
>> OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellComma
>> nd.inf {
>>>      <PcdsFixedAtBuild>
>>>        gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>>> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc index
>>> 782803cb2787..fe549502863a 100644
>>> --- a/OvmfPkg/OvmfXen.dsc
>>> +++ b/OvmfPkg/OvmfXen.dsc
>>> @@ -675,6 +675,12 @@ [Components]
>>>      <PcdsFixedAtBuild>
>>>        gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>>>    }
>>> +
>>> +
>> ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand
>> .inf {
>>> +    <PcdsFixedAtBuild>
>>> +      gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>>> +  }
>>> +
>>>
>> OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellComma
>> nd.inf {
>>>      <PcdsFixedAtBuild>
>>>        gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>>>
>>
>> Basically same comments as under the ArmVirtPkg patch:
>>
>> (1) Please mention <https://bugzilla.tianocore.org/show_bug.cgi?id=2857>
>> in the commit message.
> OK, will do. For future contributions related to a BZ ticket - does it mean
> that mentioning of BZ id in the cover letter is not enough?

Referencing the BZ in the cover letter only is indeed not ideal; the
cover letter does not get captured in the git history. When submitting a
series for a BZ, it's best to mention the BZ in every patch in the series.

One frequently used format is

"""
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2857
"""

placed near the end of the commit message (just above the Signed-off-by
and similar tags). But other formats are fine too.

>>
>> (2) Please don't introduce the extra whitespace in the DSC files.
>>
>> (3) Please also add the new module to the following (FDF) files:
>>
>> - OvmfPkgIa32.fdf
>> - OvmfPkgIa32X64.fdf
>> - OvmfPkgX64.fdf
>> - OvmfXen.fdf
> OK, will do, thanks.
>>
>> Once you post v2 of this series, with the above points fixed (especially
>> the
>> FDF files), I'd like to test the new command (with both ArmVirtQemu and
>> OVMF).
> Sure, that would be great.

Thanks!
Laszlo