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

[Paulo Alcantara <pcacjr@zytor.com>]

Ard,

On September 8, 2017 7:17:35 PM GMT-03:00, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>On 8 September 2017 at 20:40, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 09/08/17 21:21, Ard Biesheuvel wrote:
>>> On 8 September 2017 at 19:47, Laszlo Ersek <lersek@redhat.com>
>wrote:
>>>> On 09/08/17 14:41, Paulo Alcantara wrote:
>>>>> Hi,
>>>>>
>>>>> This series introduces read-only UDF file system support in EDK2.
>As
>>>>> Laszlo (or Red Hat) seemed to be interested in such support, I'm
>posting
>>>>> it again after ~3 years.
>>>>>
>>>>> The idea is not replacing the default FAT file system, nor
>breaking any
>>>>> existing file system support, but extending EDK2 with a new file
>system
>>>>> that might be useful for some people who are looking for specific
>file
>>>>> system features that current FAT doesn't support.
>>>>>
>>>>> Originally the driver was written to support UDF file systems as
>>>>> specified by OSTA Universal Disk Format Specification 2.60.
>However,
>>>>> some Windows 10 Enterprise ISO (UDF bridge) images that I tested
>>>>> supported a revision of 1.02 thus I had to rework the driver a
>little
>>>>> bit to support such revision as well.
>>>>>
>>>>> v2:
>>>>>  - Rework to _partially_ support UDF revisions <2.60.
>>>>>  - Use existing CDROM_VOLUME_DESCRIPTOR structure defined in
>Eltorito.h
>>>>>    instead of creating another one (UDF_VOLUME_DESCRIPTOR).
>>>>>  - Fixed UdfDxe to correctly follow UEFI driver model.
>>>>>  - Use HARDDRIVE_DEVICE_PATH instead of a vendor-defined one.
>>>>>  - Detect UDF file systems only in PartitionDxe, and let UdfDxe
>driver
>>>>>    check for specific UDF device path to decide whether or not
>install
>>>>>    SimpleFs protocol.
>>>>>  - Place MdePkg changes in a separate patch.
>>>>> v3:
>>>>>  - Install UDF partition child handles with a Vendor-Defined Media
>>>>>    Device Path.
>>>>>  - Changed UdfDxe to check for Vendor-Defined Media Device Paths
>with a
>>>>>    specific UDF file system GUID when determining to whether or
>not
>>>>>    start the driver.
>>>>>  - Removed leading TAB chars in some source files identified by
>>>>>    PatchCheck.py tool.
>>>>> v4:
>>>>>  - Added missing R-b's.
>>>>> v5:
>>>>>  - Fixed OVMF IA32 build.
>>>>>  - Fixed a typo in UdfDriveBindingStop() ("This" -> "SimpleFs")
>which
>>>>>    broke retrieval of private fs data from SimpleFs protocol --
>>>>>    identified by 'reconnect -r' command in UEFI shell.
>>>>> v6:
>>>>>  - Fixed a bug in UdfRead() that'd pontentially break in ARM or
>IA32
>>>>>    by allowing caller to read more than 4GiB of data
>>>>>    (i.e. BufferSize pointer is dereferenced as an UINT64 * and
>it's
>>>>>     followed by 4 bytes that are nonzero).
>>>>>
>>>>> Repo:   https://github.com/pcacjr/edk2.git
>>>>> Branch: udf-fs-v6
>>>>>
>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>>>> Cc: Andrew Fish <afish@apple.com>
>>>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>>>> Cc: Liming Gao <liming.gao@intel.com>
>>>>> Cc: Star Zeng <star.zeng@intel.com>
>>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>>> Cc: Mark Doran <mark.doran@intel.com>
>>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>>>> Cc: hao.a.wu@intel.com
>>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>>> Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
>>>>> ---
>>>>>
>>>>> Paulo Alcantara (6):
>>>>>   MdePkg: Add UDF volume structure definitions
>>>>>   MdeModulePkg/PartitionDxe: Add UDF file system support
>>>>>   MdeModulePkg: Initial UDF/ECMA-167 file system support
>>>>>   OvmfPkg: Enable UDF file system support
>>>>>   ArmVirtPkg: Enable UDF file system support
>>>>>   Nt32Pkg: Enable UDF file system support
>>>>>
>>>>>  ArmVirtPkg/ArmVirtQemu.dsc                         |    3 +-
>>>>>  ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc               |    3 +-
>>>>>  ArmVirtPkg/ArmVirtQemuKernel.dsc                   |    3 +-
>>>>>  ArmVirtPkg/ArmVirtXen.dsc                          |    3 +-
>>>>>  ArmVirtPkg/ArmVirtXen.fdf                          |    1 +
>>>>>  .../Universal/Disk/PartitionDxe/Partition.c        |    9 +-
>>>>>  .../Universal/Disk/PartitionDxe/Partition.h        |   32 +-
>>>>>  .../Universal/Disk/PartitionDxe/PartitionDxe.inf   |    3 +-
>>>>>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c     |  318 +++
>>>>>  MdeModulePkg/Universal/Disk/UdfDxe/ComponentName.c |  185 ++
>>>>>  MdeModulePkg/Universal/Disk/UdfDxe/File.c          |  908
>++++++++
>>>>>  MdeModulePkg/Universal/Disk/UdfDxe/FileName.c      |  195 ++
>>>>>  .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 2447
>++++++++++++++++++++
>>>>>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.c           |  344 +++
>>>>>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.h           | 1244
>++++++++++
>>>>>  MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf      |   66 +
>>>>>  MdePkg/Include/IndustryStandard/Udf.h              |   60 +
>>>>>  Nt32Pkg/Nt32Pkg.dsc                                |    1 +
>>>>>  Nt32Pkg/Nt32Pkg.fdf                                |    1 +
>>>>>  OvmfPkg/OvmfPkgIa32.dsc                            |    1 +
>>>>>  OvmfPkg/OvmfPkgIa32.fdf                            |    1 +
>>>>>  OvmfPkg/OvmfPkgIa32X64.dsc                         |    1 +
>>>>>  OvmfPkg/OvmfPkgIa32X64.fdf                         |    1 +
>>>>>  OvmfPkg/OvmfPkgX64.dsc                             |    1 +
>>>>>  OvmfPkg/OvmfPkgX64.fdf                             |    1 +
>>>>>  25 files changed, 5821 insertions(+), 11 deletions(-)
>>>>>  create mode 100644 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
>>>>>  create mode 100644
>MdeModulePkg/Universal/Disk/UdfDxe/ComponentName.c
>>>>>  create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/File.c
>>>>>  create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/FileName.c
>>>>>  create mode 100644
>MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>>>>>  create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/Udf.c
>>>>>  create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/Udf.h
>>>>>  create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf
>>>>>  create mode 100644 MdePkg/Include/IndustryStandard/Udf.h
>>>>>
>>>>
>>>> Pushed as commit range 7aee391fa3d0..b696c64d4fc3.
>>>>
>>>
>>> This code breaks the Clang build:
>>>
>>>
><https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:937:11:
>>> error: variable 'Status' is used uninitialized whenever 'if'
>condition
>>> is false [-Werror,-Wsometimes-uninitialized]
>>>       if (ReadFileInfo->FileData == NULL) {
>>>           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
><https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:1148:10:
>>> note: uninitialized use occurs here
>>>   return Status;
>>>          ^~~~~~
>>>
><https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:937:7:
>>> note: remove the 'if' if its condition is always true
>>>       if (ReadFileInfo->FileData == NULL) {
>>>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
><https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:932:16:
>>> error: variable 'Status' is used uninitialized whenever 'if'
>condition
>>> is false [-Werror,-Wsometimes-uninitialized]
>>>     } else if (ReadFileInfo->Flags == READ_FILE_ALLOCATE_AND_READ) {
>>>                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
><https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:1148:10:
>>> note: uninitialized use occurs here
>>>   return Status;
>>>          ^~~~~~
>>>
><https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:932:12:
>>> note: remove the 'if' if its condition is always true
>>>     } else if (ReadFileInfo->Flags == READ_FILE_ALLOCATE_AND_READ) {
>>>            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
><https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:930:9:
>>> error: variable 'Status' is used uninitialized whenever 'if'
>condition
>>> is true [-Werror,-Wsometimes-uninitialized]
>>>     if (ReadFileInfo->Flags == READ_FILE_GET_FILESIZE) {
>>>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
><https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:1148:10:
>>> note: uninitialized use occurs here
>>>   return Status;
>>>          ^~~~~~
>>>
><https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:930:5:
>>> note: remove the 'if' if its condition is always false
>>>     if (ReadFileInfo->Flags == READ_FILE_GET_FILESIZE) {
>>>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
><https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:869:33:
>>> note: initialize the variable 'Status' to silence this warning
>>>   EFI_STATUS              Status;
>>>                                 ^
>>>                                  = 0
>>> 3 errors generated.
>>
>> Thanks for the report -- this was sort of expected, given the size of
>> the driver. My test builds for IA32/X64/ARM/AARCH64 with
>gcc-4.8/GCC48
>> and gcc-6.1.1/GCC5 didn't catch the above.
>>
>> I think if these issues were fixed, the build wouldn't complete
>> immediately; there are likely other instances.
>>
>> Could you please recommend a build command line, and perhaps clang
>> package names that are "usual" on a few GNU/Linux distros, to Paulo?
>> Then he could set up a virtual machine (or even install clang
>natively),
>> and keep writing fixes until the build finishes.
>>
>> I'm curious how many of the above warnings will uncover real bugs,
>and
>> how many will need suppression.
>>
>
>To me, it looks like the diagnostic is correct, and we should
>initialize Status to EFI_SUCCESS at the start of the function. That is
>the only breakage with clang-3.8

If that's the case, do you mind if you send out a patch that fixes it? Or perhaps I can do it by tomorrow -- no access to my machine right now.

Thanks for the report!

Paulo

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.