[PATCH 1/4] Remove off64_t from linux.h

[PATCH 1/4] Remove off64_t from linux.h

[Felix Janda]

The off64_t type is usually only conditionally exposed under the
feature test macro _LARGEFILE64_SOURCE (also defined by _GNU_SOURCE).
To make the public xfs headers more standalone therefore off64_t should
be avoided.

Enable transparent large file support via configure so that the
standard off_t is the same as off64_t. Make compilation of linux
programs using libxfs but without large file support fail. Now off64_t
can safely be replaced by off_t in linux.h.

Signed-off-by: Felix Janda <felix.janda@posteo.de>
---
 configure.ac    | 2 ++
 include/linux.h | 7 ++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index a4a6cfa..2ea3e50 100644
--- a/configure.ac
+++ b/configure.ac
@@ -107,6 +107,8 @@ AC_PACKAGE_UTILITIES(xfsprogs)
 AC_MULTILIB($enable_lib64)
 AC_RT($enable_librt)
 
+AC_SYS_LARGEFILE
+
 AC_PACKAGE_NEED_UUID_H
 AC_PACKAGE_NEED_UUIDCOMPARE
 
diff --git a/include/linux.h b/include/linux.h
index cc0f70c..b94de81 100644
--- a/include/linux.h
+++ b/include/linux.h
@@ -142,7 +142,12 @@ 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;
+/* Make compilation fail when off_t has 32 bits */
+struct __xfs_assert_64bit_off_t {
+	int assert_64bit_off_t[sizeof(off_t)-8];
+};
+
+typedef off_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

Re: [PATCH 1/4] Remove off64_t from linux.h

[Dave Chinner]

On Sat, Jun 18, 2016 at 04:52:38PM +0200, Felix Janda wrote:
> The off64_t type is usually only conditionally exposed under the
> feature test macro _LARGEFILE64_SOURCE (also defined by _GNU_SOURCE).
> To make the public xfs headers more standalone therefore off64_t should
> be avoided.

"more standalone"?

What does that mean? And what does it mean for all the xfsprogs code
that still uses off64_t?

i.e. if you are going to make xfsprogs fail to compile on configs
that don't define off64_t, then it makes no sense to leave all the
users of off64_t in the xfsprogs code....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/4] Remove off64_t from linux.h

[Felix Janda]

Dave Chinner wrote:

Thanks for asking for clarification.

> On Sat, Jun 18, 2016 at 04:52:38PM +0200, Felix Janda wrote:
> > The off64_t type is usually only conditionally exposed under the
> > feature test macro _LARGEFILE64_SOURCE (also defined by _GNU_SOURCE).
> > To make the public xfs headers more standalone therefore off64_t should
> > be avoided.
> 
> "more standalone"?
> 
> What does that mean?

Programs including the xfs headers while not defining _GNU_SOURCE or
_LARGEFILE64_SOURCE will not fail with compile errors. My previous
patch changing loff_t to off64_t had the unintented consequences that
downstreams of xfs-progs like ceph had to define _LARGEFILE64_SOURCE
on linux.

> And what does it mean for all the xfsprogs code that still uses
> off64_t?

off_t and off64_t are now synomyms and 64 bit on all architectures.
So no difference for code using off64_t.

Under some conditions there can be a difference for code using
off_t.

> i.e. if you are going to make xfsprogs fail to compile on configs
> that don't define off64_t, then it makes no sense to leave all the
> users of off64_t in the xfsprogs code....

On all supported systems except for current default linux (glibc)
configurations either there does not exist off64_t or it is the same
as off_t. The configure test will define _FILE_OFFSET_BITS=64 on linux
systems (i.e. enabling transparent large file support) with the result
that now even on linux off_t is 64 bit wide in all cases.

The "assert" ensures that programs using the xfs headers do not
compile when the expectation sizeof(off_t)=8 is not met. This makes
programs on 32 bit linux systems using the xfs headers but not
defining _FILE_OFFSET_BITS=64 not compile. I would consider this as
beneficial because such programs easily can have bugs regarding
support of files of size >2GB on 32 bit systems.

Felix

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/4] Remove off64_t from linux.h

[Dave Chinner]

