[PATCH] hfsplus: emit proper file type from readdir

[PATCH] hfsplus: emit proper file type from readdir

[Sergei Antonov]

hfsplus_readdir() incorrectly returned DT_REG for symbolic links and special
files. Return DT_LNK, DT_FIFO, DT_CHR, DT_REG, DT_SOCK, or DT_UNKNOWN according
to mode field in catalog record. Programs relying on information from readdir
will now work correctly with HFS+.

CC: Al Viro <viro@zeniv.linux.org.uk>
CC: Christoph Hellwig <hch@infradead.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Vyacheslav Dubeyko <slava@dubeyko.com>
CC: Hin-Tak Leung <htl10@users.sourceforge.net>
Signed-off-by: Sergei Antonov <saproj@gmail.com>
---
 fs/hfsplus/dir.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
index bdec665..8397d64 100644
--- a/fs/hfsplus/dir.c
+++ b/fs/hfsplus/dir.c
@@ -212,13 +212,31 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
 				    be32_to_cpu(entry.folder.id), DT_DIR))
 				break;
 		} else if (type == HFSPLUS_FILE) {
+			u16 mode;
+			unsigned type = DT_UNKNOWN;
+
 			if (fd.entrylength < sizeof(struct hfsplus_cat_file)) {
 				pr_err("small file entry\n");
 				err = -EIO;
 				goto out;
 			}
+
+			mode = be16_to_cpu(entry.file.permissions.mode);
+			if (S_ISFIFO(mode))
+				type = DT_FIFO;
+			if (S_ISCHR(mode))
+				type = DT_CHR;
+			if (S_ISBLK(mode))
+				type = DT_BLK;
+			if (S_ISREG(mode))
+				type = DT_REG;
+			if (S_ISLNK(mode))
+				type = DT_LNK;
+			if (S_ISSOCK(mode))
+				type = DT_SOCK;
+
 			if (!dir_emit(ctx, strbuf, len,
-				    be32_to_cpu(entry.file.id), DT_REG))
+				      be32_to_cpu(entry.file.id), type))
 				break;
 		} else {
 			pr_err("bad catalog entry type\n");
-- 
1.9.0

Re: [PATCH] hfsplus: emit proper file type from readdir

[Anton Altaparmakov]

Hi,

On 7 May 2014, at 15:54, Sergei Antonov <saproj@gmail.com> wrote:

> hfsplus_readdir() incorrectly returned DT_REG for symbolic links and special
> files. Return DT_LNK, DT_FIFO, DT_CHR, DT_REG, DT_SOCK, or DT_UNKNOWN according
> to mode field in catalog record. Programs relying on information from readdir
> will now work correctly with HFS+.
> 
> CC: Al Viro <viro@zeniv.linux.org.uk>
> CC: Christoph Hellwig <hch@infradead.org>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Vyacheslav Dubeyko <slava@dubeyko.com>
> CC: Hin-Tak Leung <htl10@users.sourceforge.net>
> Signed-off-by: Sergei Antonov <saproj@gmail.com>
> ---
> fs/hfsplus/dir.c | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
> index bdec665..8397d64 100644
> --- a/fs/hfsplus/dir.c
> +++ b/fs/hfsplus/dir.c
> @@ -212,13 +212,31 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
> 				    be32_to_cpu(entry.folder.id), DT_DIR))
> 				break;
> 		} else if (type == HFSPLUS_FILE) {
> +			u16 mode;
> +			unsigned type = DT_UNKNOWN;
> +
> 			if (fd.entrylength < sizeof(struct hfsplus_cat_file)) {
> 				pr_err("small file entry\n");
> 				err = -EIO;
> 				goto out;
> 			}
> +
> +			mode = be16_to_cpu(entry.file.permissions.mode);
> +			if (S_ISFIFO(mode))
> +				type = DT_FIFO;
> +			if (S_ISCHR(mode))
> +				type = DT_CHR;
> +			if (S_ISBLK(mode))
> +				type = DT_BLK;
> +			if (S_ISREG(mode))
> +				type = DT_REG;
> +			if (S_ISLNK(mode))
> +				type = DT_LNK;
> +			if (S_ISSOCK(mode))
> +				type = DT_SOCK;
> +

I would either use "switch"/"case" or "else if" instead of "if" otherwise you are doing all the checks for all the types even after you have found the right one - also place the DT_REG check first as that is the common case...

Best regards,

	Anton

> 			if (!dir_emit(ctx, strbuf, len,
> -				    be32_to_cpu(entry.file.id), DT_REG))
> +				      be32_to_cpu(entry.file.id), type))
> 				break;
> 		} else {
> 			pr_err("bad catalog entry type\n");

-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

Re: [PATCH] hfsplus: emit proper file type from readdir

[Vyacheslav Dubeyko]

Hi,

On Wed, 2014-05-07 at 16:55 +0100, Anton Altaparmakov wrote:

[snip]
> > +
> > +			mode = be16_to_cpu(entry.file.permissions.mode);
> > +			if (S_ISFIFO(mode))
> > +				type = DT_FIFO;
> > +			if (S_ISCHR(mode))
> > +				type = DT_CHR;
> > +			if (S_ISBLK(mode))
> > +				type = DT_BLK;
> > +			if (S_ISREG(mode))
> > +				type = DT_REG;
> > +			if (S_ISLNK(mode))
> > +				type = DT_LNK;
> > +			if (S_ISSOCK(mode))
> > +				type = DT_SOCK;
> > +
> 
> I would either use "switch"/"case" or "else if" instead of "if" otherwise you are doing all the checks for all the types even after you have found the right one - also place the DT_REG check first as that is the common case...
> 

I believe that such example is good solution too:

static unsigned char
nilfs_filetype_table[NILFS_FT_MAX] = {
        [NILFS_FT_UNKNOWN]      = DT_UNKNOWN,
        [NILFS_FT_REG_FILE]     = DT_REG,
        [NILFS_FT_DIR]          = DT_DIR,
        [NILFS_FT_CHRDEV]       = DT_CHR,
        [NILFS_FT_BLKDEV]       = DT_BLK,
        [NILFS_FT_FIFO]         = DT_FIFO,
        [NILFS_FT_SOCK]         = DT_SOCK,
        [NILFS_FT_SYMLINK]      = DT_LNK,
};

What about likewise way?

Thanks,
Vyacheslav Dubeyko.

Re: [PATCH] hfsplus: emit proper file type from readdir

[Anton Altaparmakov]

Hi,

On 7 May 2014, at 17:25, Vyacheslav Dubeyko <slava@dubeyko.com> wrote:
> On Wed, 2014-05-07 at 16:55 +0100, Anton Altaparmakov wrote:
> 
> [snip]
>>> +
>>> +			mode = be16_to_cpu(entry.file.permissions.mode);
>>> +			if (S_ISFIFO(mode))
>>> +				type = DT_FIFO;
>>> +			if (S_ISCHR(mode))
>>> +				type = DT_CHR;
>>> +			if (S_ISBLK(mode))
>>> +				type = DT_BLK;
>>> +			if (S_ISREG(mode))
>>> +				type = DT_REG;
>>> +			if (S_ISLNK(mode))
>>> +				type = DT_LNK;
>>> +			if (S_ISSOCK(mode))
>>> +				type = DT_SOCK;
>>> +
>> 
>> I would either use "switch"/"case" or "else if" instead of "if" otherwise you are doing all the checks for all the types even after you have found the right one - also place the DT_REG check first as that is the common case...
>> 
> 
> I believe that such example is good solution too:
> 
> static unsigned char
> nilfs_filetype_table[NILFS_FT_MAX] = {
>        [NILFS_FT_UNKNOWN]      = DT_UNKNOWN,
>        [NILFS_FT_REG_FILE]     = DT_REG,
>        [NILFS_FT_DIR]          = DT_DIR,
>        [NILFS_FT_CHRDEV]       = DT_CHR,
>        [NILFS_FT_BLKDEV]       = DT_BLK,
>        [NILFS_FT_FIFO]         = DT_FIFO,
>        [NILFS_FT_SOCK]         = DT_SOCK,
>        [NILFS_FT_SYMLINK]      = DT_LNK,
> };
> 
> What about likewise way?

Sure, that also works.  My objection was purely because all the "if"s were being evaluated unnecessarily.

Best regards,

	Anton

> Thanks,
> Vyacheslav Dubeyko.

-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

Re: [PATCH] hfsplus: emit proper file type from readdir

[Sergei Antonov]

On 7 May 2014 18:25, Vyacheslav Dubeyko <slava@dubeyko.com> wrote:
> Hi,
>
> On Wed, 2014-05-07 at 16:55 +0100, Anton Altaparmakov wrote:
>
> [snip]
>> > +
>> > +                   mode = be16_to_cpu(entry.file.permissions.mode);
>> > +                   if (S_ISFIFO(mode))
>> > +                           type = DT_FIFO;
>> > +                   if (S_ISCHR(mode))
>> > +                           type = DT_CHR;
>> > +                   if (S_ISBLK(mode))
>> > +                           type = DT_BLK;
>> > +                   if (S_ISREG(mode))
>> > +                           type = DT_REG;
>> > +                   if (S_ISLNK(mode))
>> > +                           type = DT_LNK;
>> > +                   if (S_ISSOCK(mode))
>> > +                           type = DT_SOCK;
>> > +
>>
>> I would either use "switch"/"case" or "else if" instead of "if" otherwise you are doing all the checks for all the types even after you have found the right one - also place the DT_REG check first as that is the common case...
>>
>
> I believe that such example is good solution too:
>
> static unsigned char
> nilfs_filetype_table[NILFS_FT_MAX] = {
>         [NILFS_FT_UNKNOWN]      = DT_UNKNOWN,
>         [NILFS_FT_REG_FILE]     = DT_REG,
>         [NILFS_FT_DIR]          = DT_DIR,
>         [NILFS_FT_CHRDEV]       = DT_CHR,
>         [NILFS_FT_BLKDEV]       = DT_BLK,
>         [NILFS_FT_FIFO]         = DT_FIFO,
>         [NILFS_FT_SOCK]         = DT_SOCK,
>         [NILFS_FT_SYMLINK]      = DT_LNK,
> };
>
> What about likewise way?

Using an array means calculating an index to it using such expression:
(mode & S_IFMT) >> 12.
I prefer a more straightforward "if/else if".

Re: [PATCH] hfsplus: emit proper file type from readdir

[Matthew Wilcox]

On Wed, May 07, 2014 at 06:56:56PM +0200, Sergei Antonov wrote:
> On 7 May 2014 18:25, Vyacheslav Dubeyko <slava@dubeyko.com> wrote:
> > I believe that such example is good solution too:
> >
> > static unsigned char
> > nilfs_filetype_table[NILFS_FT_MAX] = {
> >         [NILFS_FT_UNKNOWN]      = DT_UNKNOWN,
> >         [NILFS_FT_REG_FILE]     = DT_REG,
> >         [NILFS_FT_DIR]          = DT_DIR,
> >         [NILFS_FT_CHRDEV]       = DT_CHR,
> >         [NILFS_FT_BLKDEV]       = DT_BLK,
> >         [NILFS_FT_FIFO]         = DT_FIFO,
> >         [NILFS_FT_SOCK]         = DT_SOCK,
> >         [NILFS_FT_SYMLINK]      = DT_LNK,
> > };
> >
> > What about likewise way?
> 
> Using an array means calculating an index to it using such expression:
> (mode & S_IFMT) >> 12.
> I prefer a more straightforward "if/else if".

This should all be wrapped up neatly like so:

 		} else if (type == HFSPLUS_FILE) {
+			unsigned type;
+
			if (fd.entrylength < sizeof(struct hfsplus_cat_file)) {
				pr_err("small file entry\n");
				err = -EIO;
				goto out;
			}
+
+			type = hfsplus_de_type(&entry);
+
			if (!dir_emit(ctx, strbuf, len,
-					be32_to_cpu(entry.file.id), DT_REG))
+					be32_to_cpu(entry.file.id), type))
 				break;

