* [PATCH xfsprogs 2/2] linux.h: Define xfs_off_t as int64_t @ 2016-07-30 13:37 Felix Janda 2016-07-30 16:36 ` Eric Sandeen 2016-08-01 6:24 ` Christoph Hellwig 0 siblings, 2 replies; 13+ messages in thread From: Felix Janda @ 2016-07-30 13:37 UTC (permalink / raw) To: xfs int64_t is guaranteed to have the correct size and signedness and is always avaible because linux.h has a <inttypes.h> include. Fixes compilation error "unkown type name 'off64_t'" on linux when the public header <xfs.h> is included without _LARGEFILE64_SOURCE or _GNU_SOURCE defined. This bug was introduced in commit cb898f157f8410a03cf5f3400baa1df9e5eecd33. Signed-off-by: Felix Janda <felix.janda@posteo.de> --- include/linux.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux.h b/include/linux.h index 5614719..7653cac 100644 --- a/include/linux.h +++ b/include/linux.h @@ -137,7 +137,7 @@ platform_discard_blocks(int fd, uint64_t start, uint64_t len) #define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */ #define EFSBADCRC EBADMSG /* Bad CRC detected */ -typedef off64_t xfs_off_t; +typedef int64_t xfs_off_t; typedef __uint64_t xfs_ino_t; typedef __uint32_t xfs_dev_t; typedef __int64_t xfs_daddr_t; -- 2.7.3 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH xfsprogs 2/2] linux.h: Define xfs_off_t as int64_t 2016-07-30 13:37 [PATCH xfsprogs 2/2] linux.h: Define xfs_off_t as int64_t Felix Janda @ 2016-07-30 16:36 ` Eric Sandeen 2016-08-01 6:24 ` Christoph Hellwig 1 sibling, 0 replies; 13+ messages in thread From: Eric Sandeen @ 2016-07-30 16:36 UTC (permalink / raw) To: xfs On 7/30/16 8:37 AM, Felix Janda wrote: > int64_t is guaranteed to have the correct size and signedness and is > always avaible because linux.h has a <inttypes.h> include. > > Fixes compilation error "unkown type name 'off64_t'" on linux when the > public header <xfs.h> is included without _LARGEFILE64_SOURCE or > _GNU_SOURCE defined. This bug was introduced in commit > cb898f157f8410a03cf5f3400baa1df9e5eecd33. Ok, I think that makes sense. So the progression was: Originally: typedef loff_t xfs_off_t; (But, "musl does not know loff_t") Next: typedef off64_t xfs_off_t; (But, can break compilation w/o special defines) Now: typedef int64_t xfs_off_t; which... I guess... satisfies everyone? A comment about why this, and not loff_t, might be worthwhile. So I have to ask, seeing __int64_t right below this int64_t; what's the difference/point in that? Does this need the __int64_t treatment for any other reason, can you tell? Just trying to avoid a 3rd change down the road. ;) Thanks, -Eric > Signed-off-by: Felix Janda <felix.janda@posteo.de> > --- > include/linux.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux.h b/include/linux.h > index 5614719..7653cac 100644 > --- a/include/linux.h > +++ b/include/linux.h > @@ -137,7 +137,7 @@ platform_discard_blocks(int fd, uint64_t start, uint64_t len) > #define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */ > #define EFSBADCRC EBADMSG /* Bad CRC detected */ > > -typedef off64_t xfs_off_t; > +typedef int64_t xfs_off_t; > typedef __uint64_t xfs_ino_t; > typedef __uint32_t xfs_dev_t; > typedef __int64_t xfs_daddr_t; > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH xfsprogs 2/2] linux.h: Define xfs_off_t as int64_t 2016-07-30 13:37 [PATCH xfsprogs 2/2] linux.h: Define xfs_off_t as int64_t Felix Janda 2016-07-30 16:36 ` Eric Sandeen @ 2016-08-01 6:24 ` Christoph Hellwig 2016-08-01 6:54 ` Felix Janda 1 sibling, 1 reply; 13+ messages in thread From: Christoph Hellwig @ 2016-08-01 6:24 UTC (permalink / raw) To: Felix Janda; +Cc: xfs On Sat, Jul 30, 2016 at 03:37:37PM +0200, Felix Janda wrote: > int64_t is guaranteed to have the correct size and signedness and is > always avaible because linux.h has a <inttypes.h> include. > > Fixes compilation error "unkown type name 'off64_t'" on linux when the > public header <xfs.h> is included without _LARGEFILE64_SOURCE or > _GNU_SOURCE defined. This bug was introduced in commit > cb898f157f8410a03cf5f3400baa1df9e5eecd33. I would much prefer to just define _LARGEFILE64_SOURCE in linux.h.. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH xfsprogs 2/2] linux.h: Define xfs_off_t as int64_t 2016-08-01 6:24 ` Christoph Hellwig @ 2016-08-01 6:54 ` Felix Janda 2016-08-04 0:47 ` Dave Chinner 0 siblings, 1 reply; 13+ messages in thread From: Felix Janda @ 2016-08-01 6:54 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs Christoph Hellwig wrote: > On Sat, Jul 30, 2016 at 03:37:37PM +0200, Felix Janda wrote: > > int64_t is guaranteed to have the correct size and signedness and is > > always avaible because linux.h has a <inttypes.h> include. > > > > Fixes compilation error "unkown type name 'off64_t'" on linux when the > > public header <xfs.h> is included without _LARGEFILE64_SOURCE or > > _GNU_SOURCE defined. This bug was introduced in commit > > cb898f157f8410a03cf5f3400baa1df9e5eecd33. > > I would much prefer to just define _LARGEFILE64_SOURCE in linux.h.. Thanks for the suggestion, but that does not work if the system header defining (or not) off64_t is included before the xfs headers. --- Felix _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH xfsprogs 2/2] linux.h: Define xfs_off_t as int64_t 2016-08-01 6:54 ` Felix Janda @ 2016-08-04 0:47 ` Dave Chinner 2016-08-05 8:02 ` Felix Janda 0 siblings, 1 reply; 13+ messages in thread From: Dave Chinner @ 2016-08-04 0:47 UTC (permalink / raw) To: Felix Janda; +Cc: Christoph Hellwig, xfs On Mon, Aug 01, 2016 at 08:54:10AM +0200, Felix Janda wrote: > Christoph Hellwig wrote: > > On Sat, Jul 30, 2016 at 03:37:37PM +0200, Felix Janda wrote: > > > int64_t is guaranteed to have the correct size and signedness and is > > > always avaible because linux.h has a <inttypes.h> include. > > > > > > Fixes compilation error "unkown type name 'off64_t'" on linux when the > > > public header <xfs.h> is included without _LARGEFILE64_SOURCE or > > > _GNU_SOURCE defined. This bug was introduced in commit > > > cb898f157f8410a03cf5f3400baa1df9e5eecd33. > > > > I would much prefer to just define _LARGEFILE64_SOURCE in linux.h.. > > Thanks for the suggestion, but that does not work if the system header > defining (or not) off64_t is included before the xfs headers. Which, to me, is a build bug in whatever code is including the xfs headers. Isn't it the responsibility of the build environment to ensure the dependencies of the libraries being used are correctly met? Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH xfsprogs 2/2] linux.h: Define xfs_off_t as int64_t 2016-08-04 0:47 ` Dave Chinner @ 2016-08-05 8:02 ` Felix Janda 2016-08-05 11:52 ` Dave Chinner 0 siblings, 1 reply; 13+ messages in thread From: Felix Janda @ 2016-08-05 8:02 UTC (permalink / raw) To: Dave Chinner; +Cc: Christoph Hellwig, xfs Dave Chinner wrote: > On Mon, Aug 01, 2016 at 08:54:10AM +0200, Felix Janda wrote: > > Christoph Hellwig wrote: > > > On Sat, Jul 30, 2016 at 03:37:37PM +0200, Felix Janda wrote: > > > > int64_t is guaranteed to have the correct size and signedness and is > > > > always avaible because linux.h has a <inttypes.h> include. > > > > > > > > Fixes compilation error "unkown type name 'off64_t'" on linux when the > > > > public header <xfs.h> is included without _LARGEFILE64_SOURCE or > > > > _GNU_SOURCE defined. This bug was introduced in commit > > > > cb898f157f8410a03cf5f3400baa1df9e5eecd33. > > > > > > I would much prefer to just define _LARGEFILE64_SOURCE in linux.h.. > > > > Thanks for the suggestion, but that does not work if the system header > > defining (or not) off64_t is included before the xfs headers. > > Which, to me, is a build bug in whatever code is including the xfs > headers. Isn't it the responsibility of the build environment to > ensure the dependencies of the libraries being used are correctly > met? Every program using the xfs header is supposed to know that (only on linux) since commit cb898f157f8410a03cf5f3400baa1df9e5eecd33 it is necessary to define _LARGEFILE64_SOURCE or _GNU_SOURCE? In commit f96aff0c250ae8c70f50d5becb592723eb064f5d something very similar was done. Felix _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH xfsprogs 2/2] linux.h: Define xfs_off_t as int64_t 2016-08-05 8:02 ` Felix Janda @ 2016-08-05 11:52 ` Dave Chinner 2016-08-05 13:09 ` Felix Janda 0 siblings, 1 reply; 13+ messages in thread From: Dave Chinner @ 2016-08-05 11:52 UTC (permalink / raw) To: Felix Janda; +Cc: Christoph Hellwig, xfs On Fri, Aug 05, 2016 at 10:02:41AM +0200, Felix Janda wrote: > Dave Chinner wrote: > > On Mon, Aug 01, 2016 at 08:54:10AM +0200, Felix Janda wrote: > > > Christoph Hellwig wrote: > > > > On Sat, Jul 30, 2016 at 03:37:37PM +0200, Felix Janda wrote: > > > > > int64_t is guaranteed to have the correct size and signedness and is > > > > > always avaible because linux.h has a <inttypes.h> include. > > > > > > > > > > Fixes compilation error "unkown type name 'off64_t'" on linux when the > > > > > public header <xfs.h> is included without _LARGEFILE64_SOURCE or > > > > > _GNU_SOURCE defined. This bug was introduced in commit > > > > > cb898f157f8410a03cf5f3400baa1df9e5eecd33. > > > > > > > > I would much prefer to just define _LARGEFILE64_SOURCE in linux.h.. > > > > > > Thanks for the suggestion, but that does not work if the system header > > > defining (or not) off64_t is included before the xfs headers. > > > > Which, to me, is a build bug in whatever code is including the xfs > > headers. Isn't it the responsibility of the build environment to > > ensure the dependencies of the libraries being used are correctly > > met? > > Every program using the xfs header is supposed to know that (only on > linux) since commit cb898f157f8410a03cf5f3400baa1df9e5eecd33 it is > necessary to define _LARGEFILE64_SOURCE or _GNU_SOURCE? Which, I'd say, most already do, because anything trying to use XFS ioctls needs to be 64 bit offset clean, even on 32 bit systems. I don't see any problem with requiring it when including a header that exposes ioctl interfaces with 64 bit file size/offset fields in them.... Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH xfsprogs 2/2] linux.h: Define xfs_off_t as int64_t 2016-08-05 11:52 ` Dave Chinner @ 2016-08-05 13:09 ` Felix Janda 2016-08-05 22:44 ` Dave Chinner 0 siblings, 1 reply; 13+ messages in thread From: Felix Janda @ 2016-08-05 13:09 UTC (permalink / raw) To: Dave Chinner; +Cc: Christoph Hellwig, xfs Dave Chinner wrote: > On Fri, Aug 05, 2016 at 10:02:41AM +0200, Felix Janda wrote: > > Dave Chinner wrote: > > > On Mon, Aug 01, 2016 at 08:54:10AM +0200, Felix Janda wrote: > > > > Christoph Hellwig wrote: > > > > > On Sat, Jul 30, 2016 at 03:37:37PM +0200, Felix Janda wrote: > > > > > > int64_t is guaranteed to have the correct size and signedness and is > > > > > > always avaible because linux.h has a <inttypes.h> include. > > > > > > > > > > > > Fixes compilation error "unkown type name 'off64_t'" on linux when the > > > > > > public header <xfs.h> is included without _LARGEFILE64_SOURCE or > > > > > > _GNU_SOURCE defined. This bug was introduced in commit > > > > > > cb898f157f8410a03cf5f3400baa1df9e5eecd33. > > > > > > > > > > I would much prefer to just define _LARGEFILE64_SOURCE in linux.h.. > > > > > > > > Thanks for the suggestion, but that does not work if the system header > > > > defining (or not) off64_t is included before the xfs headers. > > > > > > Which, to me, is a build bug in whatever code is including the xfs > > > headers. Isn't it the responsibility of the build environment to > > > ensure the dependencies of the libraries being used are correctly > > > met? > > > > Every program using the xfs header is supposed to know that (only on > > linux) since commit cb898f157f8410a03cf5f3400baa1df9e5eecd33 it is > > necessary to define _LARGEFILE64_SOURCE or _GNU_SOURCE? > > Which, I'd say, most already do, because anything trying to use XFS > ioctls needs to be 64 bit offset clean, even on 32 bit systems. I > don't see any problem with requiring it when including a header > that exposes ioctl interfaces with 64 bit file size/offset fields > in them.... The easiest way to be 64bit clean is to use _FILEOFFSET_BITS=64. Then off_t is 64bit on all architectures and it is impossible to use 32bit interfaces. However the type off64_t will still not be defined... (On the other hand, when just using _LARGEFILE64_SOURCE it is still easy to mix 32 and 64bit interfaces.) Please refer to the "Using LFS Section" of: http://users.suse.com/~aj/linux_lfs.html Cheers, Felix _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH xfsprogs 2/2] linux.h: Define xfs_off_t as int64_t 2016-08-05 13:09 ` Felix Janda @ 2016-08-05 22:44 ` Dave Chinner 2016-08-06 8:38 ` Felix Janda 0 siblings, 1 reply; 13+ messages in thread From: Dave Chinner @ 2016-08-05 22:44 UTC (permalink / raw) To: Felix Janda; +Cc: Christoph Hellwig, xfs On Fri, Aug 05, 2016 at 03:09:05PM +0200, Felix Janda wrote: > Dave Chinner wrote: > > On Fri, Aug 05, 2016 at 10:02:41AM +0200, Felix Janda wrote: > > > Dave Chinner wrote: > > > > On Mon, Aug 01, 2016 at 08:54:10AM +0200, Felix Janda wrote: > > > > > Christoph Hellwig wrote: > > > > > > On Sat, Jul 30, 2016 at 03:37:37PM +0200, Felix Janda wrote: > > > > > > > int64_t is guaranteed to have the correct size and signedness and is > > > > > > > always avaible because linux.h has a <inttypes.h> include. > > > > > > > > > > > > > > Fixes compilation error "unkown type name 'off64_t'" on linux when the > > > > > > > public header <xfs.h> is included without _LARGEFILE64_SOURCE or > > > > > > > _GNU_SOURCE defined. This bug was introduced in commit > > > > > > > cb898f157f8410a03cf5f3400baa1df9e5eecd33. > > > > > > > > > > > > I would much prefer to just define _LARGEFILE64_SOURCE in linux.h.. > > > > > > > > > > Thanks for the suggestion, but that does not work if the system header > > > > > defining (or not) off64_t is included before the xfs headers. > > > > > > > > Which, to me, is a build bug in whatever code is including the xfs > > > > headers. Isn't it the responsibility of the build environment to > > > > ensure the dependencies of the libraries being used are correctly > > > > met? > > > > > > Every program using the xfs header is supposed to know that (only on > > > linux) since commit cb898f157f8410a03cf5f3400baa1df9e5eecd33 it is > > > necessary to define _LARGEFILE64_SOURCE or _GNU_SOURCE? > > > > Which, I'd say, most already do, because anything trying to use XFS > > ioctls needs to be 64 bit offset clean, even on 32 bit systems. I > > don't see any problem with requiring it when including a header > > that exposes ioctl interfaces with 64 bit file size/offset fields > > in them.... > > The easiest way to be 64bit clean is to use _FILEOFFSET_BITS=64. Then > off_t is 64bit on all architectures and it is impossible to use 32bit > interfaces. However the type off64_t will still not be defined... > > (On the other hand, when just using _LARGEFILE64_SOURCE it is still > easy to mix 32 and 64bit interfaces.) Which, with library code, we are likely to see applications using. If you want to clean this up, then remove the dependence on _LARGEFILE64_SOURCE in the entire xfsprogs code base (e.g. it uses lseek64 everywhere which requires off64_t to be defined) and instead make it dependent on _FILEOFFSET_BITS=64. Then you can get rid of all the uses of off64_t completely, and we can break the build if _FILEOFFSET_BITS != 64 on inclusion of xfs.h. Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH xfsprogs 2/2] linux.h: Define xfs_off_t as int64_t 2016-08-05 22:44 ` Dave Chinner @ 2016-08-06 8:38 ` Felix Janda 2016-08-06 9:13 ` Felix Janda 2016-08-06 23:18 ` Dave Chinner 0 siblings, 2 replies; 13+ messages in thread From: Felix Janda @ 2016-08-06 8:38 UTC (permalink / raw) To: Dave Chinner; +Cc: Christoph Hellwig, xfs Dave Chinner wrote: > On Fri, Aug 05, 2016 at 03:09:05PM +0200, Felix Janda wrote: > > Dave Chinner wrote: > > > On Fri, Aug 05, 2016 at 10:02:41AM +0200, Felix Janda wrote: > > > > Dave Chinner wrote: > > > > > On Mon, Aug 01, 2016 at 08:54:10AM +0200, Felix Janda wrote: > > > > > > Christoph Hellwig wrote: > > > > > > > On Sat, Jul 30, 2016 at 03:37:37PM +0200, Felix Janda wrote: > > > > > > > > int64_t is guaranteed to have the correct size and signedness and is > > > > > > > > always avaible because linux.h has a <inttypes.h> include. > > > > > > > > > > > > > > > > Fixes compilation error "unkown type name 'off64_t'" on linux when the > > > > > > > > public header <xfs.h> is included without _LARGEFILE64_SOURCE or > > > > > > > > _GNU_SOURCE defined. This bug was introduced in commit > > > > > > > > cb898f157f8410a03cf5f3400baa1df9e5eecd33. > > > > > > > > > > > > > > I would much prefer to just define _LARGEFILE64_SOURCE in linux.h.. > > > > > > > > > > > > Thanks for the suggestion, but that does not work if the system header > > > > > > defining (or not) off64_t is included before the xfs headers. > > > > > > > > > > Which, to me, is a build bug in whatever code is including the xfs > > > > > headers. Isn't it the responsibility of the build environment to > > > > > ensure the dependencies of the libraries being used are correctly > > > > > met? > > > > > > > > Every program using the xfs header is supposed to know that (only on > > > > linux) since commit cb898f157f8410a03cf5f3400baa1df9e5eecd33 it is > > > > necessary to define _LARGEFILE64_SOURCE or _GNU_SOURCE? > > > > > > Which, I'd say, most already do, because anything trying to use XFS > > > ioctls needs to be 64 bit offset clean, even on 32 bit systems. I > > > don't see any problem with requiring it when including a header > > > that exposes ioctl interfaces with 64 bit file size/offset fields > > > in them.... > > > > The easiest way to be 64bit clean is to use _FILEOFFSET_BITS=64. Then > > off_t is 64bit on all architectures and it is impossible to use 32bit > > interfaces. However the type off64_t will still not be defined... > > > > (On the other hand, when just using _LARGEFILE64_SOURCE it is still > > easy to mix 32 and 64bit interfaces.) > > Which, with library code, we are likely to see applications using. > > If you want to clean this up, then remove the dependence on > _LARGEFILE64_SOURCE in the entire xfsprogs code base (e.g. it uses > lseek64 everywhere which requires off64_t to be defined) and instead > make it dependent on _FILEOFFSET_BITS=64. Then you can get rid of > all the uses of off64_t completely, and we can break the build if > _FILEOFFSET_BITS != 64 on inclusion of xfs.h. Yes, I'd like to clean this up. But first note that you can have both _FILE_OFFSET_BITS=64 and _LARGEFILE64_SOURCE. Then everything (off64_t, lseek64, ...) is defined and everything (off_t, lseek, ...) is 64bit. So to clean up I would first get _FILE_OFFSET_BITS=64 defined and then start "removing 64" from functions/types in any order. *Before modifying the public headers* the sizeof(off_t)=8 check needs to be put into xfs.h. Also note that there are 3 different (but equivalent) off_t types currently used in the code base: off64_t, loff_t and xfs_off_t. Should these be converted to xfs_off_t or off_t? Still, doing these type conversions is going to be pretty invasive and is not unlikely to conflict with outstanding patches. Is now a good time for this? (How about the __uint -> uint, __int -> int conversion?) Thanks, Felix _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH xfsprogs 2/2] linux.h: Define xfs_off_t as int64_t 2016-08-06 8:38 ` Felix Janda @ 2016-08-06 9:13 ` Felix Janda 2016-08-06 23:18 ` Dave Chinner 1 sibling, 0 replies; 13+ messages in thread From: Felix Janda @ 2016-08-06 9:13 UTC (permalink / raw) To: Dave Chinner; +Cc: Christoph Hellwig, xfs Felix Janda wrote: > Dave Chinner wrote: > > On Fri, Aug 05, 2016 at 03:09:05PM +0200, Felix Janda wrote: > > > Dave Chinner wrote: > > > > On Fri, Aug 05, 2016 at 10:02:41AM +0200, Felix Janda wrote: > > > > > Dave Chinner wrote: > > > > > > On Mon, Aug 01, 2016 at 08:54:10AM +0200, Felix Janda wrote: > > > > > > > Christoph Hellwig wrote: > > > > > > > > On Sat, Jul 30, 2016 at 03:37:37PM +0200, Felix Janda wrote: > > > > > > > > > int64_t is guaranteed to have the correct size and signedness and is > > > > > > > > > always avaible because linux.h has a <inttypes.h> include. > > > > > > > > > > > > > > > > > > Fixes compilation error "unkown type name 'off64_t'" on linux when the > > > > > > > > > public header <xfs.h> is included without _LARGEFILE64_SOURCE or > > > > > > > > > _GNU_SOURCE defined. This bug was introduced in commit > > > > > > > > > cb898f157f8410a03cf5f3400baa1df9e5eecd33. > > > > > > > > > > > > > > > > I would much prefer to just define _LARGEFILE64_SOURCE in linux.h.. > > > > > > > > > > > > > > Thanks for the suggestion, but that does not work if the system header > > > > > > > defining (or not) off64_t is included before the xfs headers. > > > > > > > > > > > > Which, to me, is a build bug in whatever code is including the xfs > > > > > > headers. Isn't it the responsibility of the build environment to > > > > > > ensure the dependencies of the libraries being used are correctly > > > > > > met? > > > > > > > > > > Every program using the xfs header is supposed to know that (only on > > > > > linux) since commit cb898f157f8410a03cf5f3400baa1df9e5eecd33 it is > > > > > necessary to define _LARGEFILE64_SOURCE or _GNU_SOURCE? > > > > > > > > Which, I'd say, most already do, because anything trying to use XFS > > > > ioctls needs to be 64 bit offset clean, even on 32 bit systems. I > > > > don't see any problem with requiring it when including a header > > > > that exposes ioctl interfaces with 64 bit file size/offset fields > > > > in them.... > > > > > > The easiest way to be 64bit clean is to use _FILEOFFSET_BITS=64. Then > > > off_t is 64bit on all architectures and it is impossible to use 32bit > > > interfaces. However the type off64_t will still not be defined... > > > > > > (On the other hand, when just using _LARGEFILE64_SOURCE it is still > > > easy to mix 32 and 64bit interfaces.) > > > > Which, with library code, we are likely to see applications using. > > > > If you want to clean this up, then remove the dependence on > > _LARGEFILE64_SOURCE in the entire xfsprogs code base (e.g. it uses > > lseek64 everywhere which requires off64_t to be defined) and instead > > make it dependent on _FILEOFFSET_BITS=64. Then you can get rid of > > all the uses of off64_t completely, and we can break the build if > > _FILEOFFSET_BITS != 64 on inclusion of xfs.h. > > Yes, I'd like to clean this up. > > But first note that you can have both _FILE_OFFSET_BITS=64 and > _LARGEFILE64_SOURCE. Then everything (off64_t, lseek64, ...) is > defined and everything (off_t, lseek, ...) is 64bit. > > So to clean up I would first get _FILE_OFFSET_BITS=64 defined and then Actually I just noticed that xfsprogs has had _FILE_OFFSET_BITS=64 defined for linux (the only platform where this is necessary) since the beginning of xfsprogs-dev git history. Felix _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH xfsprogs 2/2] linux.h: Define xfs_off_t as int64_t 2016-08-06 8:38 ` Felix Janda 2016-08-06 9:13 ` Felix Janda @ 2016-08-06 23:18 ` Dave Chinner 2016-08-07 7:09 ` Felix Janda 1 sibling, 1 reply; 13+ messages in thread From: Dave Chinner @ 2016-08-06 23:18 UTC (permalink / raw) To: Felix Janda; +Cc: Christoph Hellwig, xfs On Sat, Aug 06, 2016 at 10:38:52AM +0200, Felix Janda wrote: > Dave Chinner wrote: > > On Fri, Aug 05, 2016 at 03:09:05PM +0200, Felix Janda wrote: > > > Dave Chinner wrote: > > > > On Fri, Aug 05, 2016 at 10:02:41AM +0200, Felix Janda wrote: > > > > > Dave Chinner wrote: > > > > > > On Mon, Aug 01, 2016 at 08:54:10AM +0200, Felix Janda wrote: > > > > > > > Christoph Hellwig wrote: > > > > > > > > On Sat, Jul 30, 2016 at 03:37:37PM +0200, Felix Janda wrote: > > > > > > > > > int64_t is guaranteed to have the correct size and signedness and is > > > > > > > > > always avaible because linux.h has a <inttypes.h> include. > > > > > > > > > > > > > > > > > > Fixes compilation error "unkown type name 'off64_t'" on linux when the > > > > > > > > > public header <xfs.h> is included without _LARGEFILE64_SOURCE or > > > > > > > > > _GNU_SOURCE defined. This bug was introduced in commit > > > > > > > > > cb898f157f8410a03cf5f3400baa1df9e5eecd33. > > > > > > > > > > > > > > > > I would much prefer to just define _LARGEFILE64_SOURCE in linux.h.. > > > > > > > > > > > > > > Thanks for the suggestion, but that does not work if the system header > > > > > > > defining (or not) off64_t is included before the xfs headers. > > > > > > > > > > > > Which, to me, is a build bug in whatever code is including the xfs > > > > > > headers. Isn't it the responsibility of the build environment to > > > > > > ensure the dependencies of the libraries being used are correctly > > > > > > met? > > > > > > > > > > Every program using the xfs header is supposed to know that (only on > > > > > linux) since commit cb898f157f8410a03cf5f3400baa1df9e5eecd33 it is > > > > > necessary to define _LARGEFILE64_SOURCE or _GNU_SOURCE? > > > > > > > > Which, I'd say, most already do, because anything trying to use XFS > > > > ioctls needs to be 64 bit offset clean, even on 32 bit systems. I > > > > don't see any problem with requiring it when including a header > > > > that exposes ioctl interfaces with 64 bit file size/offset fields > > > > in them.... > > > > > > The easiest way to be 64bit clean is to use _FILEOFFSET_BITS=64. Then > > > off_t is 64bit on all architectures and it is impossible to use 32bit > > > interfaces. However the type off64_t will still not be defined... > > > > > > (On the other hand, when just using _LARGEFILE64_SOURCE it is still > > > easy to mix 32 and 64bit interfaces.) > > > > Which, with library code, we are likely to see applications using. > > > > If you want to clean this up, then remove the dependence on > > _LARGEFILE64_SOURCE in the entire xfsprogs code base (e.g. it uses > > lseek64 everywhere which requires off64_t to be defined) and instead > > make it dependent on _FILEOFFSET_BITS=64. Then you can get rid of > > all the uses of off64_t completely, and we can break the build if > > _FILEOFFSET_BITS != 64 on inclusion of xfs.h. > > Yes, I'd like to clean this up. > > But first note that you can have both _FILE_OFFSET_BITS=64 and > _LARGEFILE64_SOURCE. Then everything (off64_t, lseek64, ...) is > defined and everything (off_t, lseek, ...) is 64bit. > > So to clean up I would first get _FILE_OFFSET_BITS=64 defined and then > start "removing 64" from functions/types in any order. *Before > modifying the public headers* the sizeof(off_t)=8 check needs to > be put into xfs.h. > > Also note that there are 3 different (but equivalent) off_t types > currently used in the code base: off64_t, loff_t and xfs_off_t. > Should these be converted to xfs_off_t or off_t? Not that simple. loff_t has to remain for the copy_file_range() syscall in xfs_io. That syscall requires _GNU_SOURCE and loff_t to be defined from the system headers, so it can't really go away. xfs_off_t is an internal XFS file offset definition, used by the code in libxfs/ and shared with the kernel code, so it can't go away, either. So, essentially, the only code that should change is all the code that uses off64_t - that can use off_t as that's what all the systems that use those variables require... > Still, doing these type conversions is going to be pretty invasive > and is not unlikely to conflict with outstanding patches. Is now > a good time for this? (How about the __uint -> uint, __int -> int > conversion?) off64_t -> off_t affects very little of the new code we have outstanding. It mostly affects xfs_io, so there's little to worry about in terms of merge conflicts here. The __*int conversions are a different matter. They affect the entire code base - they are widespread through the libxfs code so we need to do a kernel code conversion first. Then we can propagate that back into the libxfs code in xfsprogs, and then the rest of xfsprogs can be done. Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH xfsprogs 2/2] linux.h: Define xfs_off_t as int64_t 2016-08-06 23:18 ` Dave Chinner @ 2016-08-07 7:09 ` Felix Janda 0 siblings, 0 replies; 13+ messages in thread From: Felix Janda @ 2016-08-07 7:09 UTC (permalink / raw) To: Dave Chinner; +Cc: Christoph Hellwig, xfs Dave Chinner wrote: > On Sat, Aug 06, 2016 at 10:38:52AM +0200, Felix Janda wrote: > > Dave Chinner wrote: [snip] > > > If you want to clean this up, then remove the dependence on > > > _LARGEFILE64_SOURCE in the entire xfsprogs code base (e.g. it uses > > > lseek64 everywhere which requires off64_t to be defined) and instead > > > make it dependent on _FILEOFFSET_BITS=64. Then you can get rid of > > > all the uses of off64_t completely, and we can break the build if > > > _FILEOFFSET_BITS != 64 on inclusion of xfs.h. > > > > Yes, I'd like to clean this up. > > > > But first note that you can have both _FILE_OFFSET_BITS=64 and > > _LARGEFILE64_SOURCE. Then everything (off64_t, lseek64, ...) is > > defined and everything (off_t, lseek, ...) is 64bit. > > > > So to clean up I would first get _FILE_OFFSET_BITS=64 defined and then > > start "removing 64" from functions/types in any order. *Before > > modifying the public headers* the sizeof(off_t)=8 check needs to > > be put into xfs.h. > > > > Also note that there are 3 different (but equivalent) off_t types > > currently used in the code base: off64_t, loff_t and xfs_off_t. > > Should these be converted to xfs_off_t or off_t? > > Not that simple. loff_t has to remain for the copy_file_range() > syscall in xfs_io. That syscall requires _GNU_SOURCE and loff_t to > be defined from the system headers, so it can't really go away. loff_t is the kernel name for the 64 bit offset type. (The kernel needs to distinguish between off_t and loff_t because it needs to export both 32bit and 64bit syscalls.) Since we are in userspace, we can use the user space off_t, which coincides with kernel loff_t. > xfs_off_t is an internal XFS file offset definition, used by the > code in libxfs/ and shared with the kernel code, so it can't go > away, either. > > So, essentially, the only code that should change is all > the code that uses off64_t - that can use off_t as that's what > all the systems that use those variables require... ok. > > Still, doing these type conversions is going to be pretty invasive > > and is not unlikely to conflict with outstanding patches. Is now > > a good time for this? (How about the __uint -> uint, __int -> int > > conversion?) > > off64_t -> off_t affects very little of the new code we have > outstanding. It mostly affects xfs_io, so there's little to worry > about in terms of merge conflicts here. will send a patch series soon. > The __*int conversions are a different matter. They affect the > entire code base - they are widespread through the libxfs code so we > need to do a kernel code conversion first. Then we can propagate > that back into the libxfs code in xfsprogs, and then the rest of > xfsprogs can be done. ok, I looked into the kernel. The only user of __uint*_t and __int*_t are inside fs/xfs. I can prepare a patch for that. Thanks, Felix _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-08-07 7:10 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-30 13:37 [PATCH xfsprogs 2/2] linux.h: Define xfs_off_t as int64_t Felix Janda 2016-07-30 16:36 ` Eric Sandeen 2016-08-01 6:24 ` Christoph Hellwig 2016-08-01 6:54 ` Felix Janda 2016-08-04 0:47 ` Dave Chinner 2016-08-05 8:02 ` Felix Janda 2016-08-05 11:52 ` Dave Chinner 2016-08-05 13:09 ` Felix Janda 2016-08-05 22:44 ` Dave Chinner 2016-08-06 8:38 ` Felix Janda 2016-08-06 9:13 ` Felix Janda 2016-08-06 23:18 ` Dave Chinner 2016-08-07 7:09 ` Felix Janda
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).