Re: [PATCH] hfsplus: add HFSX subfolder count support

[Vyacheslav Dubeyko <slava@dubeyko.com>]

Hi Sergei,

Thank you for the second version of the patch. It looks much better. But
it is easier to answer on the patch in the e-mail body. Anyway, you
should send the next (and, maybe, final) version of the patch in the
e-mail body.

>> This patch adds support for HFSX 'HasFolderCount' flag and a corresponding 'folderCount' field in folder records. (For reference see HFS_FOLDERCOUNT and kHFSHasFolderCountBit/kHFSHasFolderCountMask in Apple's source code.)

Please, use 80 symbol on the line rule and for patch's comment too.

>> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
>> index 08846425b..e1911fc 100644
>> --- a/fs/hfsplus/hfsplus_fs.h
>> +++ b/fs/hfsplus/hfsplus_fs.h
>> @@ -242,6 +242,8 @@ struct hfsplus_inode_info {
>> 	 */
>> 	sector_t fs_blocks;
>> 	u8 userflags;		/* BSD user file flags */
>> +	u32 folder_count;	/* Subfolder count if
>> +				 * HFSPLUS_HAS_FOLDER_COUNT is set */

I suggest to change comment here: "Subfolders count (HFSX only)".
And it makes sense to add more detailed comment for
HFSPLUS_HAS_FOLDER_COUNT flag.

>> diff --git a/fs/hfsplus/hfsplus_raw.h b/fs/hfsplus/hfsplus_raw.h
>> index 8ffb3a8..61aeea8 100644
>> --- a/fs/hfsplus/hfsplus_raw.h
>> +++ b/fs/hfsplus/hfsplus_raw.h
>> @@ -261,7 +261,10 @@ struct hfsplus_cat_folder {
>> 	struct DInfo user_info;
>> 	struct DXInfo finder_info;
>> 	__be32 text_encoding;
>> -	u32 reserved;
>> +
>> +	/* Number of subfolders when HFSPLUS_HAS_FOLDER_COUNT flag is set.
>> +	 * Reserved otherwise. */
>> +	__be32 folder_count;

What about shorter name as "subfolders" for both fields in struct
hfsplus_inode_info and in struct hfsplus_cat_folder? I suppose it will
be more informative and correct.

I suggest to change comment here: "Subfolders count (HFSX only).
Reserved otherwise.". And it makes sense to add more detailed comment
for HFSPLUS_HAS_FOLDER_COUNT flag.

>> diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
>> index 968ce41..04228d6 100644
>> --- a/fs/hfsplus/catalog.c
>> +++ b/fs/hfsplus/catalog.c
>> @@ -103,6 +103,8 @@ static int hfsplus_cat_build_record(hfsplus_cat_entry *entry,
>> 		folder = &entry->folder;
>> 		memset(folder, 0, sizeof(*folder));
>> 		folder->type = cpu_to_be16(HFSPLUS_FOLDER);
>>+		if (test_bit(HFSPLUS_SB_HFSX, &sbi->flags))
>>+			folder->flags |= cpu_to_be16(HFSPLUS_HAS_FOLDER_COUNT);

Are you sure that on every HFSX volume folder records in CatalogFile
have HFSPLUS_HAS_FOLDER_COUNT flag as set? Maybe it can depend from some
flag or hints in superblock? I simply want to be sure that you tested
all key situations.

Thanks,
Vyacheslav Dubeyko.