(heh - forgot to answer your question about the GUID :-) )

On 8/9/2017 10:26 AM, Paulo Alcantara wrote:
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?

The GUID is used in both Partition and UdfDxe drivers. Do you mean that we should use HARDDRIVE_DEVICE_PATH and then appending the UDF-specific GUID to it?

Thanks,
Paulo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to