On Mon, Jun 20, 2016 at 08:53:48AM +0200, Felix Janda wrote:
> Dave Chinner wrote:
> 
> Thanks for asking for clarification.
> 
> > On Sat, Jun 18, 2016 at 04:52:38PM +0200, Felix Janda wrote:
> > > The off64_t type is usually only conditionally exposed under the
> > > feature test macro _LARGEFILE64_SOURCE (also defined by _GNU_SOURCE).
> > > To make the public xfs headers more standalone therefore off64_t should
> > > be avoided.
> > 
> > "more standalone"?
> > 
> > What does that mean?
> 
> Programs including the xfs headers while not defining _GNU_SOURCE or
> _LARGEFILE64_SOURCE will not fail with compile errors. My previous
> patch changing loff_t to off64_t had the unintented consequences that
> downstreams of xfs-progs like ceph had to define _LARGEFILE64_SOURCE
> on linux.

That needs to be in the patch description - it's the motivation for
the change (i.e. that downstream apps need to add new defines).

> > And what does it mean for all the xfsprogs code that still uses
> > off64_t?
> 
> off_t and off64_t are now synomyms and 64 bit on all architectures.
> So no difference for code using off64_t.
> 
> Under some conditions there can be a difference for code using
> off_t.

Right, I understand that there is a difference - what I'm asking for
is a description of the difference and an explanation of why:

$ git grep off64_t | wc -l
62
$

the other ~60 uses of off64_t in the xfsprogs code are not being
removed, too. i.e. if the code now won't compile if off_t isn't 64
bits, then why keep off64_t at all in any of the code?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/4] Remove off64_t from linux.h

[Felix Janda]

Dave Chinner wrote:
> On Mon, Jun 20, 2016 at 08:53:48AM +0200, Felix Janda wrote:
> > Dave Chinner wrote:
> > 
> > Thanks for asking for clarification.
> > 
> > > On Sat, Jun 18, 2016 at 04:52:38PM +0200, Felix Janda wrote:
> > > > The off64_t type is usually only conditionally exposed under the
> > > > feature test macro _LARGEFILE64_SOURCE (also defined by _GNU_SOURCE).
> > > > To make the public xfs headers more standalone therefore off64_t should
> > > > be avoided.
> > > 
> > > "more standalone"?
> > > 
> > > What does that mean?
> > 
> > Programs including the xfs headers while not defining _GNU_SOURCE or
> > _LARGEFILE64_SOURCE will not fail with compile errors. My previous
> > patch changing loff_t to off64_t had the unintented consequences that
> > downstreams of xfs-progs like ceph had to define _LARGEFILE64_SOURCE
> > on linux.
> 
> That needs to be in the patch description - it's the motivation for
> the change (i.e. that downstream apps need to add new defines).

Sure.

> > > And what does it mean for all the xfsprogs code that still uses
> > > off64_t?
> > 
> > off_t and off64_t are now synomyms and 64 bit on all architectures.
> > So no difference for code using off64_t.
> > 
> > Under some conditions there can be a difference for code using
> > off_t.
> 
> Right, I understand that there is a difference - what I'm asking for
> is a description of the difference and an explanation of why:
> 
> $ git grep off64_t | wc -l
> 62
> $
> 
> the other ~60 uses of off64_t in the xfsprogs code are not being
> removed, too. i.e. if the code now won't compile if off_t isn't 64
> bits, then why keep off64_t at all in any of the code?

On 32bit systems using glibc the following changes happen:

1. off_t is now 64 bit instead of 32 bit
2. all functions and structures using off_t are mapped to versions
   using the 64 bit off_t

1. means that off_t and off64_t are now equivalents, whereas 2. means
that struct stat and struct stat64, open() and open64() and many others
become equivalent.

Because, in addition to off64_t, "64"-functions and structures are
used very consistently in the code I have hold off sending a patch
changing these. I recall also seeing in some commit messages that at
some sites off64_t was changed to int64_t (or __int64_t...) for header
portability; for these sites it might be nice to change back to off_t.

Grepping the code I found one occurence in fsr/xfs_fsr.c of off_t (not
off64_t) which will be affected by the change (on 32bit linux). Right
now the F_GETLK fcntl with the struct flock is used there instead of
F_GETLK64 with struct flock64. The current usage is ok because the
off_t arguments are only used for the value 0 which happily fits into
variables of any size. After the change, the fcntl F_GETLK64, struct
flock64 and off64_t will be used instead, leading to no visible change
in behavior.

Felix

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs