Hi Sergey,
On Feb 3, 2014, at 9:47 PM, Sergei Antonov wrote:
> 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.)
>
> Ignoring subfolder count leads to fs errors found by Mac:
> ...
> Checking catalog hierarchy.
> HasFolderCount flag needs to be set (id = 105)
> (It should be 0x10 instead of 0)
> Incorrect folder count in a directory (id = 2)
> (It should be 7 instead of 6)
> ...
>
> Steps to reproduce:
> Format with "newfs_hfs -s /dev/diskXXX".
> Mount in Linux.
> Create a new directory in root.
> Unmount.
> Run "fsck_hfs /dev/diskXXX".
>
> Signed-off-by: Sergei Antonov <saproj@xxxxxxxxx>
Thank you for the patch. Please, find my remarks below.
I am going to review your patch more deeper tomorrow.
> ---
> 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);
> 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;
> 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++;
> 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--;
> 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 */
I am afraid this line breaks kernel code style. I mean that
comment is too long for one line and it hasn't white space
between code and comment.
> 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. */
Ditto. Too long comment for one line.
> } __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
>
> /* HFS+ catalog thread (part of a cat_entry) */
> struct hfsplus_cat_thread {
> diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
> index fa929f3..7d18221 100644
> --- a/fs/hfsplus/inode.c
> +++ b/fs/hfsplus/inode.c
> @@ -494,6 +494,7 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd)
> inode->i_ctime = hfsp_mt2ut(folder->attribute_mod_date);
> HFSPLUS_I(inode)->create_date = folder->create_date;
> HFSPLUS_I(inode)->fs_blocks = 0;
> + HFSPLUS_I(inode)->folder_count = be32_to_cpu(folder->folder_count);
You check HFSPLUS_HAS_FOLDER_COUNT flag below in hfsplus_cat_write_inode()
but you don't check here. Are you sure that it is right way?
Thanks,
Vyacheslav Dubeyko.
> inode->i_op = &hfsplus_dir_inode_operations;
> inode->i_fop = &hfsplus_dir_operations;
> } else if (type == HFSPLUS_FILE) {
> @@ -566,6 +567,8 @@ int hfsplus_cat_write_inode(struct inode *inode)
> folder->content_mod_date = hfsp_ut2mt(inode->i_mtime);
> folder->attribute_mod_date = hfsp_ut2mt(inode->i_ctime);
> folder->valence = cpu_to_be32(inode->i_size - 2);
> + if (folder->flags & cpu_to_be16(HFSPLUS_HAS_FOLDER_COUNT))
> + folder->folder_count = cpu_to_be32(HFSPLUS_I(inode)->folder_count);
> hfs_bnode_write(fd.bnode, &entry, fd.entryoffset,
> sizeof(struct hfsplus_cat_folder));
> } else if (HFSPLUS_IS_RSRC(inode)) {
> --
> 1.7.10.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
