[PATCH 0/2] xfsprogs: "Fix" --disable-static option

[PATCH 0/2] xfsprogs: "Fix" --disable-static option

[Dave Chinner]

Hi folks,

These patches fix the build failures that come from trying
to prevent static xfsprogs libraries being built via the 
'configure --disable-static' option.

The internal libraries are statically linked to the xfsprogs
binaries, with external dependencies then dynamically linked. Not
building the internal libraries statically results in linking them
dynamically and so the pull in all their dependencies as well. for
example, libfrog has the blkid topology code in it, so when linked
as a shared library it always brings that dependency with it,
regardless of whether the binary actually uses the topology code
or not.

Linking the libraries statically allows dead code elimination to
remove the topology code if it is not used by the binary, hence
eliminating the need to link that binary against libblkid. This
makes the binaries smaller, they load faster, and we don't have to
ship public libraries for all the internal xfsprogs code.

Hence we need to force the internal libraries to build a staticly
linked archive even when a user says "--disable-static". We can do
this by passing libtool the "-static" option on the link link for
each of the internal libraries. Hence we always build static
libraries for the internel libs as the build needs them to be
static.

Doing this, however, exposes the fact taht mkfs has it's own version
of cvtnum and it links against libfrog which also has version of
cvtnum. The linker previously resolved this automatically by using
the local version, but with -static defined it errors out on
multiply-defined symbol errors. SO the first patch fixes the cvtnum
issue. Note i left xfs_estimate alone - it doesn't link against
libfrog, so I didn't remove it's simple cvtnum copy as it's not
necessary to fix the build issue and linking against libfrog might
introduce other issues.

Cheers,

Dave.

[PATCH 1/2] mkfs: use cvtnum from libfrog

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Move the checks for zero block/sector size to the libfrog code
and return -1LL as an invalid value instead. Catch the invalid
value in mkfs and error out there instead of inside cvtnum.

Also rename the libfrog block/sector size variables so they don't
shadow the mkfs global variables of the same name.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 libfrog/convert.c | 12 +++++---
 mkfs/xfs_mkfs.c   | 71 ++++-------------------------------------------
 2 files changed, 14 insertions(+), 69 deletions(-)

