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@xxxxxxxxxxxxxxxxxx>, ChristophHellwig
<hch@xxxxxxxxxxxxx>, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>. I will
glad to add my Reviewed-by because current state of the patch looks good
for me.
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
