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@xxxxxxxxx> > --- > 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. -- 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