On Thu, Oct 25, 2018 at 01:20:44PM +0200, Jan Kara wrote:
> On Tue 23-10-18 21:19:53, Phillip Potter wrote:
> > Many file systems use a copy&paste implementation
> > of dirent to on-disk file type conversions.
> >
> > Create a common implementation to be used by file systems
> > with some useful conversion helpers to reduce open coded
> > file type conversions in file system code.
>
> Thanks for the patch. I like the cleanup. Some comments inline...
>
> > diff --git a/include/linux/file_type.h b/include/linux/file_type.h
> > new file mode 100644
> > index 000000000000..f015c41ca90c
> > --- /dev/null
> > +++ b/include/linux/file_type.h
> > @@ -0,0 +1,108 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _LINUX_FILE_TYPE_H
> > +#define _LINUX_FILE_TYPE_H
> > +
> > +/*
> > + * This is a common implementation of dirent to fs on-disk
> > + * file type conversion. Although the fs on-disk bits are
> > + * specific to every file system, in practice, many file systems
> > + * use the exact same on-disk format to describe the lower 3
> > + * file type bits that represent the 7 POSIX file types.
> > + * All those file systems can use this generic code for the
> > + * conversions:
> > + * i_mode -> fs on-disk file type (ftype)
> > + * fs on-disk file type (ftype) -> dirent file type (dtype)
> > + * i_mode -> dirent file type (dtype)
>
> I don't think these three lines above are really useful. If you want to
> make it easier for fs developers, add Docbook coments at function
> implementations.
>
> > + */
> > +
> > +/*
> > + * struct dirent file types
> > + * exposed to user via getdents(2), readdir(3)
> > + *
> > + * These match bits 12..15 of stat.st_mode
> > + * (ie "(i_mode >> 12) & 15").
> > + */
> > +#define S_DT_SHIFT 12
> > +#define S_DT(mode) (((mode) & S_IFMT) >> S_DT_SHIFT)
> > +#define S_DT_MASK (S_IFMT >> S_DT_SHIFT)
> > +
> > +#define DT_UNKNOWN 0
> > +#define DT_FIFO S_DT(S_IFIFO) /* 1 */
> > +#define DT_CHR S_DT(S_IFCHR) /* 2 */
> > +#define DT_DIR S_DT(S_IFDIR) /* 4 */
> > +#define DT_BLK S_DT(S_IFBLK) /* 6 */
> > +#define DT_REG S_DT(S_IFREG) /* 8 */
> > +#define DT_LNK S_DT(S_IFLNK) /* 10 */
> > +#define DT_SOCK S_DT(S_IFSOCK) /* 12 */
>
> Why not keep the original definition by absolute number. After all these
> must match glibc...
>
> > +#define DT_WHT 14
> > +
> > +#define DT_MAX (S_DT_MASK + 1) /* 16 */
> > +
> > +/*
> > + * fs on-disk file types.
> > + * Only the low 3 bits are used for the POSIX file types.
> > + * Other bits are reserved for fs private use.
> > + *
> > + * Note that no fs currently stores the whiteout type on-disk,
> > + * so whiteout dirents are exposed to user as DT_CHR.
> > + */
> > +#define FT_UNKNOWN 0
> > +#define FT_REG_FILE 1
> > +#define FT_DIR 2
> > +#define FT_CHRDEV 3
> > +#define FT_BLKDEV 4
> > +#define FT_FIFO 5
> > +#define FT_SOCK 6
> > +#define FT_SYMLINK 7
> > +
> > +#define FT_MAX 8
> > +
> > +/*
> > + * fs on-disk file type to dirent file type conversion
> > + */
> > +static unsigned char fs_dtype_by_ftype[FT_MAX] = {
> > + [FT_UNKNOWN] = DT_UNKNOWN,
> > + [FT_REG_FILE] = DT_REG,
> > + [FT_DIR] = DT_DIR,
> > + [FT_CHRDEV] = DT_CHR,
> > + [FT_BLKDEV] = DT_BLK,
> > + [FT_FIFO] = DT_FIFO,
> > + [FT_SOCK] = DT_SOCK,
> > + [FT_SYMLINK] = DT_LNK
> > +};
>
> So you define static variable in a header file. That will make it appear in
> each and every object that includes this header (when not used it will get
> optimized away but still...). IMO that is not good and I don't think
> anything here is so performance super-crittical, that the cost of
> additional function call would matter (correct me if I'm wrong here). So
> I'd rather see these tables and associated functions in some C file.
>
> > +static inline unsigned char fs_dtype(int filetype)
>
> This function name is not very descriptive and consistent with the other
> two. It should be like fs_ftype_to_dtype(), right?
>
> > +{
> > + if (filetype >= FT_MAX)
> > + return DT_UNKNOWN;
> > +
> > + return fs_dtype_by_ftype[filetype];
> > +}
>
> Otherwise the patch looks good to me.
>
> Honza
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR
Dear Jan,
Thanks for your comments/feedback, really appreciate it. All good points
and I will make sure I act on it all before republishing the series. I'd
be interested to know your thoughts on Ted T'so's suggestion about moving
the new header to the uapi area - yes or no in your opinion? Happy with
either, but looking for the widest possible acceptance. Thanks.
Regards,
Phil
