[v6] read-only UDF file system support

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

Posted by Paulo Alcantara 6 years, 7 months ago

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

-- 
2.11.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Laszlo Ersek 6 years, 7 months ago

Ray,

On 09/08/17 14:41, Paulo Alcantara wrote:

> 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

The v5-v6 diff is as follows:

> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> index 2dbcff0be4a3..8b9339567f8e 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> @@ -325,8 +325,9 @@ UdfRead (
>    UDF_FILE_IDENTIFIER_DESCRIPTOR  *NewFileIdentifierDesc;
>    VOID                            *NewFileEntryData;
>    CHAR16                          FileName[UDF_FILENAME_LENGTH] = { 0 };
>    UINT64                          FileSize;
> +  UINT64                          BufferSizeUint64;
>
>    OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
>
>    if (This == NULL || BufferSize == NULL || (*BufferSize != 0 &&
> @@ -363,18 +364,22 @@ UdfRead (
>        Status = EFI_SUCCESS;
>        goto Done;
>      }
>
> +    BufferSizeUint64 = *BufferSize;
> +
>      Status = ReadFileData (
>        BlockIo,
>        DiskIo,
>        Volume,
>        Parent,
>        PrivFileData->FileSize,
>        &PrivFileData->FilePosition,
>        Buffer,
> -      (UINT64 *)(UINTN)BufferSize
> +      &BufferSizeUint64
>        );
> +    ASSERT (BufferSizeUint64 <= MAX_UINTN);
> +    *BufferSize = (UINTN)BufferSizeUint64;
>    } else if (IS_FID_DIRECTORY_FILE (Parent->FileIdentifierDesc)) {
>      if (ReadDirInfo->FidOffset == 0 && PrivFileData->FilePosition > 0) {
>        Status = EFI_DEVICE_ERROR;
>        *BufferSize = 0;

It looks OK to me, and it builds fine for IA32, X64, ARM and AARCH64:

  Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/UdfDxe.efi
  Build/ArmVirtQemu-ARM/DEBUG_GCC5/ARM/UdfDxe.efi
  Build/OvmfIa32/NOOPT_GCC48/IA32/UdfDxe.efi
  Build/OvmfX64/NOOPT_GCC48/X64/UdfDxe.efi

Green light from your side?

Paulo: you forgot to pick up Ray's R-b for patches #4 and #5, from his
v5 response
<http://mid.mail-archive.com/734D49CCEBEEF84792F5B80ED585239D5BA282B7@SHSMSX104.ccr.corp.intel.com>
-- it was for the entire series.

But, I'll apply that for you.

Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Paulo Alcantara 6 years, 7 months ago

Hi,

On 08/09/2017 10:25, Laszlo Ersek wrote:
> Ray,
> 
> On 09/08/17 14:41, Paulo Alcantara wrote:
> 
>> 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
> 
> The v5-v6 diff is as follows:
> 
>> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
>> index 2dbcff0be4a3..8b9339567f8e 100644
>> --- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
>> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
>> @@ -325,8 +325,9 @@ UdfRead (
>>     UDF_FILE_IDENTIFIER_DESCRIPTOR  *NewFileIdentifierDesc;
>>     VOID                            *NewFileEntryData;
>>     CHAR16                          FileName[UDF_FILENAME_LENGTH] = { 0 };
>>     UINT64                          FileSize;
>> +  UINT64                          BufferSizeUint64;
>>
>>     OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
>>
>>     if (This == NULL || BufferSize == NULL || (*BufferSize != 0 &&
>> @@ -363,18 +364,22 @@ UdfRead (
>>         Status = EFI_SUCCESS;
>>         goto Done;
>>       }
>>
>> +    BufferSizeUint64 = *BufferSize;
>> +
>>       Status = ReadFileData (
>>         BlockIo,
>>         DiskIo,
>>         Volume,
>>         Parent,
>>         PrivFileData->FileSize,
>>         &PrivFileData->FilePosition,
>>         Buffer,
>> -      (UINT64 *)(UINTN)BufferSize
>> +      &BufferSizeUint64
>>         );
>> +    ASSERT (BufferSizeUint64 <= MAX_UINTN);
>> +    *BufferSize = (UINTN)BufferSizeUint64;
>>     } else if (IS_FID_DIRECTORY_FILE (Parent->FileIdentifierDesc)) {
>>       if (ReadDirInfo->FidOffset == 0 && PrivFileData->FilePosition > 0) {
>>         Status = EFI_DEVICE_ERROR;
>>         *BufferSize = 0;
> 
> It looks OK to me, and it builds fine for IA32, X64, ARM and AARCH64:
> 
>    Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/UdfDxe.efi
>    Build/ArmVirtQemu-ARM/DEBUG_GCC5/ARM/UdfDxe.efi
>    Build/OvmfIa32/NOOPT_GCC48/IA32/UdfDxe.efi
>    Build/OvmfX64/NOOPT_GCC48/X64/UdfDxe.efi
> 
> Green light from your side?

Yes - at least from what I've been testing :-)

> 
> Paulo: you forgot to pick up Ray's R-b for patches #4 and #5, from his
> v5 response
> <http://mid.mail-archive.com/734D49CCEBEEF84792F5B80ED585239D5BA282B7@SHSMSX104.ccr.corp.intel.com>
> -- it was for the entire series.

D'oh - I only considered the Mde*Pkg and Nt32Pkg R-b's. Sorry.

> But, I'll apply that for you.

Thank you very much!

Paulo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Ni, Ruiyu 6 years, 7 months ago

Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Friday, September 8, 2017 9:26 PM
To: Paulo Alcantara <pcacjr@zytor.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
Cc: Dong, Eric <eric.dong@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org; Andrew Fish <afish@apple.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH v6 0/6] read-only UDF file system support

Ray,

On 09/08/17 14:41, Paulo Alcantara wrote:

> 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

The v5-v6 diff is as follows:

> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c 
> b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> index 2dbcff0be4a3..8b9339567f8e 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> @@ -325,8 +325,9 @@ UdfRead (
>    UDF_FILE_IDENTIFIER_DESCRIPTOR  *NewFileIdentifierDesc;
>    VOID                            *NewFileEntryData;
>    CHAR16                          FileName[UDF_FILENAME_LENGTH] = { 0 };
>    UINT64                          FileSize;
> +  UINT64                          BufferSizeUint64;
>
>    OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
>
>    if (This == NULL || BufferSize == NULL || (*BufferSize != 0 && @@ 
> -363,18 +364,22 @@ UdfRead (
>        Status = EFI_SUCCESS;
>        goto Done;
>      }
>
> +    BufferSizeUint64 = *BufferSize;
> +
>      Status = ReadFileData (
>        BlockIo,
>        DiskIo,
>        Volume,
>        Parent,
>        PrivFileData->FileSize,
>        &PrivFileData->FilePosition,
>        Buffer,
> -      (UINT64 *)(UINTN)BufferSize
> +      &BufferSizeUint64
>        );
> +    ASSERT (BufferSizeUint64 <= MAX_UINTN);
> +    *BufferSize = (UINTN)BufferSizeUint64;
>    } else if (IS_FID_DIRECTORY_FILE (Parent->FileIdentifierDesc)) {
>      if (ReadDirInfo->FidOffset == 0 && PrivFileData->FilePosition > 0) {
>        Status = EFI_DEVICE_ERROR;
>        *BufferSize = 0;

It looks OK to me, and it builds fine for IA32, X64, ARM and AARCH64:

  Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/UdfDxe.efi
  Build/ArmVirtQemu-ARM/DEBUG_GCC5/ARM/UdfDxe.efi
  Build/OvmfIa32/NOOPT_GCC48/IA32/UdfDxe.efi
  Build/OvmfX64/NOOPT_GCC48/X64/UdfDxe.efi

Green light from your side?

Paulo: you forgot to pick up Ray's R-b for patches #4 and #5, from his
v5 response
<http://mid.mail-archive.com/734D49CCEBEEF84792F5B80ED585239D5BA282B7@SHSMSX104.ccr.corp.intel.com>
-- it was for the entire series.

But, I'll apply that for you.

Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Laszlo Ersek 6 years, 7 months ago

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.

Thank you for the contribution, Paulo, and everyone else for the feedback!

Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Ard Biesheuvel 6 years, 7 months ago

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,
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Laszlo Ersek 6 years, 7 months ago

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Ard Biesheuvel 6 years, 7 months ago

On 8 September 2017 at 20:40, Laszlo Ersek <lersek@redhat.com> wrote:
> 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.
>

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
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Paulo Alcantara 6 years, 7 months ago


Ard,

On September 8, 2017 7:17:35 PM GMT-03:00, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>On 8 September 2017 at 20:40, Laszlo Ersek <lersek@redhat.com> wrote:
>> 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.
>>
>
>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
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Laszlo Ersek 6 years, 7 months ago

On 09/09/17 01:30, Paulo Alcantara wrote:
> 
> 
> Ard,
> 
> On September 8, 2017 7:17:35 PM GMT-03:00, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 8 September 2017 at 20:40, Laszlo Ersek <lersek@redhat.com> wrote:
>>> 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.
>>>
>>
>> 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.

I managed to install llvm+clang, and I can reproduce the build error. I
agree that clang's report is valid. If (RecordingFlags==INLINE_DATA),
and we don't run into an AllocatePool() failure, then we successfully
complete the ReadFile() operation, but fail to set Status to anything,
before returning from the function.

Technically, setting Status to EFI_SUCCESS at the top of the function
would sove this problem. However, I don't think that it would be good
style. The INLINE_DATA branch is quite far from the location that clang
suggests to pre-set the variable: we don't initialize locals in edk2, so
the earliest setting would be on line 886, but the *end* of the
INLINE_DATA case -- when we know for sure that we succeeded --, i.e. the
"break" statement, is at line 960. It depends on individual settings, of
course, but when I move the "break" to the bottom of my screen, I don't
see the Status setting at the top of the screen.

The rest of the function seems to set Status (or jumps right after
setting it) whenever it decides it has succeeded or failed. For example,
on line 1121, it sets Status explicitly to EFI_SUCCESS, even though at
that point Status is bound to be EFI_SUCCESS already. So I think we
should stick with the localized Status setting.

Another thing I'd find confusing: under INLINE_DATA, if AllocatePool()
failed, we'd return EFI_OUT_OF_RESOURCES directly, but in case of
success -- AllocatePool() succeeded, or we didn't even need the extra
memory --, we'd do nothing explicit, just rely on the default Status
(EFI_SUCCESS). To me that's hard to understand.

I'll post a patch that sets EFI_STATUS just before the "break", at the
end of INLINE_DATA.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel