Thanks for the review. My comments below.
On 8/9/2017 3:05 AM, Ni, Ruiyu wrote:
Thanks for enabling the UDF support into EDKII.
Below are several comments:
1. Could you please separate the patch to modify MdePkg and MdeModulePkg?
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
path? Can we just use HARDDRIVE_DEVICE_PATH? Or at least
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.
edk2-devel mailing list