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

[Laszlo Ersek <lersek@redhat.com>]

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.

Thanks!
Laszlo