Bug 42986 - ext2_fs.h requires undefined type umode_t
Summary: ext2_fs.h requires undefined type umode_t
Status: RESOLVED CODE_FIX
Alias: None
Product: File System
Classification: Unclassified
Component: ext2 (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: fs_ext2@kernel-bugs.osdl.org
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-24 13:22 UTC by Allan
Modified: 2012-04-13 06:48 UTC (History)
4 users (show)

See Also:
Kernel Version: 3.3
Subsystem:
Regression: Yes
Bisected commit-id:


Attachments

Description Allan 2012-03-24 13:22:59 UTC
In the user-space headers:

/usr/include/linux/ext2_fs.h:178:41: error: unknown type name 'umode_t'

umode_t is not provided in userspace since commit 0583fcc9
Comment 1 Theodore Tso 2012-03-24 14:04:34 UTC
Why do you need ext2_fs.h?   We're planning on removing ext2_fs.h as an exported header file very shortly (perhaps as soon as v3.4; it's already in Al Viro's VFS git tree).

Ext 2/3/4 userspace programs ***really*** should be using libext2fs, and if for some reason they don't want to do that, they can use the ext2_fs.h header file that is shipped with e2fsprogs and libext2fs.

Programs written as much as 8 years ago and which used libext2fs generally were able to support ext4 file systems have generally been able to support ext4; that's one of the major reasons why I strongly suggest the use of that library, instead of trying to use kernel header files to roll your own thing....
Comment 2 Allan 2012-03-24 14:16:19 UTC
I noticed this issue while building the latest syslinux.
Comment 3 Theodore Tso 2012-03-25 01:20:58 UTC
From Peter Anvin, the maintainer of Syslinux:

   Syslinux already includes a private copy of ext2_fs.h, but I
   need to scrub the umode_t inclusion.

This is therefore something that needs to be fixed in Syslinux, and not in the kernel, so I'm closing this bug.  Peter has been made aware of this issue, so presumably he will be fixing and releasing a new version of Syslinux shortly.
Comment 4 Allan 2012-03-25 02:47:29 UTC
Sure, that fixes syslinux...  But what about every other user of this header?  The answer of "don't use it" is not very good if it is being installed.
Comment 5 Theodore Tso 2012-03-25 17:21:44 UTC
It's not going to be installed.  Al Viro has patches in his git tree which is going to make it ***disappear*** from the kernel source tree altogether.  The definitions which are needed for fs/ext2 will be moved to fs/ext2/ext2.h.   The userspace definitions are being deleted.  The few places in the rest of the kernel which were using ext2_fs.h and implying knowledge of the ext2 superblock layout are being changed to use a function exported by the ext2 code, thus properly providing better abstraction between the various kernel components.

There should be no valid user of this header, and there should not have been one for at least eight years.   If they do exist, we'll find out when they scream, and then we'll tell them that they are broken, and they should be using the ext2_fs.h being shipped with e2fsprogs, and ideally libext2fs being shipped with e2fsprogs.

The whole point is that it's insane to be maintaining two userspace headers, especially since the one in the kernel tree has effectively not been maintained for years (at least for use in userspace).

Again, if you know of other actual users of the ext2_fs.h being shipped in the kernel, please let me know, and I'll personally reach out to them (and tell them that they are close to a decade behind the times....)
Comment 6 H. Peter Anvin 2012-03-27 04:41:22 UTC
Okay... I looked into this a bit more.

The reason Syslinux included <linux/ext2_fs.h> is to get the EXT2_IOC_* constants.  If <linux/ext2_fs.h> is going away, then what is expected to export those constants?
Comment 7 Theodore Tso 2012-03-27 05:02:37 UTC
Which EXT2_IOC_* constants are you using?   Aside from the largely specialized and as far as I know, not ever used, EXT2_IOC_GETRSVSZ and EXT3_IOC_SETRSVSZ, you can also use (and in fact are encouraged to use), the generic FS_IOC_* ioctls instead:

#define	EXT2_IOC_GETFLAGS		FS_IOC_GETFLAGS
#define	EXT2_IOC_SETFLAGS		FS_IOC_SETFLAGS
#define	EXT2_IOC_GETVERSION		FS_IOC_GETVERSION
#define	EXT2_IOC_SETVERSION		FS_IOC_SETVERSION

These ioctl started out as ext2 specific, but they are used by multiple file systems.
Comment 8 H. Peter Anvin 2012-03-27 05:33:47 UTC
That's fine, but it's also a gratuitous change to previously working userspace.  It sounds like the right thing would be to leave <linux/ext2_fs.h> in place exporting only those constants as a backwards compatibility measure.

I can update Syslinux, but it isn't just about Syslinux; in general it's rude to break userspace unnecessarily :-/
Comment 9 Theodore Tso 2012-03-27 05:58:48 UTC
Breaking things at compile-time (as opposed to breaking already created executables) is not something I worry that much about --- especially if they are flags that are fairly specialized.   In fact I was quite surprised there were programs other than lsattr/chattr that were using these ioctls.....
Comment 10 H. Peter Anvin 2012-03-27 06:05:47 UTC
The flags-setting ioctls aren't really all that different from chmod(), so I'm a bit surprised at your surprise.
Comment 11 Theodore Tso 2012-03-27 06:17:59 UTC
Yeah, but chmod is POSIX; EXT2_IOC_GETFLAGS is not.  And if you are using the EXT2_IOC_GETFLAGS variant, then you're saying that you only care about it working on ext2, and most programs hopefully aspire to more portability than that.  I really doubt there are _that_ many ext2-specific program out there, and the flags that can be set by IOC_GETFLAGS really are more esoteric than the chmod flags.

In any case, Al Viro is carrying the patch in his tree, so ultimately it will be up to him.
Comment 12 H. Peter Anvin 2012-03-27 06:21:24 UTC
It's mostly an issue with keeping compatibility with as many systems as possible.  It is fairly painful to deal with breakages in userspace without breaking backwards compatibility.

I'll talk to Al.
Comment 13 Denys Vlasenko 2012-04-12 11:37:05 UTC
For the record: busybox's mkfs_ext2.c uses linux/ext2_fs.h.
If its include is commented out, the following errors pop out:

error: ‘EXT2_DFL_MAX_MNT_COUNT’ undeclared (first use in this function)
error: ‘EXT2_DYNAMIC_REV’ undeclared (first use in this function)
error: ‘EXT2_ERRORS_DEFAULT’ undeclared (first use in this function)
error: ‘EXT2_FEATURE_COMPAT_DIR_INDEX’ undeclared (first use in this function)
error: ‘EXT2_FEATURE_COMPAT_RESIZE_INO’ undeclared (first use in this function)
error: ‘EXT2_FEATURE_COMPAT_SUPP’ undeclared (first use in this function)
error: ‘EXT2_FEATURE_INCOMPAT_FILETYPE’ undeclared (first use in this function)
error: ‘EXT2_FEATURE_RO_COMPAT_SPARSE_SUPER’ undeclared (first use in this function)
error: ‘EXT2_FT_DIR’ undeclared (first use in this function)
error: ‘EXT2_GOOD_OLD_FIRST_INO’ undeclared (first use in this function)
error: ‘EXT2_MAX_BLOCK_SIZE’ undeclared (first use in this function)
error: ‘EXT2_MIN_BLOCK_LOG_SIZE’ undeclared (first use in this function)
error: ‘EXT2_MIN_BLOCK_SIZE’ undeclared (first use in this function)
error: ‘EXT2_NDIR_BLOCKS’ undeclared (first use in this function)
error: ‘EXT2_OS_LINUX’ undeclared (first use in this function)
error: ‘EXT2_ROOT_INO’ undeclared (first use in this function)
error: ‘EXT2_SUPER_MAGIC’ undeclared (first use in this function)
error: invalid application of ‘sizeof’ to incomplete type ‘struct ext2_inode’
error: invalid use of undefined type ‘struct ext2_group_desc’

struct ext2_inode is complex, I'd prefer to use system header than maintaining my own copy....
Comment 14 Theodore Tso 2012-04-12 15:04:23 UTC
Use the one which is shipped with e2fsprogs.   Better yet, use libext2fs to access the file system, and busybox will be future-proofed against future changes / improvements in the ext 2/3/4 family of file systems.
Comment 15 Artem Bityutskiy 2012-04-13 06:48:52 UTC
(In reply to comment #13)
> For the record: busybox's mkfs_ext2.c uses linux/ext2_fs.h.
> If its include is commented out, the following errors pop out:

The idea is that file-system on-disk layout is stable, so it should be easy for you to have a copy, and you do not need to ever update it unless you add support of a new feature in busybox which requeres an up-to-date header file, and this should be a rare event.

From the FS'es POW - the on-disk layout is not kernel-userspace API so kernel should not export it to user-space. This will also make things easier for the kernel people - we can freely clean-up/rename/shuffle things in the header without being afraid of breaking busybox or any other tool.

Note You need to log in before you can comment on or make changes to this bug.