Re: [PATCH 0/4] read-only UDF file system support

[Paulo Alcantara <pcacjr@zytor.com>]

(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