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

[Paulo Alcantara <pcacjr@zytor.com>]

Hi,

(sorry for the late reply)

On 09/08/2017 22:11, Ni, Ruiyu wrote:
> 
> 
> Regards,
> Ray
> 
>> -----Original Message-----
>> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>> Sent: Wednesday, August 9, 2017 10:01 PM
>> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
>> Cc: Dong, Eric <eric.dong@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
>> Andrew Fish <afish@apple.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
>> Laszlo Ersek <lersek@redhat.com>
>> Subject: Re: [edk2] [PATCH 0/4] read-only UDF file system support
>>
>> (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?
> 
> I originally thought UdfDxe driver needs to use this GUID to know it's a UDF partition.
> But later I found UdfDxe contains a function SupportUdfFileSystem() to parse the
> partition content to decide whether to manage that partition.
> I wasn't able to find the reference of this GUID.

Yes - you're right. I think it should just check for the installed GUID, 
indeed. So we don't have to parse and look for UDF file systems twice.

> 
> I just tried again, still didn't find it. But I did find some improper implementation of
> driver-model logic.
> I saw UdfDxe uses OPEN_PROTOCOL_GET to open the BlockIo and DiskIo.
> It should use OPEN_PROTOCOL_BY_DRIVER.
> In details, it should firstly try OPEN_BY_DRIVER to open the BlockIo and DiskIo in
> Supported(), when either one fails, the Supported() returns failure. But please
> keep in mind to close the successfully-opened BlockIo/DiskIo. Supported() is a
> test function called by DxeCore, and it should not alter the system state.
> Start() should then repeat the same open logic as that in Supported(), but it's
> find to use ASSERT_EFI_ERROR() to assert the return status of OPEN_BY_DRIVER
> is EFI_SUCCESS because per UEFI spec, Start() should only be called when Supported()
> returns EFI_SUCCESS. Start() will then install the SimpleFileSystem instance on the
> same Handle that have BlockIo/DiskIo installed. (I noticed that your driver creates
> a new handle for SimpleFileSystem, which is unnecessary and may break the driver
> model chain.)
> 
> The OPEN_BY_DRIVER open is necessary because DxeCore driver model core logic
> records the special open operation. and when BlockIo or DiskIo is uninstalled,
> the Stop() function whose corresponding Start() is opening the same BlockIo/DiskIo
> OPEN_BY_DRIVER will get called. So you can treat OPEN_BY_DRIVER is a mechanism
> to notify the upper layer driver to destroy the newly created service(SimpleFileSystem)
> when lower layer services (BlockIo/DiskIo) it layers on is destroyed (Uninstalled).
> 
> Not sure if I explained well. You could refer to
> https://github.com/tianocore/tianocore.github.io/wiki/UEFI-Driver-Writer's-Guide
> written by Kinney Michael for detailed instructions.

Good catch! You explained it very well. I'll fix that up in the next series.

Thanks!
Paulo