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

[Paulo Alcantara <pcacjr@zytor.com>]

Ruiyu,

On 9/21/2017 11:50 PM, Ni, Ruiyu wrote:
> 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.

Right. I'll remove it.

>   
> #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;

Much better! Thanks for looking into that.

The UDF 2.60 specification says:

 > UDF classifies Entity Identifiers into 4 separate types. Each type 
has > its own Suffix Type for the Identifier Suffix field. The 4 types are:
 >
 >  Domain Entity Identifiers         with a Domain Identifier Suffix
 >  UDF Entity Identifiers            with a UDF Identifier Suffix
 >  Implementation Entity Identifiers with an Implementation Identifier 
Suffix
 >  Application Entity Identifiers    with an Application Identifier Suffix

Given that, I think an union would be more appropriate.

E.g.,

typedef struct {
   UINT8           Flags;
   UINT8           Identifier[23];
   union {
     struct {
       UINT16      UdfRevision;
       UINT8       DomainFlags;
       UINT8       Reversed[5];
     } DomainIdentifier;
     struct {
       UINT16      UdfRevision;
       UINT8       OSClass;
       UINT8       OSIdentifier;
       UINT8       Reserved[4];
     } EntityIdentifier;
     struct {
       UINT8       OSClass;
       UINT8       OSIdentifier;
       UINT8       ImplementationUseArea[6];
     } ImplementationEntityIdentifier;
     struct {
       UINT8       ApplicationUseArea[8];
     } ApplicationEntityIdentifier;
     UINT8         IdentifierSuffix[8];
   };
} UDF_ENTITY_ID;

Do you agree with me? Or that wouldn't be necessary?

Thanks!
Paulo

> 
> 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
> 
>