Re: UDF filesystem fixes for grub

[Giuseppe Caizzone]

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,
-- 
Giuseppe Caizzone <address@hidden>

1-udf-large-files.patch
Description: Binary data

1-udf-large-files.changelog
Description: Binary data

2-udf-deleted-files.patch
Description: Binary data

2-udf-deleted-files.changelog
Description: Binary data

3-udf-generic-block-size-v2.patch
Description: Binary data

3-udf-generic-block-size-v2.changelog
Description: Binary data

4-udf-ad-extents-v2.patch
Description: Binary data

4-udf-ad-extents-v2.changelog
Description: Binary data