xfsprogs: "Fix" --disable-static option
cover xfsprogs: "Fix" --disable-static option
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
From: Dave Chinner <[email protected]>
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.
--- libfrog/convert.c | 12 +++++--- mkfs/xfs_mkfs.c | 71 ++++------------------------------------------- 2 files changed, 14 insertions(+), 69 deletions(-)
libfrog/convert.c +8 -4
@@ -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' :
mkfs/xfs_mkfs.c +6 -65
84 lines not on this page · raw message
Signature · 1 line
2.23.0.rc1
Re: [PATCH 1/2] mkfs: use cvtnum from libfrog
Quoted 76 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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 <[email protected]>--- libfrog/convert.c | 12 +++++--- mkfs/xfs_mkfs.c | 71 ++++------------------------------------------- 2 files changed, 14 insertions(+), 69 deletions(-)libfrog/convert.c +8
-4@@ -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' :mkfs/xfs_mkfs.c +0
-22@@ -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,
mkfs/xfs_mkfs.c unknown( · line 976
Quoted 10 lines
mkfs/xfs_mkfs.c +0
-10-_("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
Quoted 53 lines
mkfs/xfs_mkfs.c +6
-33-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 );Signature · 1 line
2.23.0.rc1
Re: [PATCH 1/2] mkfs: use cvtnum from libfrog
Quoted 9 lines · Darrick J. Wong
mkfs/xfs_mkfs.c unknown( · line 976
Quoted 6 lines · Dave Chinner
From: Dave Chinner <[email protected]>
mkfs/xfs_mkfs.c +0
-5- } -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.
Quoted 4 lines
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.
Signature · 2 lines
Dave Chinner [email protected]
Darrick J. Wong
Reviewed-byRe: [PATCH 1/2] mkfs: use cvtnum from libfrog
Quoted 32 lines · Dave Chinner
Quoted 9 lines · Darrick J. Wong
mkfs/xfs_mkfs.c unknown( · line 976
Quoted 6 lines · Dave Chinner
From: Dave Chinner <[email protected]>
mkfs/xfs_mkfs.c +0
-5- } -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 suffixIt 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.
Quoted 4 lines
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....
Quoted 3 lines
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. :)
--D
Quoted 6 lines
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
PATCH 2/2 xfsprogs: Fix --disable-static option build
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
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
--- libfrog/Makefile | 2 ++ libxcmd/Makefile | 2 ++ libxfs/Makefile | 2 ++ libxlog/Makefile | 2 ++ 4 files changed, 8 insertions(+)
libfrog/Makefile +2 -0
@@ -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 \
libxcmd/Makefile +2 -0
@@ -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
libxfs/Makefile +2 -0
@@ -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 \
libxlog/Makefile +2 -0
@@ -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
Signature · 1 line
2.23.0.rc1
Re: [PATCH 2/2] xfsprogs: Fix --disable-static option build
Quoted 8 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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.
Quoted 7 lines
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
Quoted 67 lines
This build command now succeeds:
$ make realclean; make configure; ./configure --disable-static ; makeSigned-off-by Dave Chinner <[email protected]>--- libfrog/Makefile | 2 ++ libxcmd/Makefile | 2 ++ libxfs/Makefile | 2 ++ libxlog/Makefile | 2 ++ 4 files changed, 8 insertions(+)libfrog/Makefile +2
-0@@ -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 \libxcmd/Makefile +2
-0@@ -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.clibxfs/Makefile +2
-0@@ -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 \libxlog/Makefile +2
-0@@ -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.cSignature · 1 line
2.23.0.rc1
Re: [PATCH 2/2] xfsprogs: Fix --disable-static option build
Quoted 17 lines · Darrick J. Wong
Quoted 8 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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.
Quoted 7 lines
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.
Quoted 4 lines
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.
Signature · 2 lines
Dave Chinner [email protected]