From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x235.google.com (mail-io0-x235.google.com [IPv6:2607:f8b0:4001:c06::235]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id E1CED21A143F5 for ; Fri, 8 Sep 2017 15:14:44 -0700 (PDT) Received: by mail-io0-x235.google.com with SMTP id y123so8462588iod.0 for ; Fri, 08 Sep 2017 15:17:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=QfWdnYHOZw8h6bbGph7dDHfPxwet6Xj/WpNlYki1doo=; b=aiOcKUTSjIr6fX/9SxGaf6GGQHK/fm+RIIfkz+6qptO00XojnsLOfI/r3xKLkD1prA MDKR7fVNPmPQazB79wJtSHPO3jZuv0aQBGp7PG5Glsm3SQMwcAoOs+N/ZNtkM9QvVrh2 vQ/YE7d7orfjXIKhPbXTVqy9BFQbN4bQwmk10= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=QfWdnYHOZw8h6bbGph7dDHfPxwet6Xj/WpNlYki1doo=; b=ttroPf5EaUFWYWBXkLjILNvD8cHFeoafv1hwSyzur6q3QxikS9+/Bu9FXYs4vi2tni NxdD/4MpwrOQrrQGYYWtJVGaNNAU0900xmQNkUmz7CeaWEsN0F6U6A1zovq1Elr5/ZLU 7rx8eDgTgdCN1G0DMJ8Sa2j5yzdnazFhIGAw0TbAhCq06zaqnTK7pwyaHnqT2SgH/xSF 7RChIFYzkCusqwEIODvCPZIkv4wU0jDaTJsx8GPr9T2uEWEgR4slmpudeTTXCfNwvidw CfBJgb21lN4sKmGJ0twrlFYWYMIcUwsZKKw4/ryklNJI2JjwKcbSvYXSOAsbMWJwvPiV Q2Pg== X-Gm-Message-State: AHPjjUgv8yU7eAXRWOMJ1Rwo6NZQk5H/uFRs6So2BVhvy26dPLW1+sIw CVTXuASeSM21w8PF8rpAohPduBek3uKP X-Google-Smtp-Source: ADKCNb52xqCST4OMuo1E7mX1vyBP8NUCOo9sIn2xN6sIz/D/F55xE2/vY7CvEaWeoL+1P86W2w4o8+CWOk/TFLv1hXI= X-Received: by 10.107.15.141 with SMTP id 13mr5544662iop.141.1504909056854; Fri, 08 Sep 2017 15:17:36 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.162.1 with HTTP; Fri, 8 Sep 2017 15:17:35 -0700 (PDT) In-Reply-To: <2921e12c-b6af-5b50-a89d-4a41ca7e5e4f@redhat.com> References: <77914c33-7637-e759-02d0-f94e448157ff@redhat.com> <2921e12c-b6af-5b50-a89d-4a41ca7e5e4f@redhat.com> From: Ard Biesheuvel Date: Fri, 8 Sep 2017 23:17:35 +0100 Message-ID: To: Laszlo Ersek Cc: Paulo Alcantara , "edk2-devel@lists.01.org" , Ruiyu Ni , Eric Dong , "Wu, Hao A" , Jordan Justen , Andrew Fish , Liming Gao , Michael D Kinney , Star Zeng 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 22:14:45 -0000 Content-Type: text/plain; charset="UTF-8" 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. 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 >>>> 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.a.wu@intel.com >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> 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.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: >> >> :937:11: >> error: variable 'Status' is used uninitialized whenever 'if' condition >> is false [-Werror,-Wsometimes-uninitialized] >> if (ReadFileInfo->FileData == 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 == NULL) { >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> :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) { >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> :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 == READ_FILE_ALLOCATE_AND_READ) { >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> :930:9: >> error: variable 'Status' is used uninitialized whenever 'if' condition >> is true [-Werror,-Wsometimes-uninitialized] >> if (ReadFileInfo->Flags == 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 == READ_FILE_GET_FILESIZE) { >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> :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