Hi Ray, Thanks for the review. My comments below.
On 8/9/2017 3:05 AM, Ni, Ruiyu wrote:
Paulo, Thanks for enabling the UDF support into EDKII. Below are several comments: 1. Could you please separate the patch to modify MdePkg and MdeModulePkg?
Sure.
2. UDF_CDROM_VOLUME_IDENTIFIER is also defined in Eltorito.h as CDVOL_ID. Maybe we could redefine CDVOL_ID to UDF_CDROM_VOLUME_IDENTIFIER?
The Volume Identifier structure is defined in ECMA-167 specification (not UDF specific), so perhaps it would be better if we create a separate header file (e.g., Ecma-167.h) and define ECMA167_VOLUME_IDENTIFIER structure. That would be used in both ElTorito and UDF codes. What do you think?
2. Why do you need a PCD to control the UDF support? I prefer no. More choices is not always good😊
The original idea of including this PCD was to avoid adding unnecessary overhead in PartitionDxe driver to something (UDF) that wasn't actually defined in UEFI specification -- leaving it as an optional feature.
But yes, I agree with you that just complicates things and would be better to remove it.
3. Is gUdfVolumeSignatureGuid only used in Partition driver to produce the device path? Can we just use HARDDRIVE_DEVICE_PATH? Or at least gUdfVolumeSignatureGuid can be removed, the GUID macro is enough? 4. Have you verified on a CDROM which contains both ElTorito and CDFS?
In the past I tested it with some Windows 8 and 10 ISO images I grabbed somewhere (both had ISO9660 & ElTorito's boot catalogs) but I haven't verified if it *still* works with them, yet. I'll do it when I get a chance.
Thanks, Paulo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel