fs: common implementation of file type
RFC PATCH v3 1/10 fs: common implementation of file type
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.
Original patch written by Amir Goldstein.
v3:
- Rebased against Linux 4.19 by Phillip Potter
- Added SPDX tag to new include/linux/file_type.h
- Added include/linux/file_type.h to MAINTAINERS
v2:
- s/DT_MASK/S_DT_MASK to fix redefinition in drivers/scsi/qla2xxx/qla_def.h
- Explicit initialization of fs_dtype_by_ftype[] using [FT_*] = DT_*
v1:
- Initial implementation
--- MAINTAINERS | 1 + include/linux/file_type.h | 108 ++++++++++++++++++++++++++++++++++++++ include/linux/fs.h | 17 +----- 3 files changed, 110 insertions(+), 16 deletions(-) create mode 100644 include/linux/file_type.h
MAINTAINERS +1 -0
@@ -5689,6 +5689,7 @@ L: [email protected] S: Maintained F: fs/* F: include/linux/fs.h +F: include/linux/file_type.h F: include/uapi/linux/fs.h FINTEK F75375S HARDWARE MONITOR AND FAN CONTROLLER DRIVER
include/linux/file_type.h new file +108 -0
108 lines not on this page · raw message
include/linux/fs.h +1 -16
@@ -37,6 +37,7 @@#include <linux/uuid.h> #include <linux/errseq.h> #include <linux/ioprio.h> +#include <linux/file_type.h> #include <asm/byteorder.h> #include <uapi/linux/fs.h>
@@ -1663,22 +1664,6 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical, u64 phys, u64 len, u32 flags);int fiemap_check_flags (struct fiemap_extent_info *fieinfo,u32 fs_flags); -/* -* File types -* -* NOTE! These match bits 12..15 of stat.st_mode -* (ie "(i_mode >> 12) & 15"). -*/ -#define DT_UNKNOWN 0 -#define DT_FIFO 1 -#define DT_CHR 2 -#define DT_DIR 4 -#define DT_BLK 6 -#define DT_REG 8 -#define DT_LNK 10 -#define DT_SOCK 12 -#define DT_WHT 14 -/* * This is the "filldir" function type, used by readdir() to let * the kernel specify what kind of dirent layout it wants to have.
Signature · 1 line
2.17.2
Quoted 9 lines · Phillip Potter
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.
Original patch written by Amir Goldstein.
Looks good.
I guess you used 'git apply' or just 'patch'
What you usually do when applying someone else mostly unchanged patches is use 'git am -s -3' so you preserve the original author and original commit message including the Signed-of-by line. You can edit your patch by hand to change the From: line to change the author and add
(you sign below me as you changed the patch last)
Quoted 6 lines
v3:
- Rebased against Linux 4.19 by Phillip Potter
- Added SPDX tag to newinclude/linux/file_type.h
- Addedinclude/linux/file_type.htoMAINTAINERS
As you can see in my original posting, this part comes after the --- line which means it will not be included in the commit message, because the specific development history of this patch may be interesting for reviewers but it is less interesting in the context of git log.
Quoted 179 lines
v2:
- s/DT_MASK/S_DT_MASK to fix redefinition indrivers/scsi/qla2xxx/qla_def.h
- Explicit initialization of fs_dtype_by_ftype[] using [FT_*] = DT_*v1:
- Initial implementationSigned-off-by Phillip Potter <[email protected]>--- MAINTAINERS | 1 + include/linux/file_type.h | 108 ++++++++++++++++++++++++++++++++++++++ include/linux/fs.h | 17 +----- 3 files changed, 110 insertions(+), 16 deletions(-) create mode 100644 include/linux/file_type.hMAINTAINERS +1
-0@@ -5689,6 +5689,7 @@ L: [email protected] S: Maintained F: fs/* F: include/linux/fs.h +F: include/linux/file_type.h F: include/uapi/linux/fs.h FINTEK F75375S HARDWARE MONITOR AND FAN CONTROLLER DRIVERinclude/linux/file_type.h new file +108
-0@@ -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) +*/ + +/* +* 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 */ +#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 +}; + +static inline unsigned char fs_dtype (int filetype) +{ +if (filetype >=FT_MAX ) +return DT_UNKNOWN ; + +return fs_dtype_by_ftype[filetype]; +} + +/* +* dirent file type to fs on-disk file type conversion +* Values not initialized explicitly are FT_UNKNOWN (0). +*/ +static unsigned char fs_ftype_by_dtype[DT_MAX ] = { + [DT_REG ] =FT_REG_FILE , + [DT_DIR ] =FT_DIR , + [DT_LNK ] =FT_SYMLINK , + [DT_CHR ] =FT_CHRDEV , + [DT_BLK ] =FT_BLKDEV , + [DT_FIFO ] =FT_FIFO , + [DT_SOCK ] =FT_SOCK , +}; + +/* st_mode to fs on-disk file type conversion */ +static inline unsigned char fs_umode_to_ftype (umode_t mode) +{ +return fs_ftype_by_dtype[S_DT (mode)]; +} + +/* st_mode to dirent file type conversion */ +static inline unsigned char fs_umode_to_dtype (umode_t mode) +{ +return fs_dtype (fs_umode_to_ftype (mode)); +} + +#endif include/linux/fs.h +1
-17@@ -37,6 +37,7 @@#include <linux/uuid.h> #include <linux/errseq.h> #include <linux/ioprio.h> +#include <linux/file_type.h> #include <asm/byteorder.h> #include <uapi/linux/fs.h> @@ -1663,22 +1664,6 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical, u64 phys, u64 len, u32 flags);int fiemap_check_flags (struct fiemap_extent_info *fieinfo,u32 fs_flags); -/* -* File types -* -* NOTE! These match bits 12..15 of stat.st_mode -* (ie "(i_mode >> 12) & 15"). -*/ -#define DT_UNKNOWN 0 -#define DT_FIFO 1 -#define DT_CHR 2 -#define DT_DIR 4 -#define DT_BLK 6 -#define DT_REG 8 -#define DT_LNK 10 -#define DT_SOCK 12 -#define DT_WHT 14 -/* * This is the "filldir" function type, used by readdir() to let * the kernel specify what kind of dirent layout it wants to have. -- 2.17.2
Quoted 18 lines · Amir Goldstein
Quoted 9 lines · Phillip Potter
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.
Original patch written by Amir Goldstein.
Looks good.
I guess you used 'git apply' or just 'patch'
What you usually do when applying someone else mostly unchanged patches is use 'git am -s -3' so you preserve the original author and original commit message including the Signed-of-by line. You can edit your patch by hand to change the From: line to change the author and add
Quoted 3 lines
Signed-off-by Amir Goldstein <[email protected]>(you sign below me as you changed the patch last)
Dear Amir,
Yes, I applied each patch manually to my tree, fixed it up where needed, then after rebuilding and testing each one I committed it and regenerated each patch. Thank you very much for your advice, I will take it into account and make the necessary changes. In the meantime, do I add other tags in the order they are received also (such as Reviewed-by:) and am I safe to add these in when I re-send the patches with the changes you and others have suggested (or would that offend people that have offered the tags)?
Regards,
Phil
Quoted 26 lines · Phillip Potter
Quoted 18 lines · Amir Goldstein
Quoted 9 lines · Phillip Potter
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.
Original patch written by Amir Goldstein.
Looks good.
I guess you used 'git apply' or just 'patch'
What you usually do when applying someone else mostly unchanged patches is use 'git am -s -3' so you preserve the original author and original commit message including the Signed-of-by line. You can edit your patch by hand to change the From: line to change the author and addQuoted 3 lines
Signed-off-by Amir Goldstein <[email protected]>(you sign below me as you changed the patch last)
Dear Amir,
Quoted 9 lines
Yes, I applied each patch manually to my tree, fixed it up where needed, then after rebuilding and testing each one I committed it and regenerated each patch. Thank you very much for your advice, I will take it into account and make the necessary changes. In the meantime, do I add other tags in the order they are received also (such as Reviewed-by:) and am I safe to add these in when I re-send the patches with the changes you and others have suggested (or would that offend people that have offered the tags)?
Reviewed-by before of after Signed-off-by.
I prefer Signed-off-by last which conceptually covers the entire patch, the commit message including all the review tags that you may have added.
Some developers add Reviewed-by after Signed-off-by signifying the order that things happened, so choose your own preference.
As a reviewer, and I speak only for myself, if I offered my Reviewed-by I expect it to be removed if a future revision of the patch has changed so I have an indication of patches that I need to re-review. But if the patch changed very lightly, like small edits to commit message and code nits in general, that would not invalidate my review. When in doubt, you can always explicitly ask the reviewer.
Thanks,
Amir.
Quoted 27 lines · Amir Goldstein
Quoted 11 lines · Phillip Potter
Dear Amir,
Yes, I applied each patch manually to my tree, fixed it up where needed, then after rebuilding and testing each one I committed it and regenerated each patch. Thank you very much for your advice, I will take it into account and make the necessary changes. In the meantime, do I add other tags in the order they are received also (such as Reviewed-by:) and am I safe to add these in when I re-send the patches with the changes you and others have suggested (or would that offend people that have offered the tags)?
Reviewed-by before of after Signed-off-by.
I prefer Signed-off-by last which conceptually covers the entire patch, the commit message including all the review tags that you may have added.Some developers add Reviewed-by after Signed-off-by signifying the order that things happened, so choose your own preference.
As a reviewer, and I speak only for myself, if I offered my Reviewed-by I expect it to be removed if a future revision of the patch has changed so I have an indication of patches that I need to re-review. But if the patch changed very lightly, like small edits to commit message and code nits in general, that would not invalidate my review. When in doubt, you can always explicitly ask the reviewer.
Quoted 2 lines
Thanks,
Amir.
Dear Amir,
Thanks - I am just going to fix up the commit messages as you suggested using git am etc. The content of the patches themselves will not change (until further feedback is received).
Regards,
Phil
Quoted 29 lines · Phillip Potter
Quoted 27 lines · Amir Goldstein
Quoted 11 lines · Phillip Potter
Dear Amir,
Yes, I applied each patch manually to my tree, fixed it up where needed, then after rebuilding and testing each one I committed it and regenerated each patch. Thank you very much for your advice, I will take it into account and make the necessary changes. In the meantime, do I add other tags in the order they are received also (such as Reviewed-by:) and am I safe to add these in when I re-send the patches with the changes you and others have suggested (or would that offend people that have offered the tags)?
Reviewed-by before of after Signed-off-by.
I prefer Signed-off-by last which conceptually covers the entire patch, the commit message including all the review tags that you may have added.Some developers add Reviewed-by after Signed-off-by signifying the order that things happened, so choose your own preference.
As a reviewer, and I speak only for myself, if I offered my Reviewed-by I expect it to be removed if a future revision of the patch has changed so I have an indication of patches that I need to re-review. But if the patch changed very lightly, like small edits to commit message and code nits in general, that would not invalidate my review. When in doubt, you can always explicitly ask the reviewer.
Quoted 9 lines
Quoted 2 lines
Thanks,
Amir.Dear Amir,
Thanks - I am just going to fix up the commit messages as you suggested using git am etc. The content of the patches themselves will not change (until further feedback is received).
Well, I did request to change some content (the location and the comment above BUILD_BUG_ON section) which is relevant for several patches. However, so far affected patched did not get any Reviewed-by.
Thanks,
Amir.
Quoted 40 lines · Amir Goldstein
Quoted 29 lines · Phillip Potter
Quoted 27 lines · Amir Goldstein
Quoted 11 lines · Phillip Potter
Dear Amir,
Yes, I applied each patch manually to my tree, fixed it up where needed, then after rebuilding and testing each one I committed it and regenerated each patch. Thank you very much for your advice, I will take it into account and make the necessary changes. In the meantime, do I add other tags in the order they are received also (such as Reviewed-by:) and am I safe to add these in when I re-send the patches with the changes you and others have suggested (or would that offend people that have offered the tags)?
Reviewed-by before of after Signed-off-by.
I prefer Signed-off-by last which conceptually covers the entire patch, the commit message including all the review tags that you may have added.Some developers add Reviewed-by after Signed-off-by signifying the order that things happened, so choose your own preference.
As a reviewer, and I speak only for myself, if I offered my Reviewed-by I expect it to be removed if a future revision of the patch has changed so I have an indication of patches that I need to re-review. But if the patch changed very lightly, like small edits to commit message and code nits in general, that would not invalidate my review. When in doubt, you can always explicitly ask the reviewer.
Quoted 9 lines
Quoted 2 lines
Thanks,
Amir.Dear Amir,
Thanks - I am just going to fix up the commit messages as you suggested using git am etc. The content of the patches themselves will not change (until further feedback is received).
Quoted 6 lines
Well, I did request to change some content (the location and the comment above
BUILD_BUG_ONsection) which is relevant for several patches. However, so far affected patched did not get any Reviewed-by.Thanks,
Amir.
Sorry, I missed that bit at the end, was too keen to click through to the note about the alleged ext2 bug :-) I will make sure those changes are made as well. By location do you mean the location of the v3, v2, etc. stuff and your point about including it in the main [0/10] message rather than the patches themselves? Again, thank you for your feedback and for being patient with me, I really do appreciate it.
Regards,
Phil
Quoted 2 lines · Phillip Potter
On Wed, Oct 24, 2018 at 12:44:50PM +0300, Amir Goldstein wrote:
...
Quoted 7 lines
Quoted 6 lines
Well, I did request to change some content (the location and the comment above
BUILD_BUG_ONsection) which is relevant for several patches. However, so far affected patched did not get any Reviewed-by.Thanks,
Amir.
Quoted 7 lines
Sorry, I missed that bit at the end, was too keen to click through to the note about the alleged ext2 bug :-) I will make sure those changes are made as well. By location do you mean the location of the v3, v2, etc. stuff and your point about including it in the main [0/10] message rather than the patches themselves? Again, thank you for your feedback and for being patient with me, I really do appreciate it.
By location I meant place BUILD_BUG_ON() at the beginning of ext2_set_de_type() and not nested inside inner scope of ext2_readdir(). And find similar appropriate places for other patches.
And one more thing for next posting - LKML is probably a too wide forum for this filesystems cleanup series.
Cheers,
Amir.
Al Viro
maintainerQuoted 1 line · Phillip Potter
+static inline unsigned char
fs_dtype(int filetype)
That "int" is asking for trouble, especially since negative argument will blow up. And it comes from untrusted source...
Quoted 6 lines
include/linux/file_type.h new file +6
-0+{ +if (filetype >=FT_MAX ) +return DT_UNKNOWN ; + +return fs_dtype_by_ftype[filetype]; +}
include/linux/file_type.h
Quoted 1 line · Phillip Potter
include/linux/file_type.h +0
-0
Shouldn't this be in include/uapi/linux/fs_types.h?
One of things which must be made crystal clear is these definitions MUST NOT ever change. It would break the Userspace ABI, and would break file systems on-disk format.
It might also be useful to be clear *why* we are making this change in the first place. Code refactorization is good from a code maintenance perspective (either to fix bugs, although this code is pretty trivial), or to make it easier to make changes in a single central place (which MUST NOT) happen, or to make the compiled code more compact.
So some documentation of how much text is actually saved might be worthwhile.
- Ted
Quoted 6 lines · Theodore Y. Ts'o
include/linux/file_type.h
Quoted 1 line · Phillip Potter
include/linux/file_type.h +0
-0Shouldn't this be in
include/uapi/linux/fs_types.h?
IDGI. Why do we want this file in uapi?
The DT_ constants are already defined by glibc dirent.h and the FT_ constants and macros we don't want to expose to uapi at all. Right?
Maybe all we need is a comment above DT_ constants that those are defined by POSIX and in glibc dirent.h?
Quoted 8 lines
One of things which must be made crystal clear is these definitions MUST NOT ever change. It would break the Userspace ABI, and would break file systems on-disk format.
It might also be useful to be clear *why* we are making this change in the first place. Code refactorization is good from a code maintenance perspective (either to fix bugs, although this code is pretty trivial),
Very trivial code that has had an out of bounds access bug for two decades and bug was duplicated to 7 filesystems. IMO, fixing the bug in one place instead of 7 is a good enough reason for re-factoring.
Thanks,
Amir.
Quoted 7 lines · Al Viro
Quoted 1 line · Phillip Potter
+static inline unsigned char
fs_dtype(int filetype)That "int" is asking for trouble, especially since negative argument will blow up. And it comes from untrusted source...
Quoted 6 lines
include/linux/file_type.h line 80
Quoted 6 lines
include/linux/file_type.h new file +6
-0+{ +if (filetype >=FT_MAX ) +return DT_UNKNOWN ; + +return fs_dtype_by_ftype[filetype]; +}
Dear Al,
Thank you, good point, I will change to unsigned int and republish as part of new series.
Regards,
Phil
Quoted 16 lines · Theodore Y. Ts'o
include/linux/file_type.h
Quoted 1 line · Phillip Potter
include/linux/file_type.h +0
-0Shouldn't this be in
include/uapi/linux/fs_types.h?One of things which must be made crystal clear is these definitions MUST NOT ever change. It would break the Userspace ABI, and would break file systems on-disk format.
It might also be useful to be clear *why* we are making this change in the first place. Code refactorization is good from a code maintenance perspective (either to fix bugs, although this code is pretty trivial), or to make it easier to make changes in a single central place (which MUST NOT) happen, or to make the compiled code more compact.
Quoted 4 lines
So some documentation of how much text is actually saved might be worthwhile.
- Ted
Dear Ted,
Regarding location of the additional header, thank you for this suggestion - I will move it.
As for making it extra clear that the definitions must not change, I will add this to the comment at the beginning of the file, and also in the intro message of the new series I am about to publish.
I can't speak for Amir, but regarding why I think this would be a good change, my thought process is that it would make adding new file systems that use these on-disk type layouts in future more easy, as the central generic implementation can be used rather than copying and pasting and changing names etc. as needed, which is more error prone. I am happy to add more documentation in this regard, and also mention savings where appropriate.
Regards,
Phil
Quoted 25 lines · Amir Goldstein
Quoted 6 lines · Theodore Y. Ts'o
include/linux/file_type.h
Quoted 1 line · Phillip Potter
include/linux/file_type.h +0
-0Shouldn't this be in
include/uapi/linux/fs_types.h?IDGI. Why do we want this file in uapi?
TheDT_constants are already defined by glibcdirent.hand theFT_constants and macros we don't want to expose to uapi at all. Right?Maybe all we need is a comment above
DT_constants that those are defined by POSIX and in glibcdirent.h?Quoted 8 lines
One of things which must be made crystal clear is these definitions MUST NOT ever change. It would break the Userspace ABI, and would break file systems on-disk format.
It might also be useful to be clear *why* we are making this change in the first place. Code refactorization is good from a code maintenance perspective (either to fix bugs, although this code is pretty trivial),
Quoted 6 lines
Very trivial code that has had an out of bounds access bug for two decades and bug was duplicated to 7 filesystems. IMO, fixing the bug in one place instead of 7 is a good enough reason for re-factoring.
Thanks,
Amir.
Dear Amir,
Regarding the location of the extra header file, I'm happy to stick it in either include/linux or include/uapi/linux before sending out the revised series. You make a valid point about the FT_ constants and macros being kernel only though, so perhaps the comment is a better option? I will wait a bit before sending out as I'm interested to see what Ted thinks. Many thanks.
Regards,
Phil
Jan Kara
reviewerQuoted 6 lines · Phillip Potter
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...
Quoted 11 lines
include/linux/file_type.h new file +5
-0@@ -0,0 +1,108 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _LINUX_FILE_TYPE_H +#define _LINUX_FILE_TYPE_H + +/*
include/linux/file_type.h line 15
Quoted 10 lines
include/linux/file_type.h new file +10
-0+* 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.
Quoted 11 lines
include/linux/file_type.h new file +11
-0+*/ + +/* +* 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)
include/linux/file_type.h line 36
Quoted 10 lines
include/linux/file_type.h new file +10
-0+#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...
Quoted 26 lines
include/linux/file_type.h new file +26
-0+#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 +*/
include/linux/file_type.h line 72
Quoted 10 lines
include/linux/file_type.h new file +10
-0+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.
include/linux/file_type.h line 74
Quoted 1 line
include/linux/file_type.h new file +1
-0+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?
include/linux/file_type.h line 80
Quoted 6 lines
include/linux/file_type.h new file +6
-0+{ +if (filetype >=FT_MAX ) +return DT_UNKNOWN ; + +return fs_dtype_by_ftype[filetype]; +}
Otherwise the patch looks good to me.
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Quoted 119 lines · Jan Kara
Quoted 6 lines · Phillip Potter
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...
Quoted 11 lines
include/linux/file_type.h new file +5
-0@@ -0,0 +1,108 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _LINUX_FILE_TYPE_H +#define _LINUX_FILE_TYPE_H + +/* include/linux/file_type.h line 15
Quoted 10 lines
include/linux/file_type.h new file +10
-0+* 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.
Quoted 11 lines
include/linux/file_type.h new file +11
-0+*/ + +/* +* 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)include/linux/file_type.h line 36
Quoted 10 lines
include/linux/file_type.h new file +10
-0+#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...
Quoted 26 lines
include/linux/file_type.h new file +26
-0+#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 +*/ include/linux/file_type.h line 72
Quoted 10 lines
include/linux/file_type.h new file +10
-0+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.
include/linux/file_type.h line 74
Quoted 1 line
include/linux/file_type.h new file +1
-0+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?include/linux/file_type.h line 80
Quoted 6 lines
include/linux/file_type.h new file +6
-0+{ +if (filetype >=FT_MAX ) +return DT_UNKNOWN ; + +return fs_dtype_by_ftype[filetype]; +}Otherwise the patch looks good to me.
Quoted 4 lines
Honza
Signature · 2 lines
Jan Kara <[email protected]> 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
Jan Kara
reviewerQuoted 5 lines · Phillip Potter
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.
I don't think moving this to uapi headers makes sense. As Amir wrote, all necessary definitions for userspace already come from glibc headers.
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Quoted 7 lines · Jan Kara
Quoted 5 lines · Phillip Potter
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.
Quoted 7 lines
I don't think moving this to uapi headers makes sense. As Amir wrote, all necessary definitions for userspace already come from glibc headers.
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Thanks, I will leave it in include/linux for now then and separate out the relevant parts to a separate C file as you suggested.
Regards,
Phil
Quoted 18 lines · Amir Goldstein
Quoted 9 lines · Phillip Potter
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.
Original patch written by Amir Goldstein.
Looks good.
I guess you used 'git apply' or just 'patch'
What you usually do when applying someone else mostly unchanged patches is use 'git am -s -3' so you preserve the original author and original commit message including the Signed-of-by line. You can edit your patch by hand to change the From: line to change the author and add
Quoted 3 lines
Signed-off-by Amir Goldstein <[email protected]>(you sign below me as you changed the patch last)
Dear Amir,
I am almost ready to send out the new series with all the suggestions so far incorporated. Would you be happy for me to add to the commit messages slightly to mention David Sterba's point about a brief explanation? It would be the same in each case and I would keep it brief. Also, I have split the functions into a C file as per Jan Kara's suggestion. Are you still happy for me to still include your Signed-off-by tag or would you rather I keep it out until you've had a look? Thanks.
Regards,
Phil