Re: [PATCH v3 1/2] MdePkg: Add UDF volume structure definitions

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

Paulo,
Comments below:

#define UDF_TAG_ID(_Tag) \
  (UDF_VOLUME_DESCRIPTOR_ID)((_Tag)->TagIdentifier)
1. I prefer to remove the UDF_TAG_ID macro. Adding type-cast to get TAG_ID is very straightforward.
 
#define UDF_LVD_REVISION(_Lv) \
  *(UINT16 *)(UINTN)(_Lv)->DomainIdentifier.IdentifierSuffix
typedef struct {
  UINT8           Flags;
  UINT8           Identifier[23];
  UINT8           IdentifierSuffix[8];
} UDF_ENTITY_ID;
2. Entity structure is defined by ECMA-167 spec and re-used by UDF spec.
     I think since we are creating UDF structure in udf.h, how about using the below
     structure layout and removing the UDF_LVD_REVISION macro:
     Typedef struct {
       UINT8           Flags;
       UINT8           Identifier[23];
       UINT16         UdfRevision;
       UINT8           DomainFlags;
       UINT8           Reserved[5];
    } UDF_ENTITY_ID;

I am not sure if there are other structures that are defined in ECMA spec and
re-used by UDF spec. I think we can apply the similar rules to those structures
as well.

Thanks/Ray

> -----Original Message-----
> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
> Sent: Thursday, September 21, 2017 2:16 AM
> To: edk2-devel@lists.01.org
> Cc: Paulo Alcantara <pcacjr@zytor.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Laszlo
> Ersek <lersek@redhat.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: [PATCH v3 1/2] MdePkg: Add UDF volume structure definitions
> 
> This patch adds a few more UDF volume structures in order to detect an UDF
> file system which is supported by current EDK2 UDF file system
> implementation in Partition driver.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
> ---
>  MdePkg/Include/IndustryStandard/Udf.h | 63 ++++++++++++++++++--
>  1 file changed, 59 insertions(+), 4 deletions(-)
> 
> diff --git a/MdePkg/Include/IndustryStandard/Udf.h
> b/MdePkg/Include/IndustryStandard/Udf.h
> index 0febb4bcda..002e87e150 100644
> --- a/MdePkg/Include/IndustryStandard/Udf.h
> +++ b/MdePkg/Include/IndustryStandard/Udf.h
> @@ -24,11 +24,28 @@
>  #define UDF_LOGICAL_SECTOR_SIZE   ((UINT64)(1ULL <<
> UDF_LOGICAL_SECTOR_SHIFT))
>  #define UDF_VRS_START_OFFSET      ((UINT64)(16ULL <<
> UDF_LOGICAL_SECTOR_SHIFT))
> 
> -#define _GET_TAG_ID(_Pointer) \
> -  (((UDF_DESCRIPTOR_TAG *)(_Pointer))->TagIdentifier)
> +typedef enum {
> +  UdfPrimaryVolumeDescriptor = 1,
> +  UdfAnchorVolumeDescriptorPointer = 2,
> +  UdfVolumeDescriptorPointer = 3,
> +  UdfImplemenationUseVolumeDescriptor = 4,
> +  UdfPartitionDescriptor = 5,
> +  UdfLogicalVolumeDescriptor = 6,
> +  UdfUnallocatedSpaceDescriptor = 7,
> +  UdfTerminatingDescriptor = 8,
> +  UdfLogicalVolumeIntegrityDescriptor = 9,
> +  UdfFileSetDescriptor = 256,
> +  UdfFileIdentifierDescriptor = 257,
> +  UdfAllocationExtentDescriptor = 258,
> +  UdfFileEntry = 261,
> +  UdfExtendedFileEntry = 266,
> +} UDF_VOLUME_DESCRIPTOR_ID;
> 
> -#define IS_AVDP(_Pointer) \
> -  ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 2))
> +#define UDF_TAG_ID(_Tag) \
> +  (UDF_VOLUME_DESCRIPTOR_ID)((_Tag)->TagIdentifier)
> +
> +#define UDF_LVD_REVISION(_Lv) \
> +  *(UINT16 *)(UINTN)(_Lv)->DomainIdentifier.IdentifierSuffix
> 
>  #pragma pack(1)
> 
> @@ -49,12 +66,50 @@ typedef struct {
>  } UDF_EXTENT_AD;
> 
>  typedef struct {
> +  UINT8           CharacterSetType;
> +  UINT8           CharacterSetInfo[63];
> +} UDF_CHAR_SPEC;
> +
> +typedef struct {
> +  UINT8           Flags;
> +  UINT8           Identifier[23];
> +  UINT8           IdentifierSuffix[8];
> +} UDF_ENTITY_ID;
> +
> +typedef struct {
> +  UINT32        LogicalBlockNumber;
> +  UINT16        PartitionReferenceNumber;
> +} UDF_LB_ADDR;
> +
> +typedef struct {
> +  UINT32                           ExtentLength;
> +  UDF_LB_ADDR                      ExtentLocation;
> +  UINT8                            ImplementationUse[6];
> +} UDF_LONG_ALLOCATION_DESCRIPTOR;
> +
> +typedef struct {
>    UDF_DESCRIPTOR_TAG  DescriptorTag;
>    UDF_EXTENT_AD       MainVolumeDescriptorSequenceExtent;
>    UDF_EXTENT_AD       ReserveVolumeDescriptorSequenceExtent;
>    UINT8               Reserved[480];
>  } UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER;
> 
> +typedef struct {
> +  UDF_DESCRIPTOR_TAG              DescriptorTag;
> +  UINT32                          VolumeDescriptorSequenceNumber;
> +  UDF_CHAR_SPEC                   DescriptorCharacterSet;
> +  UINT8                           LogicalVolumeIdentifier[128];
> +  UINT32                          LogicalBlockSize;
> +  UDF_ENTITY_ID                   DomainIdentifier;
> +  UDF_LONG_ALLOCATION_DESCRIPTOR  LogicalVolumeContentsUse;
> +  UINT32                          MapTableLength;
> +  UINT32                          NumberOfPartitionMaps;
> +  UDF_ENTITY_ID                   ImplementationIdentifier;
> +  UINT8                           ImplementationUse[128];
> +  UDF_EXTENT_AD                   IntegritySequenceExtent;
> +  UINT8                           PartitionMaps[6];
> +} UDF_LOGICAL_VOLUME_DESCRIPTOR;
> +
>  #pragma pack()
> 
>  #endif
> --
> 2.11.0