Re: [PATCH] fuse: use kernel headers when __KERNEL__ is set
On Wed, Apr 17, 2013 at 9:45 PM, Colin Cross wrote: > This fixes my build issue, and doesn't introduce a new make > headers_check warning so you can add my Tested-by: Colin Cross > Thanks for testing. Linus, can you please pull this single fix from: git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git for-linus Thanks, Miklos Miklos Szeredi (1): fuse: fix type definitions in uapi header --- include/uapi/linux/fuse.h | 436 ++--- 1 file changed, 216 insertions(+), 220 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fuse: use kernel headers when __KERNEL__ is set
On Wed, Apr 17, 2013 at 2:57 AM, Miklos Szeredi wrote: > On Tue, Apr 16, 2013 at 01:00:37PM -0700, Colin Cross wrote: > >> Every other uapi header includes linux/types.h to get its type >> definitions, and fuse.h should do the same when compiling for >> userspace targeting linux > > Not necessarily. > > Here's a patch (largish but only search&replace) that should fix all the > issues > and actually removes a hack instead of adding more. > > Thanks, > Miklos > > > From: Miklos Szeredi > Subject: fuse: fix type definitions in uapi header > > Commit 7e98d53086d18c877cb44e9065219335184024de (Synchronize fuse > header with one used in library) added #ifdef __linux__ around > defines if it is not set. The kernel build is self-contained and > can be built on non-Linux toolchains. After the mentioned commit > builds on non-Linux toolchains will try to include stdint.h and > fail due to -nostdinc, and then fail with a bunch of undefined > type errors. > > Fix by checking for __KERNEL__ instead of __linux__ and using the standard int > types instead of the linux specific ones. > > Reported-by: Arve Hjønnevåg > Reported-by: Colin Cross > Signed-off-by: Miklos Szeredi This fixes my build issue, and doesn't introduce a new make headers_check warning so you can add my Tested-by: Colin Cross I still like my patch better though. This patch results in different typedefs being used when compiling in userspace vs. kernel, so you're trusting that the host stdint.h matches the kernel's definitions. In practice I don't see how they would differ, so I guess its up to you. If it were up to me, I would use linux/types.h for kernel and linux userspace, and maybe replace the #defines with typedefs for non-linux userspace builds. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fuse: use kernel headers when __KERNEL__ is set
On Tue, Apr 16, 2013 at 01:00:37PM -0700, Colin Cross wrote: > Every other uapi header includes linux/types.h to get its type > definitions, and fuse.h should do the same when compiling for > userspace targeting linux Not necessarily. Here's a patch (largish but only search&replace) that should fix all the issues and actually removes a hack instead of adding more. Thanks, Miklos From: Miklos Szeredi Subject: fuse: fix type definitions in uapi header Commit 7e98d53086d18c877cb44e9065219335184024de (Synchronize fuse header with one used in library) added #ifdef __linux__ around defines if it is not set. The kernel build is self-contained and can be built on non-Linux toolchains. After the mentioned commit builds on non-Linux toolchains will try to include stdint.h and fail due to -nostdinc, and then fail with a bunch of undefined type errors. Fix by checking for __KERNEL__ instead of __linux__ and using the standard int types instead of the linux specific ones. Reported-by: Arve Hjønnevåg Reported-by: Colin Cross Signed-off-by: Miklos Szeredi --- include/uapi/linux/fuse.h | 436 ++ 1 file changed, 216 insertions(+), 220 deletions(-) --- a/include/uapi/linux/fuse.h +++ b/include/uapi/linux/fuse.h @@ -95,15 +95,10 @@ #ifndef _LINUX_FUSE_H #define _LINUX_FUSE_H -#ifdef __linux__ +#ifdef __KERNEL__ #include #else #include -#define __u64 uint64_t -#define __s64 int64_t -#define __u32 uint32_t -#define __s32 int32_t -#define __u16 uint16_t #endif /* @@ -139,42 +134,42 @@ userspace works under 64bit kernels */ struct fuse_attr { - __u64 ino; - __u64 size; - __u64 blocks; - __u64 atime; - __u64 mtime; - __u64 ctime; - __u32 atimensec; - __u32 mtimensec; - __u32 ctimensec; - __u32 mode; - __u32 nlink; - __u32 uid; - __u32 gid; - __u32 rdev; - __u32 blksize; - __u32 padding; + uint64_tino; + uint64_tsize; + uint64_tblocks; + uint64_tatime; + uint64_tmtime; + uint64_tctime; + uint32_tatimensec; + uint32_tmtimensec; + uint32_tctimensec; + uint32_tmode; + uint32_tnlink; + uint32_tuid; + uint32_tgid; + uint32_trdev; + uint32_tblksize; + uint32_tpadding; }; struct fuse_kstatfs { - __u64 blocks; - __u64 bfree; - __u64 bavail; - __u64 files; - __u64 ffree; - __u32 bsize; - __u32 namelen; - __u32 frsize; - __u32 padding; - __u32 spare[6]; + uint64_tblocks; + uint64_tbfree; + uint64_tbavail; + uint64_tfiles; + uint64_tffree; + uint32_tbsize; + uint32_tnamelen; + uint32_tfrsize; + uint32_tpadding; + uint32_tspare[6]; }; struct fuse_file_lock { - __u64 start; - __u64 end; - __u32 type; - __u32 pid; /* tgid */ + uint64_tstart; + uint64_tend; + uint32_ttype; + uint32_tpid; /* tgid */ }; /** @@ -364,143 +359,143 @@ enum fuse_notify_code { #define FUSE_COMPAT_ENTRY_OUT_SIZE 120 struct fuse_entry_out { - __u64 nodeid; /* Inode ID */ - __u64 generation; /* Inode generation: nodeid:gen must - be unique for the fs's lifetime */ - __u64 entry_valid;/* Cache timeout for the name */ - __u64 attr_valid; /* Cache timeout for the attributes */ - __u32 entry_valid_nsec; - __u32 attr_valid_nsec; + uint64_tnodeid; /* Inode ID */ + uint64_tgeneration; /* Inode generation: nodeid:gen must + be unique for the fs's lifetime */ + uint64_tentry_valid;/* Cache timeout for the name */ + uint64_tattr_valid; /* Cache timeout for the attributes */ + uint32_tentry_valid_nsec; + uint32_tattr_valid_nsec; struct fuse_attr attr; }; struct fuse_forget_in { - __u64 nlookup; + uint64_tnlookup; }; struct fuse_forget_one { - __u64 nodeid; - __u64 nlookup; + uint64_tnodeid; + uint64_tnlookup; }; struct fuse_batch_forget_in { - __u32 count; - __u32 dummy; + uint32_tcount; + uint32_tdummy; }; struct fuse_getattr_in { - __u32 getattr_flags; - __u32 dummy; - __u64 fh; + uint32_tgetattr_flags; + uint32_tdummy; + uint64_tfh; }; #define FUSE_COMPAT_ATTR_OUT_SIZE 96 struct fuse_attr_
Re: [PATCH] fuse: use kernel headers when __KERNEL__ is set
On Tue, Apr 16, 2013 at 12:11 PM, Miklos Szeredi wrote: > On Tue, Apr 16, 2013 at 8:29 PM, Colin Cross wrote: >> Dropping __linux__ causes a make headers_check warning, which the >> kbuild test robot reported this morning: >> usr/include/linux/fuse.h:99: found __[us]{8,16,32,64} type without >> #include >> Using my patch without modification does not cause that warning. >> >> linux/types.h defines the types that are used to communicate between >> the kernel and userspace. Redefining those in each header makes no >> sense, and will probably cause redefined types warnings if you compile >> a userspace file that includes fuse.h and another uapi header that >> properly includes linux/types.h. > > is linux specific while the fuse API is cross > platform. So the userspace header obviously has to use some cross > platform type definitions. Making the type definitions depend on > __linux__ is probably just as unreliable in userspace as it is in the > kernel, so why complicate things with that? I'm not sure what you mean by __linux__ being unreliable, it is set by any toolchain that is compield with linux as its target. It should be very reliable in userspace, its only unreliable in the kernel because the kernel doesn't rely on any specific os target in the toolchain, it is self contained. With your change, compiling a program that includes linux/fuse.h and linux/icmp.h against the headers installed by make headers_installed results in some strange preprocessor results. The lines in int-ll64.h, which is what normally defines the userspace types, are: typedef __signed__ char __s8; typedef unsigned char __u8; typedef __signed__ short __s16; typedef unsigned short __u16; typedef __signed__ int __s32; typedef unsigned int __u32; After preprocessing with your fuse.h included before icmp.h (which includes linux/types.h, which eventually includes asm-generic/int-ll64.h): typedef __signed__ char __s8; typedef unsigned char __u8; typedef __signed__ short __s16; typedef unsigned short uint16_t; typedef __signed__ int int32_t; typedef unsigned int uint32_t; The first three lines are the kernel headers providing typedefs for the types that are not used in fuse.h, and therefore not covered by the #define hacks there. The last three lines are a problem: now you have redefined the stdint types to match the kernel's types, better hope they match the host's stdint definitions or you'll get redefined type warnings. Every other uapi header includes linux/types.h to get its type definitions, and fuse.h should do the same when compiling for userspace targeting linux or when compiling the kernel. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fuse: use kernel headers when __KERNEL__ is set
On Tue, Apr 16, 2013 at 8:29 PM, Colin Cross wrote: > Dropping __linux__ causes a make headers_check warning, which the > kbuild test robot reported this morning: > usr/include/linux/fuse.h:99: found __[us]{8,16,32,64} type without > #include > Using my patch without modification does not cause that warning. > > linux/types.h defines the types that are used to communicate between > the kernel and userspace. Redefining those in each header makes no > sense, and will probably cause redefined types warnings if you compile > a userspace file that includes fuse.h and another uapi header that > properly includes linux/types.h. is linux specific while the fuse API is cross platform. So the userspace header obviously has to use some cross platform type definitions. Making the type definitions depend on __linux__ is probably just as unreliable in userspace as it is in the kernel, so why complicate things with that? Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fuse: use kernel headers when __KERNEL__ is set
On Tue, Apr 16, 2013 at 11:21 AM, Linus Torvalds wrote: >> What I meant is IF is included by userspace (it sure is >> meant to be included and *is* included by libfuse and other stuff) >> THEN using instead of is fine regardless of >> whether __linux__ is defined or not. > > That's probably true. But the patch in question adds the __KERNEL__ > test, and *that* seems required. > > If you think that we should instead drop the __linux__ test, than yes, > that part sounds fine. I thought that by "linux internal header" you > meant the fuse.h file, but you seem to mean the indirectly included > . That's fine. > > Linus Dropping __linux__ causes a make headers_check warning, which the kbuild test robot reported this morning: usr/include/linux/fuse.h:99: found __[us]{8,16,32,64} type without #include Using my patch without modification does not cause that warning. linux/types.h defines the types that are used to communicate between the kernel and userspace. Redefining those in each header makes no sense, and will probably cause redefined types warnings if you compile a userspace file that includes fuse.h and another uapi header that properly includes linux/types.h. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fuse: use kernel headers when __KERNEL__ is set
> What I meant is IF is included by userspace (it sure is > meant to be included and *is* included by libfuse and other stuff) > THEN using instead of is fine regardless of > whether __linux__ is defined or not. That's probably true. But the patch in question adds the __KERNEL__ test, and *that* seems required. If you think that we should instead drop the __linux__ test, than yes, that part sounds fine. I thought that by "linux internal header" you meant the fuse.h file, but you seem to mean the indirectly included . That's fine. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fuse: use kernel headers when __KERNEL__ is set
On Tue, Apr 16, 2013 at 5:59 PM, Linus Torvalds wrote: > On Tue, Apr 16, 2013 at 7:21 AM, Miklos Szeredi wrote: >> >> And I still disagree. Why should userspace use the linux internal >> header when there's a perfectly good standard header that it can use? > > If it's called UAPI, it damn well is *meant* for user-space inclusion. > Look at the file-name. > > And since the bug comment says "This file defines the kernel interface > of FUSE" *AND* it very clearly has explicit code to support user-space > includes with special user-space-only type defines, then your email is > obviously just pure crap, and I don't understand how you can write > that sentence with a straight face. I think I meant something different by that sentence than what you think I meant :) What I meant is IF is included by userspace (it sure is meant to be included and *is* included by libfuse and other stuff) THEN using instead of is fine regardless of whether __linux__ is defined or not. Does that sound better, or is there still something we disagree about? Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fuse: use kernel headers when __KERNEL__ is set
On Tue, Apr 16, 2013 at 7:21 AM, Miklos Szeredi wrote: > > And I still disagree. Why should userspace use the linux internal > header when there's a perfectly good standard header that it can use? If it's called UAPI, it damn well is *meant* for user-space inclusion. Look at the file-name. And since the bug comment says "This file defines the kernel interface of FUSE" *AND* it very clearly has explicit code to support user-space includes with special user-space-only type defines, then your email is obviously just pure crap, and I don't understand how you can write that sentence with a straight face. The *whole* point of the UAPI includes is two-fold: - to make it easier for user-space libraries to get at the kernel definitions. Not everybody wants to use glibc for various reasons, and where do you want people to *get* these declarations from? - to make kernel people more AWARE of when they are changing stuff that affects user-space. Now, the uapi model not perfect, but there are damn good reasons to at least *strive* for both of those things, so I really don't understand your comment there. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fuse: use kernel headers when __KERNEL__ is set
On Mon, Apr 15, 2013 at 11:47 PM, Colin Cross wrote: > On Mon, Apr 15, 2013 at 1:41 PM, Colin Cross wrote: >> Commit 7e98d53086d18c877cb44e9065219335184024de (Synchronize fuse >> header with one used in library) added #ifdef __linux__ around >> defines if it is not set. The kernel build is self-contained and >> can be built on non-Linux toolchains. After the mentioned commit >> builds on non-Linux toolchains will try to include stdint.h and >> fail due to -nostdinc, and then fail with a bunch of undefined >> type errors. >> >> Change the #ifdef to check for __linux__ or __KERNEL__ so that >> it uses the kernel typedefs if __KERNEL__ is set. >> >> Signed-off-by: Colin Cross >> --- >> I think this should go in v3.9, without it bare-metal toolchains >> that don't define __linux__ will fail to compile the kernel, and >> cross-compiles from non-linux hosts will probably also fail. > > Miklos, I see Arve sent an equivalent patch to you a month ago > (https://lkml.org/lkml/2013/3/11/620), and I agree with his response > to your question (checking for __linux__ and __KERNEL__ is better than > just checking for __KERNEL__ if you want this header to be used in > userspace builds on linux targets). And I still disagree. Why should userspace use the linux internal header when there's a perfectly good standard header that it can use? Patch committed and pushed to for-next and for-linus. I'll push to Linus tomorrow. Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fuse: use kernel headers when __KERNEL__ is set
On Mon, Apr 15, 2013 at 1:41 PM, Colin Cross wrote: > Commit 7e98d53086d18c877cb44e9065219335184024de (Synchronize fuse > header with one used in library) added #ifdef __linux__ around > defines if it is not set. The kernel build is self-contained and > can be built on non-Linux toolchains. After the mentioned commit > builds on non-Linux toolchains will try to include stdint.h and > fail due to -nostdinc, and then fail with a bunch of undefined > type errors. > > Change the #ifdef to check for __linux__ or __KERNEL__ so that > it uses the kernel typedefs if __KERNEL__ is set. > > Signed-off-by: Colin Cross > --- > I think this should go in v3.9, without it bare-metal toolchains > that don't define __linux__ will fail to compile the kernel, and > cross-compiles from non-linux hosts will probably also fail. Miklos, I see Arve sent an equivalent patch to you a month ago (https://lkml.org/lkml/2013/3/11/620), and I agree with his response to your question (checking for __linux__ and __KERNEL__ is better than just checking for __KERNEL__ if you want this header to be used in userspace builds on linux targets). Can you pull one of these two patches before 3.9? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/