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