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

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

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.

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.


>
>Thanks,
>Paulo