On 04/10/18 09:40, Long, Qin wrote: > Hi, Laszlo, > > Some comments / discussions were added in > https://bugzilla.tianocore.org/show_bug.cgi?id=915 > with comment 09 & 11.
Right. There I missed your main point about "TlsCipherMappingTable"; I'm sorry about that. With Jiaxin's more detailed explanation, I get it now. > Back to the patch review. Some comments were appended: > > #0001, #0003, #0004，#0010，#0011: > Looks good to me. Thanks -- are you OK if I squash 10 and 11, like Jiaxin suggests? > #0002 - I personally think in general we should reduce using "#pragma pack", > except that these > data really have serialization requirement (e.g. variable) to match > extra data layout. > Here we just use these structures for setting / getting data, instead > of direct data > transport. I am thinking if it's better to update the implementation > part. > But too many sizeof() were used, and Ovmf part may also need to store > preferred > CipherList data. So it's still good to me to pack something. Well, I think the *set* of structures that should be packed is clear -- given that we make explicit references to the TLS RFC, I believe we *must* pack those structures. The remaining question I see (with reference to Liming's suggestion earlier) is whether we should use separate #pragma directives for the individual struct types, or if we should move those structs to a common section of the header file called "structures from the TLS RFC" or something like that, and pack them with a single #pragma. > #0008, #0009: > - As the BZ comments. The TlsCipherMappingTable extension and > generation with script looks > incorrect. This table should include all available / supported > ciphers, which was actually > platform / configuration dependent. > I prefer to maintain one static / limited table for edk2 tls > implementation. Any new cipher > requirement can be evaluated & enabled, and then added into this > table. Yup, I'm ready to drop patches #8 and #9. Thanks for your patience explaining. > #0005, #0006, #0007, #0012， #0013: > These implementation looks good to me. > But some of updates were based on the assumption of #0008-0009. I > have no strong opinion > if some original light implementation are good enough currently. Thanks. My personal preference would be the following -- making sure Jiaxin is CC'd... yes, he is: (a) clarify how exactly we want to pack the structures in patch #2, and rework patch #2 according to agreement (b) keep *each* of the following patches separate: 01 OvmfPkg/TlsAuthConfigLib: configure trusted cipher suites for HTTPS boot 02 MdePkg/Include/Protocol/Tls.h: pack structures from the TLS RFC 03 NetworkPkg/TlsDxe: verify DataSize for EfiTlsCipherList 04 NetworkPkg/TlsDxe: clean up byte order conversion for EfiTlsCipherList 05 CryptoPkg/TlsLib: replace TlsGetCipherString() with TlsGetCipherMapping() 06 CryptoPkg/TlsLib: use binary search in the TlsGetCipherMapping() function 07 CryptoPkg/TlsLib: pre-compute OpensslCipherLength in TlsCipherMappingTable (c) drop the following patches: 08 CryptoPkg/TlsLib: add the "TlsMappingTable.sh" POSIX shell script 09 CryptoPkg/TlsLib: extend "TlsCipherMappingTable" (d) squash the following patches together (into one patch): 10 CryptoPkg/TlsLib: sort [LibraryClasses] section in the INF file 11 CryptoPkg/TlsLib: sanitize lib classes in internal header and INF (e) squash the following patches together (into another patch): 12 CryptoPkg/TlsLib: clean up leading comment for TlsSetCipherList() 13 CryptoPkg/TlsLib: rewrite TlsSetCipherList() Jiaxin, does this look OK to you? Thanks! Laszlo _______________________________________________ edk2-devel mailing list email@example.com https://lists.01.org/mailman/listinfo/edk2-devel