and then the array, case, series of else ifs, whatever can all be confined
to a single function.

Re: [PATCH] hfsplus: emit proper file type from readdir

[Sergei Antonov]

On 9 May 2014 21:05, Matthew Wilcox <willy@linux.intel.com> wrote:
> On Wed, May 07, 2014 at 06:56:56PM +0200, Sergei Antonov wrote:
>> On 7 May 2014 18:25, Vyacheslav Dubeyko <slava@dubeyko.com> wrote:
>> > I believe that such example is good solution too:
>> >
>> > static unsigned char
>> > nilfs_filetype_table[NILFS_FT_MAX] = {
>> >         [NILFS_FT_UNKNOWN]      = DT_UNKNOWN,
>> >         [NILFS_FT_REG_FILE]     = DT_REG,
>> >         [NILFS_FT_DIR]          = DT_DIR,
>> >         [NILFS_FT_CHRDEV]       = DT_CHR,
>> >         [NILFS_FT_BLKDEV]       = DT_BLK,
>> >         [NILFS_FT_FIFO]         = DT_FIFO,
>> >         [NILFS_FT_SOCK]         = DT_SOCK,
>> >         [NILFS_FT_SYMLINK]      = DT_LNK,
>> > };
>> >
>> > What about likewise way?
>>
>> Using an array means calculating an index to it using such expression:
>> (mode & S_IFMT) >> 12.
>> I prefer a more straightforward "if/else if".
>
> This should all be wrapped up neatly like so:
>
>                 } else if (type == HFSPLUS_FILE) {
> +                       unsigned type;
> +
>                         if (fd.entrylength < sizeof(struct hfsplus_cat_file)) {
>                                 pr_err("small file entry\n");
>                                 err = -EIO;
>                                 goto out;
>                         }
> +
> +                       type = hfsplus_de_type(&entry);
> +

A dedicated function is a good idea. I regret it didn't occur to me at
the moment of writing the patch.

Furthermore, hfsplus_readdir() is 125 lines long which exceeds
Documentation/CodingStyle recommended limit of two screenfuls of text
(i.e. around 50 lines). It makes sense to submit a patch breaking
hfsplus_readdir() into several (not only hfsplus_de_type()) smaller
functions.

>                         if (!dir_emit(ctx, strbuf, len,
> -                                       be32_to_cpu(entry.file.id), DT_REG))
> +                                       be32_to_cpu(entry.file.id), type))
>                                 break;
>
> and then the array, case, series of else ifs, whatever can all be confined
> to a single function.