Re: UDF filesystem fixes for grub

[Vladimir 'φ-coder/phcoder' Serbinenko]

All patches committed
Thanks
On 11/11/2010 09:56 AM, Giuseppe Caizzone wrote:
> 2010/11/6 Vladimir 'φ-coder/phcoder' Serbinenko <address@hidden>:
>   
>> On 11/02/2010 02:10 PM, Giuseppe Caizzone wrote:
>>     
>>> Hello,
>>>
>>> I've made 4 small patches that improve the UDF filesystem support in
>>> grub. With these changes, I've been able to read any regular file in
>>> UDF filesystems created by both Linux and Windows on both optical
>>> storage and USB sticks.
>>> [...]
>>> 1/4) Support for large (> 2GiB) files
>>> [...]
>>> The patch just changes an int and a grub_uint32_t to be grub_off_t.
>>>
>>>
>>>       
>> Good patch. Can you write the ChangeLog entry for it?
>>     
> OK - I've never written one, so I tried to mimic grub's.
>
>       Support reading files larger than 2 GiB.
>
>       * grub-core/fs/udf.c (grub_udf_iterate_dir): change type of variable
>       "offset" from grub_uint32_t to grub_off_t.
>       (grub_udf_read_file): change type of parameter "pos" from int to
>       grub_off_t.
>
>   
>>> 2/4) Support for deleted files
>>> [...]
>>> The patch just skips directory entries with the "deleted" bit set, in
>>> grub_udf_iterate_dir().
>>>
>>>
>>>       
>> This one is good too. Changelog entry?
>>     
>       Properly handle deleted files.
>
>       * grub-core/fs/udf.c (grub_udf_iterate_dir): Skip directory entries
>       whose "characteristics" field has the bit GRUB_UDF_FID_CHAR_DELETED
>       set.
>
>   
>>> 3/4) Support for a generic block size
>>> [...]
>>> The patch repeats the superblock search in grub_udf_mount() for 512,
>>> 1024, 2048 and 4096 block sizes, stopping if it finds a valid one AND
>>> the sector offset recorded in that superblock matches the detected
>>> superblock offset. So for instance it won't misdetect a superblock
>>> located at sector 256 with blocksize 2048 for a superblock located at
>>> sector 512 with blocksize 1024.
>>> The log2(blocksize/512) is then stored in a new field called "lbshift"
>>> in struct grub_udf_data, which gets used instead of the previous
>>> constant GRUB_UDF_LOG2_BLKSZ, while the constant GRUB_UDF_BLKSZ gets
>>> replaced with the lvd.bsize field already present in struct
>>> grub_udf_data.
>>>
>>>
>>>       
>> +  lbshift = 0;
>> +  block = 0;
>> +  while (lbshift <= 3)
>> +    {
>> For loop is way more appropriate than a while loop
>>     
> Changed it into a for loop.
>
>   
>> +  while (lbshift <= 3)
>> +    {
>> +      sblklist = sblocklist;
>> +      while (*sblklist)
>> +        {
>> Same here.
>>     
> Changed that too.
>
> I also made another change since the previous version of the patch: I
> moved the VRS check after the AVDP search, because it needs to know
> the logical block size. I originally thought that it wasn't necessary,
> because the VRS is made up of records with a fixed size of 2048 bytes,
> but the catch is that the standard says that each record must start on
> a new sector, so if block size is > 2048, one has to skip more bytes
> than 2048 to get to the next record. Tested it with mkudffs because I
> have no medium with an actual block size of 4096.
>
>   
>> ChangeLog entry?
>>     
>       Add generic logical block size support.
>
>       * grub-core/fs/udf.c (GRUB_UDF_LOG2_BLKSIZE): Removed macro.
>       (GRUB_UDF_BLKSZ): Removed macro.
>       (struct grub_udf_data): New field "lbshift" to hold the logical block
>       size of the file system in log2 format.
>       (grub_udf_read_icb): Replace usage of macro GRUB_UDF_LOG2_BLKSZ with
>       field lbshift from struct grub_udf_data.
>       (grub_udf_read_file): Likewise.
>       (grub_udf_read_block): Replace usage of macro GRUB_UDF_BLKSZ with
>       field "lvd.bsize" from struct grub_udf_data.
>       Replace division with right shift.
>       (sblocklist): Change type to unsigned.
>       (grub_udf_mount): Change type of "sblklist" to unsigned.
>       New variable "vblock" to be used during VRS recognition.
>       New variable "lbshift" to hold the detected logical block size.
>       Move AVDP search before VRS recognition, because the latter requires
>       knowledge of the logical block size, which is detected during the
>       former.
>       Detect and validate logical block size during AVDP search, adding
>       support for block sizes 512, 1024 and 4096.
>       Make VRS recognition independent of block size.
>       Replace usages of macro GRUB_UDF_LOG2_BLKSZ with variable lbshift.
>
>   
>>> 4/4) Support for allocation descriptor extensions
>>> [...]
>>> The patch checks, in grub_udf_read_block(), whether the "allocation
>>> descriptor type", previously ignored, is 3, and in this case, it loads
>>> the referenced block, checks that it's a valid allocation extension
>>> descriptor, and sets the "ad" pointer (and the "len" limit) to
>>> continue the iteration from the allocation descriptors contained in
>>> that block.
>>>
>>> This last one is the only patch which actually contains a proper new
>>> block of code, and it allocates a big structure on the stack (it's a
>>> sector, so it's up to 4K big): I don't know if this is OK, or if
>>> perhaps I should use grub_malloc() instead. Also, the patch depends on
>>> the previous "generic block size" patch.
>>>
>>>
>>>       
>> +  char buf[U32 (node->data->lvd.bsize)];
>> Will segfault if bsize is too big. You need to allocate on heap
>>     
> Done.
>
>   
>> -  else
>> +  else if (U16 (node->fe.tag.tag_ident == GRUB_UDF_TAG_IDENT_EFE))
>>     {
>> Parenthesis are wrong.
>>     
> While I was at it, I changed the 'if-else if' to a switch.
>
>   
>> +             grub_disk_addr_t sec = grub_udf_get_block(node->data,
>> +                                                       node->part_ref,
>> +                                                       ad->position);
>> ad->position isn't byte-swapped.
>>     
> grub_udf_get_block() byte-swaps it itself, so I must not byte-swap it before.
>
>   
>> ChangeLog entry?
>>     
>       Add support for allocation extent descriptors, needed on fragmented
>       volumes.
>
>       * grub-core/fs/udf.c (grub_udf_aed): New struct.
>       (grub_udf_read_block): Change type of variable "len" to grub_ssize_t.
>       Add sanity check for file entry type.
>       Replace divisions with right shifts.
>       Check the upper bits of the allocation descriptor's length and honour
>       them by loading an allocation extent descriptor if they indicate so.
>       Change all failure return paths to free the allocation extent
>       descriptor if it was allocated.
>
> I'm attaching the full patch set (including the unchanged ones for
> convenience), and also the change log entries in attachment form in
> case gmail tampers with whitespace.
>
> Thank you,
>   
>
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/grub-devel
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko

signature.asc
Description: OpenPGP digital signature