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

[Paulo Alcantara <pcacjr@zytor.com>]

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