Remove off64_t from linux.h

PATCH In review Felix Janda 2 participants 5 messages Jun 18, 2016 – Jun 21, 2016

Share & tools

Felix Janda

author

2016-06-18 14:54 UTC #0 lore raw

PATCH 1/4 Remove off64_t from linux.h

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 <[email protected]>

---
 configure.ac    | 2 ++
 include/linux.h | 7 ++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

configure.ac +2 -0

index a4a6cfa..2ea3e50 100644
@@ -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

include/linux.h +6 -1

index cc0f70c..b94de81 100644
@@ -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;

Signature · 6 lines

2.7.3

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2016-06-20 02:05 UTC #1 lore raw

Quoted 4 lines · 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.

"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.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Felix Janda

author

2016-06-20 06:55 UTC #2 lore raw

Dave Chinner wrote:

Thanks for asking for clarification.

Quoted 9 lines · Felix Janda

Quoted 4 lines
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.

Quoted 2 lines

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.

Quoted 3 lines

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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2016-06-20 23:19 UTC #3 lore raw

Quoted 12 lines · Felix Janda

Dave Chinner wrote:
Thanks for asking for clarification.
Quoted 8 lines · Felix Janda
Quoted 4 lines
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"?

Quoted 7 lines

Quoted 1 line
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).

Quoted 8 lines

Quoted 2 lines
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.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Felix Janda

author

2016-06-21 20:09 UTC #4 lore raw

Quoted 15 lines · Dave Chinner

Quoted 14 lines · Felix Janda
Dave Chinner wrote:
Thanks for asking for clarification.
Quoted 9 lines · Felix Janda
Quoted 4 lines
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?

Quoted 8 lines

Quoted 5 lines
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.

Quoted 12 lines

Quoted 8 lines
Quoted 2 lines
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:

Quoted 7 lines

$ 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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs