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

[Vyacheslav Dubeyko <slava@dubeyko.com>]

Hi Sergei,

On Thu, 2014-02-06 at 18:27 +0100, Sergei Antonov wrote:

> > So, could you check that there isn't any dependencies from mount options
> > or case sensitivity for HFSX case?
> 
> Regarding [1]. 
> HFS_FOLDERCOUNT is not a mount option. In hfs_vfsutils.c there is such 
> sequence: HFSX signature -> HFS_X flag -> HFS_FOLDERCOUNT flag. No other 
> conditions.
> 
> Regarding [2].
> I analysed this before. A misleading snippet, but it is OK.
> This code is a format utility. It has -s parameter indicating "create 
> case-sensitive FS". Only HFSX can be case-sensitive and it is the only 
> case the utility creates HFSX (it tries to create a more traditional HFS+ 
> whenever possible). So in the snippt you show the if condition is an 
> awkward way of checking "if we are to create HFSX".
> 
> Of course, I've looked through other related places in the code.

Ok. Thank you.

>  
> > > +/*
> > > + * Increment subfolder count. Note, the value is only meaningful
> > > + * for folders with HFSPLUS_HAS_FOLDER_COUNT flag set.
> > > + */
> > > +static void hfsplus_subfolders_inc(struct inode *dir)
> > > +{
> > > +	HFSPLUS_I(dir)->subfolders++;
> > > +}
> > > +
> > 
> > I suppose that using inline keyword or macro declaration can be a better
> > choice.
> 
> Use macros when functions do the job? No way!
> GCC will inline functions without my "inline" hint.
> 

Ok. I don't insist. But I think that using inline keyword is a good way.
Also, I don't think that macros is a way to the hell. :)

> I added checks into them, see the new version of my patch. The code was 
> correct without the checks (they only catch cases when FS is already 
> corrupted), but I decided to make logic similar to that of Apple.
> 

> 
> Conditional decrement - yes. Added in this patch.
> Conditional increment - do you mean a check for 'folder_count' from volume 
> header (also automatically preventing an integer overflow)? Apple does 
> not do it, but we can.
>  

I meant only that you have done in the patch yet. But, maybe, it makes
sense to check on integer overflow or folders count limitation for HFS+.

> > >  	/* remove old thread entry */
> > > diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> > > index 08846425b..62d571e 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 subfolders;		/* Subfolder count (HFSX only) */
> > 
> > "Subfolders count" in comment. It's simply mistyping.
> 
> It is not. Trust me.
> 

I think that "subfolders count" is correct. But if you think in other
way then I don't insist. It is not critical. But I suppose that we have
necessity in any count when we have many items instead of one.

> > >  /* 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	/* (HFSX only) */
> > >  
> > 
> > I've hoped that you describe flag purpose in more details. :)
> > I mean when it work, in what conditions and so on.
> 
> Expanded it a little in the new patch. Suggest your own text if you still 
> think more details are needed. No reason to write much on it. The flag's 
> only pecularity is it's HFSX-only, but otherwise it's no more mysterious 
> than other flags.
> 

Ok. Now I see that it is enough.

> By the way, there is also flag 32 which I hope to add support for 
> (encountered fsck errors caused by lack of support of it).
> 

It makes sense if this flag will be used in drivers functionality. I
think so.

> > >  /* 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..55ffba8 100644
> > > --- a/fs/hfsplus/inode.c
> > > +++ b/fs/hfsplus/inode.c
> > > @@ -375,6 +375,7 @@ struct inode *hfsplus_new_inode(struct super_block *sb, umode_t mode)
> > >  	hip->extent_state = 0;
> > >  	hip->flags = 0;
> > >  	hip->userflags = 0;
> > > +	hip->subfolders = 0;
> > >  	memset(hip->first_extents, 0, sizeof(hfsplus_extent_rec));
> > >  	memset(hip->cached_extents, 0, sizeof(hfsplus_extent_rec));
> > >  	hip->alloc_blocks = 0;
> > > @@ -494,6 +495,10 @@ 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;
> > > +		if (folder->flags & cpu_to_be16(HFSPLUS_HAS_FOLDER_COUNT)) {
> > > +			HFSPLUS_I(inode)->subfolders
> > > +				 = be32_to_cpu(folder->subfolders);
> > 
> > Usually, assign symbol is placed on lvalue's line. Maybe it makes sense
> > to declare hip variable for HFSPLUS_I(inode) and place the whole
> > operation on one line?
> 
> Just fixed = position in both palces. Thanks for telling. I will get 
> better at coding style.
> 

Ok. Now we have elaborated a good state of the patch, from my viewpoint.
I suggest to prepare and to send last version of the patch. Please, add
in Cc Al Viro <viro@zeniv.linux.org.uk>, ChristophHellwig
<hch@infradead.org>, Andrew Morton <akpm@linux-foundation.org>. I will
glad to add my Reviewed-by because current state of the patch looks good
for me.

Thanks,
Vyacheslav Dubeyko.