Re: [edk2-devel] [PATCH 4/6] NetworkPkg/IScsiDxe: support multiple hash algorithms for CHAP

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

Hi Maciej,

[snipping liberally, comments below]

On 06/25/21 16:56, Rabeda, Maciej wrote:
> On 22-Jun-21 17:57, Laszlo Ersek wrote:
>> On 06/11/21 13:54, Rabeda, Maciej wrote:
>>> On 08-Jun-21 15:06, Laszlo Ersek wrote:

>>>> +typedef struct {
>>>> +  UINT8                      Algorithm;      //
>>>> ISCSI_CHAP_ALGORITHM_*, CHAP_A
>>> Any chance to define an enum type for Algorithm? IMHO it brings more
>>> meaning to that number and limits the set of values it is expected
>>> to have.
>> I picked UINT8 intentionally; I wanted to stay close to the
>> definition at
>>
>> https://www.iana.org/assignments/ppp-numbers/ppp-numbers.xhtml#ppp-numbers-9
>>
>>
>> which says, "A one octet field". (Hence the reference to "CHAP_A" in
>> the comment as well.)
>>
>> If we want an enum here, then I need to prepend a patch, for first
>> replacing the existent ISCSI_CHAP_ALGORITHM_MD5 macro with an enum
>> type / constant.
>>
>> I still like UINT8 for this, but if you strongly prefer the enum, I
>> can certainly do it. Please advise :)
> Okay, let's stick to UINT8 based on IANA definition.

Thanks!

>>>>    -  if (CompareMem (VerifyRsp, TargetResponse, MD5_DIGEST_SIZE) !=
>>>> 0) {
>>>> +  if (CompareMem (VerifyRsp, TargetResponse,
>>>> +        AuthData->Hash->DigestSize) != 0) {
>>>>        Status = EFI_SECURITY_VIOLATION;
>>>>      }
>>> Either break like regular function call or leave it in a single line
>>> for readability, since UEFI coding standard section 5.1.1 allows for
>>> lines up to 120. I opt for the latter, since param break will look
>>> ugly, but if it looks better in your editor than a line over 80
>>> chars in size - go for it.
>> My mistake, sorry -- I forgot that the "condensed style" that we
>> actually prefer in ArmVirtPkg and OvmfPkg is not accepted in other
>> packages.
> Making it the other way around. Apart from historical reasons (EDK2
> coding style evolving) - why is a different style accepted in other
> packages? We have a EDK2 coding style document for a reason.
> If it is not perfect, it does not suit our needs or simply hurts our
> eyes (which 'param per line' in if statements does to me), we can try
> to change it to the "condensed style".

* Submitted on 11 Aug 2017:

  [edk2] [edk2-CCodingStandardsSpecification PATCH 0/2] improvements related to line wrapping
  [edk2] [edk2-CCodingStandardsSpecification PATCH 1/2] Source Files / General Rules: limit line lengths to 80 columns
  [edk2] [edk2-CCodingStandardsSpecification PATCH 2/2] Source Files / Spacing / Multi-line func. calls: allow condensed arguments

  http://mid.mail-archive.com/20170811164851.9466-1-lersek@redhat.com
  http://mid.mail-archive.com/20170811164851.9466-2-lersek@redhat.com
  http://mid.mail-archive.com/20170811164851.9466-3-lersek@redhat.com

  The condensed arguments style is described in patch 2/2.

* Filed in September 2017, from the discussion under patch 2/2:

  https://bugzilla.tianocore.org/show_bug.cgi?id=714

  In CONFIRMED status currently.

  (The link to the original email discussion (in comment#0 of the BZ) no
  longer works, because Intel has killed off the old <lists.01.org>
  archive links meanwhile. But the <mail-archive.com> links should still
  work.)

Thanks
Laszlo