diff --git a/libfrog/convert.c b/libfrog/convert.c
index 8d4d4077b331..b5f3fc1238dd 100644
--- a/libfrog/convert.c
+++ b/libfrog/convert.c
@@ -182,8 +182,8 @@ cvt_u16(
 
 long long
 cvtnum(
-	size_t		blocksize,
-	size_t		sectorsize,
+	size_t		blksize,
+	size_t		sectsize,
 	char		*s)
 {
 	long long	i;
@@ -202,9 +202,13 @@ cvtnum(
 	c = tolower(*sp);
 	switch (c) {
 	case 'b':
-		return i * blocksize;
+		if (!blksize)
+			return -1LL;
+		return i * blksize;
 	case 's':
-		return i * sectorsize;
+		if (!sectsize)
+			return -1LL;
+		return i * sectsize;
 	case 'k':
 		return KILOBYTES(i);
 	case 'm':
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 0adaa65d19f8..04063ca5b2c7 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -942,69 +942,6 @@ unknown(
 	usage();
 }
 
-long long
-cvtnum(
-	unsigned int	blksize,
-	unsigned int	sectsize,
-	const char	*s)
-{
-	long long	i;
-	char		*sp;
-	int		c;
-
-	i = strtoll(s, &sp, 0);
-	if (i == 0 && sp == s)
-		return -1LL;
-	if (*sp == '\0')
-		return i;
-
-	if (sp[1] != '\0')
-		return -1LL;
-
-	if (*sp == 'b') {
-		if (!blksize) {
-			fprintf(stderr,
-_("Blocksize must be provided prior to using 'b' suffix.\n"));
-			usage();
-		} else {
-			return i * blksize;
-		}
-	}
-	if (*sp == 's') {
-		if (!sectsize) {
-			fprintf(stderr,
-_("Sectorsize must be specified prior to using 's' suffix.\n"));
-			usage();
-		} else {
-			return i * sectsize;
-		}
-	}
-
-	c = tolower(*sp);
-	switch (c) {
-	case 'e':
-		i *= 1024LL;
-		/* fall through */
-	case 'p':
-		i *= 1024LL;
-		/* fall through */
-	case 't':
-		i *= 1024LL;
-		/* fall through */
-	case 'g':
-		i *= 1024LL;
-		/* fall through */
-	case 'm':
-		i *= 1024LL;
-		/* fall through */
-	case 'k':
-		return i * 1024LL;
-	default:
-		break;
-	}
-	return -1LL;
-}
-
 static void
 check_device_type(
 	const char	*name,
@@ -1347,9 +1284,13 @@ getnum(
 	 * convert it ourselves to guarantee there is no trailing garbage in the
 	 * number.
 	 */
-	if (sp->convert)
+	if (sp->convert) {
 		c = cvtnum(blocksize, sectorsize, str);
-	else {
+		if (c == -1LL) {
+			illegal_option(str, opts, index,
+				_("Not a valid value or illegal suffix"));
+		}
+	} else {
 		char		*str_end;
 
 		c = strtoll(str, &str_end, 0);
-- 
2.23.0.rc1

Re: [PATCH 1/2] mkfs: use cvtnum from libfrog

[Darrick J. Wong]

On Tue, Aug 13, 2019 at 03:14:19PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Move the checks for zero block/sector size to the libfrog code
> and return -1LL as an invalid value instead. Catch the invalid
> value in mkfs and error out there instead of inside cvtnum.
> 
> Also rename the libfrog block/sector size variables so they don't
> shadow the mkfs global variables of the same name.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  libfrog/convert.c | 12 +++++---
>  mkfs/xfs_mkfs.c   | 71 ++++-------------------------------------------
>  2 files changed, 14 insertions(+), 69 deletions(-)
> 
> diff --git a/libfrog/convert.c b/libfrog/convert.c
> index 8d4d4077b331..b5f3fc1238dd 100644
> --- a/libfrog/convert.c
> +++ b/libfrog/convert.c
> @@ -182,8 +182,8 @@ cvt_u16(
>  
>  long long
>  cvtnum(
> -	size_t		blocksize,
> -	size_t		sectorsize,
> +	size_t		blksize,
> +	size_t		sectsize,
>  	char		*s)
>  {
>  	long long	i;
> @@ -202,9 +202,13 @@ cvtnum(
>  	c = tolower(*sp);
>  	switch (c) {
>  	case 'b':
> -		return i * blocksize;
> +		if (!blksize)
> +			return -1LL;
> +		return i * blksize;
>  	case 's':
> -		return i * sectorsize;
> +		if (!sectsize)
> +			return -1LL;
> +		return i * sectsize;
>  	case 'k':
>  		return KILOBYTES(i);
>  	case 'm':
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 0adaa65d19f8..04063ca5b2c7 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -942,69 +942,6 @@ unknown(
>  	usage();
>  }
>  
> -long long
> -cvtnum(
> -	unsigned int	blksize,
> -	unsigned int	sectsize,
> -	const char	*s)
> -{
> -	long long	i;
> -	char		*sp;
> -	int		c;
> -
> -	i = strtoll(s, &sp, 0);
> -	if (i == 0 && sp == s)
> -		return -1LL;
> -	if (*sp == '\0')
> -		return i;
> -
> -	if (sp[1] != '\0')
> -		return -1LL;
> -
> -	if (*sp == 'b') {
> -		if (!blksize) {
> -			fprintf(stderr,
> -_("Blocksize must be provided prior to using 'b' suffix.\n"));
> -			usage();
> -		} else {
> -			return i * blksize;
> -		}
> -	}
> -	if (*sp == 's') {
> -		if (!sectsize) {
> -			fprintf(stderr,
> -_("Sectorsize must be specified prior to using 's' suffix.\n"));

Hmm, so this message is replaced with "Not a valid value or illegal suffix"?

That's not anywhere near as helpful as the old message... maybe we
should have this set errno or something so that callers can distinguish
between "you sent garbled input" vs. "you need to set up
blocksize /sectsize"... ?

--D

> -			usage();
> -		} else {
> -			return i * sectsize;
> -		}
> -	}
> -
> -	c = tolower(*sp);
> -	switch (c) {
> -	case 'e':
> -		i *= 1024LL;
> -		/* fall through */
> -	case 'p':
> -		i *= 1024LL;
> -		/* fall through */
> -	case 't':
> -		i *= 1024LL;
> -		/* fall through */
> -	case 'g':
> -		i *= 1024LL;
> -		/* fall through */
> -	case 'm':
> -		i *= 1024LL;
> -		/* fall through */
> -	case 'k':
> -		return i * 1024LL;
> -	default:
> -		break;
> -	}
> -	return -1LL;
> -}
> -
>  static void
>  check_device_type(
>  	const char	*name,
> @@ -1347,9 +1284,13 @@ getnum(
>  	 * convert it ourselves to guarantee there is no trailing garbage in the
>  	 * number.
>  	 */
> -	if (sp->convert)
> +	if (sp->convert) {
>  		c = cvtnum(blocksize, sectorsize, str);
> -	else {
> +		if (c == -1LL) {
> +			illegal_option(str, opts, index,
> +				_("Not a valid value or illegal suffix"));
> +		}
> +	} else {
>  		char		*str_end;
>  
>  		c = strtoll(str, &str_end, 0);
> -- 
> 2.23.0.rc1
> 

Re: [PATCH 1/2] mkfs: use cvtnum from libfrog

[Dave Chinner]

On Tue, Aug 13, 2019 at 07:24:14AM -0700, Darrick J. Wong wrote:
> On Tue, Aug 13, 2019 at 03:14:19PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > -	}
> > -	if (*sp == 's') {
> > -		if (!sectsize) {
> > -			fprintf(stderr,
> > -_("Sectorsize must be specified prior to using 's' suffix.\n"));
> 
> Hmm, so this message is replaced with "Not a valid value or illegal suffix"?

Actually, the error message is this:

# mkfs.xfs -f -b size=1b /dev/vdc
Invalid value 1b for -b size option. Not a valid value or illegal suffix

It does actually tell you what the value is, what option is wrong,
and the message shold be fairly clear that specifying the block size
in using a "blocks" suffix is illegal.

> That's not anywhere near as helpful as the old message... maybe we
> should have this set errno or something so that callers can distinguish
> between "you sent garbled input" vs. "you need to set up
> blocksize /sectsize"... ?

Actually, the error will only occur when you use -s size= or -b
size= options, as if they are not specified we use the default
values in mkfs and cvtnum is always called with a valid
blocksize/sectorsize pair. i.e. This error only triggers when validating
the base sector size/block size options because that occurs before
we set the global varibles mkfs will use for cvtnum....

It's a chicken-egg thing, and I figured the error message prefix
would be sufficient to point out the problem with the value suffic
used for these kinda unusual corner cases.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2] mkfs: use cvtnum from libfrog

[Darrick J. Wong]

On Wed, Aug 14, 2019 at 07:29:36AM +1000, Dave Chinner wrote:
> On Tue, Aug 13, 2019 at 07:24:14AM -0700, Darrick J. Wong wrote:
> > On Tue, Aug 13, 2019 at 03:14:19PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > -	}
> > > -	if (*sp == 's') {
> > > -		if (!sectsize) {
> > > -			fprintf(stderr,
> > > -_("Sectorsize must be specified prior to using 's' suffix.\n"));
> > 
> > Hmm, so this message is replaced with "Not a valid value or illegal suffix"?
> 
> Actually, the error message is this:
> 
> # mkfs.xfs -f -b size=1b /dev/vdc
> Invalid value 1b for -b size option. Not a valid value or illegal suffix
> 
> It does actually tell you what the value is, what option is wrong,
> and the message shold be fairly clear that specifying the block size
> in using a "blocks" suffix is illegal.
> 
> > That's not anywhere near as helpful as the old message... maybe we
> > should have this set errno or something so that callers can distinguish
> > between "you sent garbled input" vs. "you need to set up
> > blocksize /sectsize"... ?
> 
> Actually, the error will only occur when you use -s size= or -b
> size= options, as if they are not specified we use the default
> values in mkfs and cvtnum is always called with a valid
> blocksize/sectorsize pair. i.e. This error only triggers when validating
> the base sector size/block size options because that occurs before
> we set the global varibles mkfs will use for cvtnum....
> 
> It's a chicken-egg thing, and I figured the error message prefix
> would be sufficient to point out the problem with the value suffic
> used for these kinda unusual corner cases.

Heh, ok, carry on then. :)

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D


> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

[PATCH 2/2] xfsprogs: Fix --disable-static option build

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Internal xfsprogs libraries are linked statically to binaries as
they are not shipped libraries. Using --disable-static prevents the
internal static libraries from being built and this breaks dead code
elimination and results in linker failures from link dependencies
introduced by dead code.

We can't remove the --disable-static option that causes this as it
is part of the libtool/autoconf generated infrastructure. We can,
however, reliably detect whether static library building has been
disabled after the libtool infrastructure has been configured.
Therefore, add a check to determine the static build status and
abort the configure script with an error if we have been configured
not to build static libraries.

This build command now succeeds:

$ make realclean; make configure; ./configure --disable-static ; make

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 libfrog/Makefile | 2 ++
 libxcmd/Makefile | 2 ++
 libxfs/Makefile  | 2 ++
 libxlog/Makefile | 2 ++
 4 files changed, 8 insertions(+)

diff --git a/libfrog/Makefile b/libfrog/Makefile
index f5a0539b3f03..4d79983eb910 100644
--- a/libfrog/Makefile
+++ b/libfrog/Makefile
@@ -9,6 +9,8 @@ LTLIBRARY = libfrog.la
 LT_CURRENT = 0
 LT_REVISION = 0
 LT_AGE = 0
+# we need a static build even if --disable-static is specified
+LTLDFLAGS += -static
 
 CFILES = \
 avl64.c \
diff --git a/libxcmd/Makefile b/libxcmd/Makefile
index 914bec024c46..f9bc1c5c483a 100644
--- a/libxcmd/Makefile
+++ b/libxcmd/Makefile
@@ -9,6 +9,8 @@ LTLIBRARY = libxcmd.la
 LT_CURRENT = 0
 LT_REVISION = 0
 LT_AGE = 0
+# we need a static build even if --disable-static is specified
+LTLDFLAGS += -static
 
 CFILES = command.c input.c help.c quit.c
 
diff --git a/libxfs/Makefile b/libxfs/Makefile
index 8c681e0b9083..d1688dc3853a 100644
--- a/libxfs/Makefile
+++ b/libxfs/Makefile
@@ -9,6 +9,8 @@ LTLIBRARY = libxfs.la
 LT_CURRENT = 0
 LT_REVISION = 0
 LT_AGE = 0
+# we need a static build even if --disable-static is specified
+LTLDFLAGS += -static
 
 # headers to install in include/xfs
 PKGHFILES = xfs_fs.h \
diff --git a/libxlog/Makefile b/libxlog/Makefile
index bdea6abacea4..b0f5ef154133 100644
--- a/libxlog/Makefile
+++ b/libxlog/Makefile
@@ -9,6 +9,8 @@ LTLIBRARY = libxlog.la
 LT_CURRENT = 0
 LT_REVISION = 0
 LT_AGE = 0
+# we need a static build even if --disable-static is specified
+LTLDFLAGS += -static
 
 CFILES = xfs_log_recover.c util.c
 
-- 
2.23.0.rc1

Re: [PATCH 2/2] xfsprogs: Fix --disable-static option build

[Darrick J. Wong]

On Tue, Aug 13, 2019 at 03:14:20PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Internal xfsprogs libraries are linked statically to binaries as
> they are not shipped libraries. Using --disable-static prevents the
> internal static libraries from being built and this breaks dead code
> elimination and results in linker failures from link dependencies
> introduced by dead code.
> 
> We can't remove the --disable-static option that causes this as it
> is part of the libtool/autoconf generated infrastructure. We can,
> however, reliably detect whether static library building has been
> disabled after the libtool infrastructure has been configured.
> Therefore, add a check to determine the static build status and
> abort the configure script with an error if we have been configured
> not to build static libraries.

Uh... is this missing from the patch?  I don't see anything that aborts
configure.  Though I sense this might be your v2 solution that works
around --disable-static via the ld command line and leaves configure
alone...? :)

--D

> This build command now succeeds:
> 
> $ make realclean; make configure; ./configure --disable-static ; make
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  libfrog/Makefile | 2 ++
>  libxcmd/Makefile | 2 ++
>  libxfs/Makefile  | 2 ++
>  libxlog/Makefile | 2 ++
>  4 files changed, 8 insertions(+)
> 
> diff --git a/libfrog/Makefile b/libfrog/Makefile
> index f5a0539b3f03..4d79983eb910 100644
> --- a/libfrog/Makefile
> +++ b/libfrog/Makefile
> @@ -9,6 +9,8 @@ LTLIBRARY = libfrog.la
>  LT_CURRENT = 0
>  LT_REVISION = 0
>  LT_AGE = 0
> +# we need a static build even if --disable-static is specified
> +LTLDFLAGS += -static
>  
>  CFILES = \
>  avl64.c \
> diff --git a/libxcmd/Makefile b/libxcmd/Makefile
> index 914bec024c46..f9bc1c5c483a 100644
> --- a/libxcmd/Makefile
> +++ b/libxcmd/Makefile
> @@ -9,6 +9,8 @@ LTLIBRARY = libxcmd.la
>  LT_CURRENT = 0
>  LT_REVISION = 0
>  LT_AGE = 0
> +# we need a static build even if --disable-static is specified
> +LTLDFLAGS += -static
>  
>  CFILES = command.c input.c help.c quit.c
>  
> diff --git a/libxfs/Makefile b/libxfs/Makefile
> index 8c681e0b9083..d1688dc3853a 100644
> --- a/libxfs/Makefile
> +++ b/libxfs/Makefile
> @@ -9,6 +9,8 @@ LTLIBRARY = libxfs.la
>  LT_CURRENT = 0
>  LT_REVISION = 0
>  LT_AGE = 0
> +# we need a static build even if --disable-static is specified
> +LTLDFLAGS += -static
>  
>  # headers to install in include/xfs
>  PKGHFILES = xfs_fs.h \
> diff --git a/libxlog/Makefile b/libxlog/Makefile
> index bdea6abacea4..b0f5ef154133 100644
> --- a/libxlog/Makefile
> +++ b/libxlog/Makefile
> @@ -9,6 +9,8 @@ LTLIBRARY = libxlog.la
>  LT_CURRENT = 0
>  LT_REVISION = 0
>  LT_AGE = 0
> +# we need a static build even if --disable-static is specified
> +LTLDFLAGS += -static
>  
>  CFILES = xfs_log_recover.c util.c
>  
> -- 
> 2.23.0.rc1
> 

Re: [PATCH 2/2] xfsprogs: Fix --disable-static option build

[Dave Chinner]

On Tue, Aug 13, 2019 at 07:28:01AM -0700, Darrick J. Wong wrote:
> On Tue, Aug 13, 2019 at 03:14:20PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Internal xfsprogs libraries are linked statically to binaries as
> > they are not shipped libraries. Using --disable-static prevents the
> > internal static libraries from being built and this breaks dead code
> > elimination and results in linker failures from link dependencies
> > introduced by dead code.
> > 
> > We can't remove the --disable-static option that causes this as it
> > is part of the libtool/autoconf generated infrastructure. We can,
> > however, reliably detect whether static library building has been
> > disabled after the libtool infrastructure has been configured.
> > Therefore, add a check to determine the static build status and
> > abort the configure script with an error if we have been configured
> > not to build static libraries.
> 
> Uh... is this missing from the patch?  I don't see anything that aborts
> configure.  Though I sense this might be your v2 solution that works
> around --disable-static via the ld command line and leaves configure
> alone...? :)

Ugh, forgot to refresh the patch after updating the comment. That
paragraph should read:

	We can't remove the --disable-static option that causes this as it
	is part of the libtool/autoconf generated infrastructure. We can,
	however, override --disable-static on a per-library basis inside the
	build by passing -static to the libtool link command. Therefore, add
	-static to all the internal libraries we build and link statically
	to the shipping binaries.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com