stop generating platform_defs.h V2

stop generating platform_defs.h V2

[Christoph Hellwig]

Hi all,

this series gets rid of the need to generate platform_defs.h from
autoconf.  Let me know what you think.

Changes since v1:
 - check for bytes, not bits

[PATCH 1/5] xfsprogs: remove the ENABLE_GETTEXT substitution in platform_defs.h.in

[Christoph Hellwig]

ENABLE_GETTEXT is already defined on the command line if enabled, no
need to duplicate it in platform_defs.h.in.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/platform_defs.h.in | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/platform_defs.h.in b/include/platform_defs.h.in
index 1f7ceafb..6cc56e31 100644
--- a/include/platform_defs.h.in
+++ b/include/platform_defs.h.in
@@ -36,8 +36,6 @@ typedef struct filldir		filldir_t;
 typedef unsigned short umode_t;
 #endif
 
-/* Define if you want gettext (I18N) support */
-#undef ENABLE_GETTEXT
 #ifdef ENABLE_GETTEXT
 # include <libintl.h>
 # define _(x)                   gettext(x)
-- 
2.24.1

[PATCH 2/5] xfsprogs: remove the SIZEOF_CHAR_P substitution in platform_defs.h.in

[Christoph Hellwig]

SIZEOF_CHAR_P is not used anywhere in the code, so remove the reference
to it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/platform_defs.h.in | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/platform_defs.h.in b/include/platform_defs.h.in
index 6cc56e31..ff0a6a4e 100644
--- a/include/platform_defs.h.in
+++ b/include/platform_defs.h.in
@@ -28,7 +28,6 @@ typedef struct filldir		filldir_t;
 
 /* long and pointer must be either 32 bit or 64 bit */
 #undef SIZEOF_LONG
-#undef SIZEOF_CHAR_P
 #define BITS_PER_LONG (SIZEOF_LONG * CHAR_BIT)
 
 /* Check whether to define umode_t ourselves. */
-- 
2.24.1

[PATCH 3/5] repair: remove BITS_PER_LONG cpp checks in bmap.[ch]

[Christoph Hellwig]

Add a little helper to validate the nex count so that we can use compile
time magic checks for sizeof long directly.  Also don't print the max
in case of an overflow as the value will always be the same.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 repair/bmap.c | 29 +++++++++++++++++++++--------
 repair/bmap.h | 13 -------------
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/repair/bmap.c b/repair/bmap.c
index 44e43ab4..d8da8c95 100644
--- a/repair/bmap.c
+++ b/repair/bmap.c
@@ -22,6 +22,22 @@
 pthread_key_t	dblkmap_key;
 pthread_key_t	ablkmap_key;
 
