From mboxrd@z Thu Jan  1 00:00:00 1970
From: Vyacheslav Dubeyko <slava@dubeyko.com>
Subject: Re: [PATCH] hfsplus: add HFSX subfolder count support
Date: Wed, 05 Feb 2014 12:57:49 +0400
Message-ID: <1391590669.2590.28.camel@ubuntu>
References: <1391453261-12131-1-git-send-email-saproj@gmail.com>
	 <A003EBDF-0DA4-46EE-8063-05EC6A72C9D4@dubeyko.com>
	 <A5BBDB2A-6DEE-475C-929F-3F82DD6E8A11@gmail.com>
	 <1391497817.2590.7.camel@ubuntu>
	 <CABikg9xKjnpP9HUyW-6JGimG8ELVpyaXku5kU+0OUFd2fyhu6A@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Cc: "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
To: Sergei Antonov <saproj@gmail.com>
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from gproxy5-pub.mail.unifiedlayer.com ([67.222.38.55]:33381 "HELO
	gproxy5-pub.mail.unifiedlayer.com" rhost-flags-OK-OK-OK-OK)
	by vger.kernel.org with SMTP id S1751017AbaBEI6L (ORCPT
	<rfc822;linux-fsdevel@vger.kernel.org>);
	Wed, 5 Feb 2014 03:58:11 -0500
In-Reply-To: <CABikg9xKjnpP9HUyW-6JGimG8ELVpyaXku5kU+0OUFd2fyhu6A@mail.gmail.com>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

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.