Re: UDF filesystem fixes for grub

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6939 bytes --]

All patches committed
Thanks
On 11/11/2010 09:56 AM, Giuseppe Caizzone wrote:
> 2010/11/6 Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com>:
>   
>> 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
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>   


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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]