+/*
+ * For 32 bit platforms, we are limited to extent arrays of 2^31 bytes, which
+ * limits the number of extents in an inode we can check. If we don't limit the
+ * valid range, we can overflow the BLKMAP_SIZE() calculation and allocate less
+ * memory than we think we needed, and hence walk off the end of the array and
+ * corrupt memory.
+ */
+static inline bool
+blkmap_nex_valid(
+	xfs_extnum_t	nex)
+{
+	if (sizeof(long) < 8 && nex >= INT_MAX / sizeof(bmap_ext_t))
+		return false;
+	return true;
+}
+
 blkmap_t *
 blkmap_alloc(
 	xfs_extnum_t	nex,
@@ -35,8 +51,7 @@ blkmap_alloc(
 	if (nex < 1)
 		nex = 1;
 
-#if (BITS_PER_LONG == 32)	/* on 64-bit platforms this is never true */
-	if (nex > BLKMAP_NEXTS_MAX) {
+	if (!blkmap_nex_valid(nex)) {
 		do_warn(
 	_("Number of extents requested in blkmap_alloc (%d) overflows 32 bits.\n"
 	  "If this is not a corruption, then you will need a 64 bit system\n"
@@ -44,7 +59,6 @@ blkmap_alloc(
 			nex);
 		return NULL;
 	}
-#endif
 
 	key = whichfork ? ablkmap_key : dblkmap_key;
 	blkmap = pthread_getspecific(key);
@@ -278,20 +292,19 @@ blkmap_grow(
 		ASSERT(pthread_getspecific(key) == blkmap);
 	}
 
-#if (BITS_PER_LONG == 32)	/* on 64-bit platforms this is never true */
-	if (new_naexts > BLKMAP_NEXTS_MAX) {
+	if (!blkmap_nex_valid(new_naexts)) {
 		do_error(
 	_("Number of extents requested in blkmap_grow (%d) overflows 32 bits.\n"
 	  "You need a 64 bit system to repair this filesystem.\n"),
 			new_naexts);
 		return NULL;
 	}
-#endif
+
 	if (new_naexts <= 0) {
 		do_error(
 	_("Number of extents requested in blkmap_grow (%d) overflowed the\n"
-	  "maximum number of supported extents (%d).\n"),
-			new_naexts, BLKMAP_NEXTS_MAX);
+	  "maximum number of supported extents.\n"),
+			new_naexts);
 		return NULL;
 	}
 
diff --git a/repair/bmap.h b/repair/bmap.h
index 4b588df8..df9602b3 100644
--- a/repair/bmap.h
+++ b/repair/bmap.h
@@ -28,19 +28,6 @@ typedef	struct blkmap {
 #define	BLKMAP_SIZE(n)	\
 	(offsetof(blkmap_t, exts) + (sizeof(bmap_ext_t) * (n)))
 
-/*
- * For 32 bit platforms, we are limited to extent arrays of 2^31 bytes, which
- * limits the number of extents in an inode we can check. If we don't limit the
- * valid range, we can overflow the BLKMAP_SIZE() calculation and allocate less
- * memory than we think we needed, and hence walk off the end of the array and
- * corrupt memory.
- */
-#if BITS_PER_LONG == 32
-#define BLKMAP_NEXTS_MAX	((INT_MAX / sizeof(bmap_ext_t)) - 1)
-#else
-#define BLKMAP_NEXTS_MAX	INT_MAX
-#endif
-
 extern pthread_key_t dblkmap_key;
 extern pthread_key_t ablkmap_key;
 
-- 
2.24.1

[PATCH 4/5] xfsprogs: remove the SIZEOF_LONG substitution in platform_defs.h.in

[Christoph Hellwig]

BITS_PER_LONG is now only checked in C expressions, so we can simply
define it based on sizeof(long).

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/platform_defs.h.in | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/platform_defs.h.in b/include/platform_defs.h.in
index ff0a6a4e..36006cbf 100644
--- a/include/platform_defs.h.in
+++ b/include/platform_defs.h.in
@@ -26,9 +26,7 @@
 
 typedef struct filldir		filldir_t;
 
-/* long and pointer must be either 32 bit or 64 bit */
-#undef SIZEOF_LONG
-#define BITS_PER_LONG (SIZEOF_LONG * CHAR_BIT)
+#define BITS_PER_LONG (sizeof(long) * CHAR_BIT)
 
 /* Check whether to define umode_t ourselves. */
 #ifndef HAVE_UMODE_T
-- 
2.24.1

[PATCH 5/5] xfsprogs: stop generating platform_defs.h

[Christoph Hellwig]

Now that all the autoconf substituations are gone, there is no need
to generate this (and thus any) header.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 .gitignore                                      |  1 -
 Makefile                                        | 15 ++++-----------
 configure.ac                                    |  1 -
 debian/rules                                    |  2 --
 include/Makefile                                |  2 +-
 include/{platform_defs.h.in => platform_defs.h} |  0
 6 files changed, 5 insertions(+), 16 deletions(-)
 rename include/{platform_defs.h.in => platform_defs.h} (100%)

diff --git a/.gitignore b/.gitignore
index fd131b6f..20d033ae 100644
--- a/.gitignore
+++ b/.gitignore
@@ -6,7 +6,6 @@
 # build system
 .census
 .gitcensus
-/include/platform_defs.h
 /include/builddefs
 /install-sh
 
diff --git a/Makefile b/Makefile
index 0edc2700..ff6a977d 100644
--- a/Makefile
+++ b/Makefile
@@ -49,7 +49,7 @@ SRCTARINC = m4/libtool.m4 m4/lt~obsolete.m4 m4/ltoptions.m4 m4/ltsugar.m4 \
            m4/ltversion.m4 po/xfsprogs.pot .gitcensus $(CONFIGURE)
 LDIRT = config.log .ltdep .dep config.status config.cache confdefs.h \
 	conftest* built .census install.* install-dev.* *.gz *.xz \
-	autom4te.cache/* libtool include/builddefs include/platform_defs.h
+	autom4te.cache/* libtool include/builddefs
 
 ifeq ($(HAVE_BUILDDEFS), yes)
 LDIRDIRT = $(SRCDIR)
@@ -84,7 +84,7 @@ endif
 # include is listed last so it is processed last in clean rules.
 SUBDIRS = $(LIBFROG_SUBDIR) $(LIB_SUBDIRS) $(TOOL_SUBDIRS) include
 
-default: include/builddefs include/platform_defs.h
+default: include/builddefs
 ifeq ($(HAVE_BUILDDEFS), no)
 	$(Q)$(MAKE) $(MAKEOPTS) -C . $@
 else
@@ -130,13 +130,6 @@ configure: configure.ac
 include/builddefs: configure
 	./configure $$LOCAL_CONFIGURE_OPTIONS
 
-include/platform_defs.h: include/builddefs
-## Recover from the removal of $@
-	@if test -f $@; then :; else \
-		rm -f include/builddefs; \
-		$(MAKE) $(MAKEOPTS) $(AM_MAKEFLAGS) include/builddefs; \
-	fi
-
 install: $(addsuffix -install,$(SUBDIRS))
 	$(INSTALL) -m 755 -d $(PKG_DOC_DIR)
 	$(INSTALL) -m 644 README $(PKG_DOC_DIR)
@@ -160,14 +153,14 @@ realclean: distclean
 #
 # All this gunk is to allow for a make dist on an unconfigured tree
 #
-dist: include/builddefs include/platform_defs.h default
+dist: include/builddefs default
 ifeq ($(HAVE_BUILDDEFS), no)
 	$(Q)$(MAKE) $(MAKEOPTS) -C . $@
 else
 	$(Q)$(MAKE) $(MAKEOPTS) $(SRCTAR)
 endif
 
-deb: include/builddefs include/platform_defs.h
+deb: include/builddefs
 ifeq ($(HAVE_BUILDDEFS), no)
 	$(Q)$(MAKE) $(MAKEOPTS) -C . $@
 else
diff --git a/configure.ac b/configure.ac
index 5eb7c14b..49c3a466 100644
--- a/configure.ac
+++ b/configure.ac
@@ -3,7 +3,6 @@ AC_PREREQ(2.50)
 AC_CONFIG_AUX_DIR([.])
 AC_CONFIG_MACRO_DIR([m4])
 AC_CONFIG_SRCDIR([include/libxfs.h])
-AC_CONFIG_HEADER(include/platform_defs.h)
 AC_PREFIX_DEFAULT(/usr)
 
 AC_PROG_INSTALL
diff --git a/debian/rules b/debian/rules
index e8509fb3..41c0c004 100755
--- a/debian/rules
+++ b/debian/rules
@@ -43,14 +43,12 @@ config: .census
 	@echo "== dpkg-buildpackage: configure" 1>&2
 	$(checkdir)
 	AUTOHEADER=/bin/true dh_autoreconf
-	$(options) $(MAKE) $(PMAKEFLAGS) include/platform_defs.h
 	touch .census
 
 dibuild:
 	$(checkdir)
 	@echo "== dpkg-buildpackage: installer" 1>&2
 	if [ ! -f mkfs/mkfs.xfs-$(bootpkg) ]; then \
-		$(diopts) $(MAKE) include/platform_defs.h; \
 		mkdir -p include/xfs; \
 		for dir in include libxfs; do \
 			$(MAKE) $(PMAKEFLAGS) -C $$dir NODEP=1 install-headers; \
diff --git a/include/Makefile b/include/Makefile
index a80867e4..c92ecbd5 100644
--- a/include/Makefile
+++ b/include/Makefile
@@ -37,7 +37,7 @@ HFILES = handle.h \
 	xqm.h \
 	xfs_arch.h
 
-LSRCFILES = platform_defs.h.in builddefs.in buildmacros buildrules install-sh
+LSRCFILES = builddefs.in buildmacros buildrules install-sh
 LSRCFILES += $(DKHFILES) $(LIBHFILES)
 LDIRT = disk
 LDIRDIRT = xfs
diff --git a/include/platform_defs.h.in b/include/platform_defs.h
similarity index 100%
rename from include/platform_defs.h.in
rename to include/platform_defs.h
-- 
2.24.1

Re: stop generating platform_defs.h V2

[Darrick J. Wong]

On Wed, Jan 29, 2020 at 07:49:18AM +0100, Christoph Hellwig wrote:
> Hi all,
> 
> this series gets rid of the need to generate platform_defs.h from
> autoconf.  Let me know what you think.
> 
> Changes since v1:
>  - check for bytes, not bits

Gonna be lazy and:
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

for the series rather than indivdually tagging each patch.

--D

Re: [PATCH 3/5] repair: remove BITS_PER_LONG cpp checks in bmap.[ch]

[Darrick J. Wong]

On Sun, Jan 26, 2020 at 12:35:39PM +0100, Christoph Hellwig wrote:
> Add a little helper to validate the nex count so that we can use compile
> time magic checks for sizeof long directly.  Also don't print the max
> in case of an overflow as the value will always be the same.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  repair/bmap.c | 29 +++++++++++++++++++++--------
>  repair/bmap.h | 13 -------------
>  2 files changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/repair/bmap.c b/repair/bmap.c
> index 44e43ab4..0d717cd3 100644
> --- a/repair/bmap.c
> +++ b/repair/bmap.c
> @@ -22,6 +22,22 @@
>  pthread_key_t	dblkmap_key;
>  pthread_key_t	ablkmap_key;
>  
> +/*
> + * For 32 bit platforms, we are limited to extent arrays of 2^31 bytes, which
> + * limits the number of extents in an inode we can check. If we don't limit the
> + * valid range, we can overflow the BLKMAP_SIZE() calculation and allocate less
> + * memory than we think we needed, and hence walk off the end of the array and
> + * corrupt memory.
> + */
> +static inline bool
> +blkmap_nex_valid(
> +	xfs_extnum_t	nex)
> +{
> +	if (sizeof(long) < 64 && nex >= INT_MAX / sizeof(bmap_ext_t))

sizeof(long) < 8

Frankly I suspect the maximum array length on 32 bit platforms is far
less than 2^31 bytes...

--D

> +		return false;
> +	return true;
> +}
> +
>  blkmap_t *
>  blkmap_alloc(
>  	xfs_extnum_t	nex,
> @@ -35,8 +51,7 @@ blkmap_alloc(
>  	if (nex < 1)
>  		nex = 1;
>  
> -#if (BITS_PER_LONG == 32)	/* on 64-bit platforms this is never true */
> -	if (nex > BLKMAP_NEXTS_MAX) {
> +	if (!blkmap_nex_valid(nex)) {
>  		do_warn(
>  	_("Number of extents requested in blkmap_alloc (%d) overflows 32 bits.\n"
>  	  "If this is not a corruption, then you will need a 64 bit system\n"
> @@ -44,7 +59,6 @@ blkmap_alloc(
>  			nex);
>  		return NULL;
>  	}
> -#endif
>  
>  	key = whichfork ? ablkmap_key : dblkmap_key;
>  	blkmap = pthread_getspecific(key);
> @@ -278,20 +292,19 @@ blkmap_grow(
>  		ASSERT(pthread_getspecific(key) == blkmap);
>  	}
>  
> -#if (BITS_PER_LONG == 32)	/* on 64-bit platforms this is never true */
> -	if (new_naexts > BLKMAP_NEXTS_MAX) {
> +	if (!blkmap_nex_valid(new_naexts)) {
>  		do_error(
>  	_("Number of extents requested in blkmap_grow (%d) overflows 32 bits.\n"
>  	  "You need a 64 bit system to repair this filesystem.\n"),
>  			new_naexts);
>  		return NULL;
>  	}
> -#endif
> +
>  	if (new_naexts <= 0) {
>  		do_error(
>  	_("Number of extents requested in blkmap_grow (%d) overflowed the\n"
> -	  "maximum number of supported extents (%d).\n"),
> -			new_naexts, BLKMAP_NEXTS_MAX);
> +	  "maximum number of supported extents.\n"),
> +			new_naexts);
>  		return NULL;
>  	}
>  
> diff --git a/repair/bmap.h b/repair/bmap.h
> index 4b588df8..df9602b3 100644
> --- a/repair/bmap.h
> +++ b/repair/bmap.h
> @@ -28,19 +28,6 @@ typedef	struct blkmap {
>  #define	BLKMAP_SIZE(n)	\
>  	(offsetof(blkmap_t, exts) + (sizeof(bmap_ext_t) * (n)))
>  
> -/*
> - * For 32 bit platforms, we are limited to extent arrays of 2^31 bytes, which
> - * limits the number of extents in an inode we can check. If we don't limit the
> - * valid range, we can overflow the BLKMAP_SIZE() calculation and allocate less
> - * memory than we think we needed, and hence walk off the end of the array and
> - * corrupt memory.
> - */
> -#if BITS_PER_LONG == 32
> -#define BLKMAP_NEXTS_MAX	((INT_MAX / sizeof(bmap_ext_t)) - 1)
> -#else
> -#define BLKMAP_NEXTS_MAX	INT_MAX
> -#endif
> -
>  extern pthread_key_t dblkmap_key;
>  extern pthread_key_t ablkmap_key;
>  
> -- 
> 2.24.1
> 

[PATCH 3/5] repair: remove BITS_PER_LONG cpp checks in bmap.[ch]

[Christoph Hellwig]

Add a little helper to validate the nex count so that we can use compile
time magic checks for sizeof long directly.  Also don't print the max
in case of an overflow as the value will always be the same.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 repair/bmap.c | 29 +++++++++++++++++++++--------
 repair/bmap.h | 13 -------------
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/repair/bmap.c b/repair/bmap.c
index 44e43ab4..0d717cd3 100644
--- a/repair/bmap.c
+++ b/repair/bmap.c
@@ -22,6 +22,22 @@
 pthread_key_t	dblkmap_key;
 pthread_key_t	ablkmap_key;
 
+/*
+ * For 32 bit platforms, we are limited to extent arrays of 2^31 bytes, which
+ * limits the number of extents in an inode we can check. If we don't limit the
+ * valid range, we can overflow the BLKMAP_SIZE() calculation and allocate less
+ * memory than we think we needed, and hence walk off the end of the array and
+ * corrupt memory.
+ */
+static inline bool
+blkmap_nex_valid(
+	xfs_extnum_t	nex)
+{
+	if (sizeof(long) < 64 && nex >= INT_MAX / sizeof(bmap_ext_t))
+		return false;
+	return true;
+}
+
 blkmap_t *
 blkmap_alloc(
 	xfs_extnum_t	nex,
@@ -35,8 +51,7 @@ blkmap_alloc(
 	if (nex < 1)
 		nex = 1;
 
-#if (BITS_PER_LONG == 32)	/* on 64-bit platforms this is never true */
-	if (nex > BLKMAP_NEXTS_MAX) {
+	if (!blkmap_nex_valid(nex)) {
 		do_warn(
 	_("Number of extents requested in blkmap_alloc (%d) overflows 32 bits.\n"
 	  "If this is not a corruption, then you will need a 64 bit system\n"
@@ -44,7 +59,6 @@ blkmap_alloc(
 			nex);
 		return NULL;
 	}
-#endif
 
 	key = whichfork ? ablkmap_key : dblkmap_key;
 	blkmap = pthread_getspecific(key);
@@ -278,20 +292,19 @@ blkmap_grow(
 		ASSERT(pthread_getspecific(key) == blkmap);
 	}
 
-#if (BITS_PER_LONG == 32)	/* on 64-bit platforms this is never true */
-	if (new_naexts > BLKMAP_NEXTS_MAX) {
+	if (!blkmap_nex_valid(new_naexts)) {
 		do_error(
 	_("Number of extents requested in blkmap_grow (%d) overflows 32 bits.\n"
 	  "You need a 64 bit system to repair this filesystem.\n"),
 			new_naexts);
 		return NULL;
 	}
-#endif
+
 	if (new_naexts <= 0) {
 		do_error(
 	_("Number of extents requested in blkmap_grow (%d) overflowed the\n"
-	  "maximum number of supported extents (%d).\n"),
-			new_naexts, BLKMAP_NEXTS_MAX);
+	  "maximum number of supported extents.\n"),
+			new_naexts);
 		return NULL;
 	}
 
diff --git a/repair/bmap.h b/repair/bmap.h
index 4b588df8..df9602b3 100644
--- a/repair/bmap.h
+++ b/repair/bmap.h
@@ -28,19 +28,6 @@ typedef	struct blkmap {
 #define	BLKMAP_SIZE(n)	\
 	(offsetof(blkmap_t, exts) + (sizeof(bmap_ext_t) * (n)))
 
-/*
- * For 32 bit platforms, we are limited to extent arrays of 2^31 bytes, which
- * limits the number of extents in an inode we can check. If we don't limit the
- * valid range, we can overflow the BLKMAP_SIZE() calculation and allocate less
- * memory than we think we needed, and hence walk off the end of the array and
- * corrupt memory.
- */
-#if BITS_PER_LONG == 32
-#define BLKMAP_NEXTS_MAX	((INT_MAX / sizeof(bmap_ext_t)) - 1)
-#else
-#define BLKMAP_NEXTS_MAX	INT_MAX
-#endif
-
 extern pthread_key_t dblkmap_key;
 extern pthread_key_t ablkmap_key;
 
-- 
2.24.1