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

[Vyacheslav Dubeyko <slava@dubeyko.com>]

Hi Sergei,

On Tue, 2014-02-04 at 01:25 +0100, Sergei Antonov wrote:

I have made some additional comments. Please, see below.

On Mon, 2014-02-03 at 19:47 +0100, Sergei Antonov wrote:

[snip]
> 
> Signed-off-by: Sergei Antonov <saproj@gmail.com>
> ---
>  fs/hfsplus/catalog.c     |    7 +++++++
>  fs/hfsplus/hfsplus_fs.h  |    1 +
>  fs/hfsplus/hfsplus_raw.h |    3 ++-
>  fs/hfsplus/inode.c       |    3 +++
>  4 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
> index 968ce41..bc770303 100644
> --- a/fs/hfsplus/catalog.c
> +++ b/fs/hfsplus/catalog.c
> @@ -103,12 +103,15 @@ 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);

I suppose that it will be better to use OR operation during flag set.
What do you think?

>  		folder->id = cpu_to_be32(inode->i_ino);
>  		HFSPLUS_I(inode)->create_date =
>  			folder->create_date =
>  			folder->content_mod_date =
>  			folder->attribute_mod_date =
>  			folder->access_date = hfsp_now2mt();
> +		HFSPLUS_I(inode)->folder_count = 0;

Is it zeroing really necessary? The memset operation doesn't set it earlier?

>  		hfsplus_cat_set_perms(inode, &folder->permissions);
>  		if (inode == sbi->hidden_dir)
>  			/* invisible and namelocked */
> @@ -247,6 +250,8 @@ int hfsplus_create_cat(u32 cnid, struct inode *dir,
>  		goto err1;
>  
>  	dir->i_size++;
> +	if (S_ISDIR(inode->i_mode))
> +		HFSPLUS_I(dir)->folder_count++;

Yes, it needs check of flag here.

>  	dir->i_mtime = dir->i_ctime = CURRENT_TIME_SEC;
>  	hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY);
>  
> @@ -336,6 +341,8 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, struct qstr *str)
>  		goto out;
>  
>  	dir->i_size--;
> +	if (type == HFSPLUS_FOLDER)
> +		HFSPLUS_I(dir)->folder_count--;

Ditto. It needs to check flag here.

>  	dir->i_mtime = dir->i_ctime = CURRENT_TIME_SEC;
>  	hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY);
>  
> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> index 08846425b..2c22cd2 100644
> --- a/fs/hfsplus/hfsplus_fs.h
> +++ b/fs/hfsplus/hfsplus_fs.h
> @@ -242,6 +242,7 @@ 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 */

Why do you place variable here? What consideration have you?

>  	struct list_head open_dir_list;
>  	loff_t phys_size;
>  
> diff --git a/fs/hfsplus/hfsplus_raw.h b/fs/hfsplus/hfsplus_raw.h
> index 8ffb3a8..6529629 100644
> --- a/fs/hfsplus/hfsplus_raw.h
> +++ b/fs/hfsplus/hfsplus_raw.h
> @@ -261,7 +261,7 @@ struct hfsplus_cat_folder {
>  	struct DInfo user_info;
>  	struct DXInfo finder_info;
>  	__be32 text_encoding;
> -	u32 reserved;
> +	__be32 folder_count;	/* Number of subfolders when HFSPLUS_HAS_FOLDER_COUNT is set. Reserved otherwise. */
>  } __packed;
>  
>  /* HFS file info (stolen from hfs.h) */
> @@ -306,6 +306,7 @@ struct hfsplus_cat_file {
>  #define HFSPLUS_FILE_THREAD_EXISTS	0x0002
>  #define HFSPLUS_XATTR_EXISTS		0x0004
>  #define HFSPLUS_ACL_EXISTS		0x0008
> +#define HFSPLUS_HAS_FOLDER_COUNT	0x0010

I am no fully confident that this constant is correct.
Where do you find it? All previous constant is file related, as far as I
can see. Why do you place folder related declaration in this group of
constants?

Thanks,
Vyacheslav Dubeyko.