Ard,
On September 8, 2017 7:17:35 PM GMT-03:00, Ard Biesheuvel <[email protected]> wrote: >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 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. _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
