[xfsprogs,2/2] linux.h: Define xfs_off_t as int64_t
diff mbox

Message ID 20160730133737.GB5895@nyan
State Rejected
Headers show

Commit Message

Felix Janda July 30, 2016, 1:37 p.m. UTC
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(-)

Comments

Eric Sandeen July 30, 2016, 4:36 p.m. UTC | #1
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;
>
hch@infradead.org Aug. 1, 2016, 6:24 a.m. UTC | #2
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..
Felix Janda Aug. 1, 2016, 6:54 a.m. UTC | #3
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
Dave Chinner Aug. 4, 2016, 12:47 a.m. UTC | #4
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.
Felix Janda Aug. 5, 2016, 8:02 a.m. UTC | #5
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
Dave Chinner Aug. 5, 2016, 11:52 a.m. UTC | #6
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.
Felix Janda Aug. 5, 2016, 1:09 p.m. UTC | #7
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
Dave Chinner Aug. 5, 2016, 10:44 p.m. UTC | #8
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.
Felix Janda Aug. 6, 2016, 8:38 a.m. UTC | #9
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
Felix Janda Aug. 6, 2016, 9:13 a.m. UTC | #10
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
Dave Chinner Aug. 6, 2016, 11:18 p.m. UTC | #11
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.
Felix Janda Aug. 7, 2016, 7:09 a.m. UTC | #12
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

Patch
diff mbox

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;