[edk2-devel] [PATCH] NetworkPkg: Correct the length of EAP Identity when in ASCII format

[Clark-williams, Zachary]

Looks good to me.

From: Li, Yi1 <yi1.li at intel.com>
Sent: Monday, June 26, 2023 1:36 AM
To: Clark-williams, Zachary <zachary.clark-williams at intel.com>; devel at edk2.groups.io
Subject: RE: [edk2-devel] [PATCH] NetworkPkg: Correct the length of EAP Identity when in ASCII format

Hi Zack,

Please check V3 patch, thanks for review.

Regards,
Yi

From: Clark-williams, Zachary <zachary.clark-williams at intel.com<mailto:zachary.clark-williams at intel.com>>
Sent: Friday, June 23, 2023 6:26 AM
To: devel at edk2.groups.io<mailto:devel at edk2.groups.io>; Li, Yi1 <yi1.li at intel.com<mailto:yi1.li at intel.com>>; Clark-williams, Zachary <zachary.clark-williams at intel.com<mailto:zachary.clark-williams at intel.com>>
Subject: RE: [edk2-devel] [PATCH] NetworkPkg: Correct the length of EAP Identity when in ASCII format

Hey Yi,

I agree with the flow you have in the PR link and removing the extra protocol locate.

My only comment is to align your comments with the commenting style of the file.
Instead of /**/, use // for both single and multi-line comments.
Example included in the comments on the PR.

Thanks,
Zack


From: devel at edk2.groups.io<mailto:devel at edk2.groups.io> <devel at edk2.groups.io<mailto:devel at edk2.groups.io>> On Behalf Of Li, Yi
Sent: Monday, June 19, 2023 9:49 PM
To: Clark-williams; Clark-williams, Zachary <zachary.clark-williams at intel.com<mailto:zachary.clark-williams at intel.com>>; devel at edk2.groups.io<mailto:devel at edk2.groups.io>
Subject: Re: [edk2-devel] [PATCH] NetworkPkg: Correct the length of EAP Identity when in ASCII format

Hi Zachary,

Thanks for review.
The protocol has changes since ADL from PlatSapmle to an advanced feature and the Protocol has shifted into EDK2, so the protocol name needs to be updated:
+  Status = gBS->LocateProtocol (&gWiFiProfileSyncProtocolGuid, NULL, (VOID **) &WiFiProfileSyncProtocol);
    Status = gBS->LocateProtocol (&gEdkiiWiFiProfileSyncProtocolGuid, NULL, (VOID **)&WiFiProfileSyncProtocol);

The locate protocol status check is enough and we do not need to add the NULL check too, we can remove that to keep it lighter.
+    if (!EFI_ERROR (Status) && WiFiProfileSyncProtocol != NULL) {
Agree with those changes, please check latest V2 patch or this PR: https://github.com/tianocore/edk2/pull/4561
Can we clean up the second locate protocol and bring the Identity allocate above the protocol check, and bring the two conditions for EapIdentity copied to Identity into the added protocol check condition.
Here is a view of what I am thinking for consolidation.
We need to get the Identity size before AllocateZeroPool(), not feasible here IMO.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106435): https://edk2.groups.io/g/devel/message/106435
Mute This Topic: https://groups.io/mt/99335243/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20230627/f321fe2c/attachment.htm>