On 8 September 2017 at 20:40, Laszlo Ersek <[email protected]> wrote: > On 09/08/17 21:21, Ard Biesheuvel wrote: >> On 8 September 2017 at 19:47, Laszlo Ersek <[email protected]> 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 <[email protected]> >>>> Cc: Jordan Justen <[email protected]> >>>> Cc: Andrew Fish <[email protected]> >>>> Cc: Michael D Kinney <[email protected]> >>>> Cc: Liming Gao <[email protected]> >>>> Cc: Star Zeng <[email protected]> >>>> Cc: Eric Dong <[email protected]> >>>> Cc: Mark Doran <[email protected]> >>>> Cc: Ruiyu Ni <[email protected]> >>>> Cc: [email protected] >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Paulo Alcantara <[email protected]> >>>> --- >>>> >>>> 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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
