From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.zytor.com (terminus.zytor.com [65.50.211.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id CE68D21E87965 for ; Fri, 8 Sep 2017 16:32:11 -0700 (PDT) Received: from [192.168.15.2] (177.204.9.54.dynamic.adsl.gvt.net.br [177.204.9.54]) (authenticated bits=0) by mail.zytor.com (8.15.2/8.15.2) with ESMTPSA id v88NUStU007289 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Fri, 8 Sep 2017 16:30:31 -0700 Date: Fri, 08 Sep 2017 20:30:20 -0300 User-Agent: K-9 Mail for Android In-Reply-To: References: <77914c33-7637-e759-02d0-f94e448157ff@redhat.com> <2921e12c-b6af-5b50-a89d-4a41ca7e5e4f@redhat.com> MIME-Version: 1.0 To: Ard Biesheuvel , Laszlo Ersek CC: "edk2-devel@lists.01.org" , Ruiyu Ni , Eric Dong , "Wu, Hao A" , Jordan Justen , Andrew Fish , Liming Gao , Michael D Kinney , Star Zeng From: Paulo Alcantara Message-ID: <52EE680E-FB70-422C-AC8A-3C5FE99A039C@zytor.com> Subject: Re: [PATCH v6 0/6] read-only UDF file system support X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 08 Sep 2017 23:32:12 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Ard, On September 8, 2017 7:17:35 PM GMT-03:00, Ard Biesheuvel wrote: >On 8 September 2017 at 20:40, Laszlo Ersek wrote: >> On 09/08/17 21:21, Ard Biesheuvel wrote: >>> On 8 September 2017 at 19:47, Laszlo Ersek >wrote: >>>> On 09/08/17 14:41, Paulo Alcantara wrote: >>>>> Hi, >>>>> >>>>> This series introduces read-only UDF file system support in EDK2=2E >As >>>>> Laszlo (or Red Hat) seemed to be interested in such support, I'm >posting >>>>> it again after ~3 years=2E >>>>> >>>>> 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=2E >>>>> >>>>> Originally the driver was written to support UDF file systems as >>>>> specified by OSTA Universal Disk Format Specification 2=2E60=2E >However, >>>>> some Windows 10 Enterprise ISO (UDF bridge) images that I tested >>>>> supported a revision of 1=2E02 thus I had to rework the driver a >little >>>>> bit to support such revision as well=2E >>>>> >>>>> v2: >>>>> - Rework to _partially_ support UDF revisions <2=2E60=2E >>>>> - Use existing CDROM_VOLUME_DESCRIPTOR structure defined in >Eltorito=2Eh >>>>> instead of creating another one (UDF_VOLUME_DESCRIPTOR)=2E >>>>> - Fixed UdfDxe to correctly follow UEFI driver model=2E >>>>> - Use HARDDRIVE_DEVICE_PATH instead of a vendor-defined one=2E >>>>> - 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=2E >>>>> - Place MdePkg changes in a separate patch=2E >>>>> v3: >>>>> - Install UDF partition child handles with a Vendor-Defined Media >>>>> Device Path=2E >>>>> - 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=2E >>>>> - Removed leading TAB chars in some source files identified by >>>>> PatchCheck=2Epy tool=2E >>>>> v4: >>>>> - Added missing R-b's=2E >>>>> v5: >>>>> - Fixed OVMF IA32 build=2E >>>>> - 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=2E >>>>> 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=2Ee=2E BufferSize pointer is dereferenced as an UINT64 * and >it's >>>>> followed by 4 bytes that are nonzero)=2E >>>>> >>>>> Repo: https://github=2Ecom/pcacjr/edk2=2Egit >>>>> Branch: udf-fs-v6 >>>>> >>>>> Cc: Laszlo Ersek >>>>> Cc: Jordan Justen >>>>> Cc: Andrew Fish >>>>> Cc: Michael D Kinney >>>>> Cc: Liming Gao >>>>> Cc: Star Zeng >>>>> Cc: Eric Dong >>>>> Cc: Mark Doran >>>>> Cc: Ruiyu Ni >>>>> Cc: hao=2Ea=2Ewu@intel=2Ecom >>>>> Contributed-under: TianoCore Contribution Agreement 1=2E1 >>>>> Signed-off-by: Paulo Alcantara >>>>> --- >>>>> >>>>> 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=2Edsc | 3 +- >>>>> ArmVirtPkg/ArmVirtQemuFvMain=2Efdf=2Einc | 3 +- >>>>> ArmVirtPkg/ArmVirtQemuKernel=2Edsc | 3 +- >>>>> ArmVirtPkg/ArmVirtXen=2Edsc | 3 +- >>>>> ArmVirtPkg/ArmVirtXen=2Efdf | 1 + >>>>> =2E=2E=2E/Universal/Disk/PartitionDxe/Partition=2Ec | 9 += - >>>>> =2E=2E=2E/Universal/Disk/PartitionDxe/Partition=2Eh | 32 += - >>>>> =2E=2E=2E/Universal/Disk/PartitionDxe/PartitionDxe=2Einf | 3 += - >>>>> MdeModulePkg/Universal/Disk/PartitionDxe/Udf=2Ec | 318 +++ >>>>> MdeModulePkg/Universal/Disk/UdfDxe/ComponentName=2Ec | 185 ++ >>>>> MdeModulePkg/Universal/Disk/UdfDxe/File=2Ec | 908 >++++++++ >>>>> MdeModulePkg/Universal/Disk/UdfDxe/FileName=2Ec | 195 ++ >>>>> =2E=2E=2E/Universal/Disk/UdfDxe/FileSystemOperations=2Ec | 2447 >++++++++++++++++++++ >>>>> MdeModulePkg/Universal/Disk/UdfDxe/Udf=2Ec | 344 +++ >>>>> MdeModulePkg/Universal/Disk/UdfDxe/Udf=2Eh | 1244 >++++++++++ >>>>> MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe=2Einf | 66 + >>>>> MdePkg/Include/IndustryStandard/Udf=2Eh | 60 + >>>>> Nt32Pkg/Nt32Pkg=2Edsc | 1 + >>>>> Nt32Pkg/Nt32Pkg=2Efdf | 1 + >>>>> OvmfPkg/OvmfPkgIa32=2Edsc | 1 + >>>>> OvmfPkg/OvmfPkgIa32=2Efdf | 1 + >>>>> OvmfPkg/OvmfPkgIa32X64=2Edsc | 1 + >>>>> OvmfPkg/OvmfPkgIa32X64=2Efdf | 1 + >>>>> OvmfPkg/OvmfPkgX64=2Edsc | 1 + >>>>> OvmfPkg/OvmfPkgX64=2Efdf | 1 + >>>>> 25 files changed, 5821 insertions(+), 11 deletions(-) >>>>> create mode 100644 MdeModulePkg/Universal/Disk/PartitionDxe/Udf=2Ec >>>>> create mode 100644 >MdeModulePkg/Universal/Disk/UdfDxe/ComponentName=2Ec >>>>> create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/File=2Ec >>>>> create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/FileName=2Ec >>>>> create mode 100644 >MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations=2Ec >>>>> create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/Udf=2Ec >>>>> create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/Udf=2Eh >>>>> create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe=2Einf >>>>> create mode 100644 MdePkg/Include/IndustryStandard/Udf=2Eh >>>>> >>>> >>>> Pushed as commit range 7aee391fa3d0=2E=2Eb696c64d4fc3=2E >>>> >>> >>> This code breaks the Clang build: >>> >>> >:937:11: >>> error: variable 'Status' is used uninitialized whenever 'if' >condition >>> is false [-Werror,-Wsometimes-uninitialized] >>> if (ReadFileInfo->FileData =3D=3D NULL) { >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> >:1148:10: >>> note: uninitialized use occurs here >>> return Status; >>> ^~~~~~ >>> >:937:7: >>> note: remove the 'if' if its condition is always true >>> if (ReadFileInfo->FileData =3D=3D NULL) { >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> >:932:16: >>> error: variable 'Status' is used uninitialized whenever 'if' >condition >>> is false [-Werror,-Wsometimes-uninitialized] >>> } else if (ReadFileInfo->Flags =3D=3D READ_FILE_ALLOCATE_AND_READ)= { >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> >:1148:10: >>> note: uninitialized use occurs here >>> return Status; >>> ^~~~~~ >>> >:932:12: >>> note: remove the 'if' if its condition is always true >>> } else if (ReadFileInfo->Flags =3D=3D READ_FILE_ALLOCATE_AND_READ)= { >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> >:930:9: >>> error: variable 'Status' is used uninitialized whenever 'if' >condition >>> is true [-Werror,-Wsometimes-uninitialized] >>> if (ReadFileInfo->Flags =3D=3D READ_FILE_GET_FILESIZE) { >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> >:1148:10: >>> note: uninitialized use occurs here >>> return Status; >>> ^~~~~~ >>> >:930:5: >>> note: remove the 'if' if its condition is always false >>> if (ReadFileInfo->Flags =3D=3D READ_FILE_GET_FILESIZE) { >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> >:869:33: >>> note: initialize the variable 'Status' to silence this warning >>> EFI_STATUS Status; >>> ^ >>> =3D 0 >>> 3 errors generated=2E >> >> Thanks for the report -- this was sort of expected, given the size of >> the driver=2E My test builds for IA32/X64/ARM/AARCH64 with >gcc-4=2E8/GCC48 >> and gcc-6=2E1=2E1/GCC5 didn't catch the above=2E >> >> I think if these issues were fixed, the build wouldn't complete >> immediately; there are likely other instances=2E >> >> 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=2E >> >> I'm curious how many of the above warnings will uncover real bugs, >and >> how many will need suppression=2E >> > >To me, it looks like the diagnostic is correct, and we should >initialize Status to EFI_SUCCESS at the start of the function=2E That is >the only breakage with clang-3=2E8 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=2E Thanks for the report! Paulo --=20 Sent from my Android device with K-9 Mail=2E Please excuse my brevity=2E