* Re: [PATCH] xfsprogs: do not redeclare globals provided by libraries
2020-01-27 22:56 [PATCH] xfsprogs: do not redeclare globals provided by libraries Eric Sandeen
@ 2020-01-28 3:29 ` Darrick J. Wong
2020-01-28 14:48 ` Eric Sandeen
2020-01-28 19:42 ` Eric Sandeen
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Darrick J. Wong @ 2020-01-28 3:29 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
On Mon, Jan 27, 2020 at 04:56:02PM -0600, Eric Sandeen wrote:
> From: Eric Sandeen <sandeen@redhat.com>
>
> In each of these cases, db, logprint, and mdrestore are redeclaring
> as a global variable something which was already provided by a
> library they link with.
Er... which library?
Also, uh...maybe we shouldn't be exporting globals across libraries?
(He says having not looked for how many there are lurki... ye gods)
--D
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> diff --git a/db/init.c b/db/init.c
> index 455220a..0ac3736 100644
> --- a/db/init.c
> +++ b/db/init.c
> @@ -27,7 +27,6 @@ static int force;
> static struct xfs_mount xmount;
> struct xfs_mount *mp;
> static struct xlog xlog;
> -libxfs_init_t x;
> xfs_agnumber_t cur_agno = NULLAGNUMBER;
>
> static void
> diff --git a/logprint/logprint.c b/logprint/logprint.c
> index 7754a2a..511a32a 100644
> --- a/logprint/logprint.c
> +++ b/logprint/logprint.c
> @@ -24,7 +24,6 @@ int print_buffer;
> int print_overwrite;
> int print_no_data;
> int print_no_print;
> -int print_exit = 1; /* -e is now default. specify -c to override */
> static int print_operation = OP_PRINT;
>
> static void
> @@ -132,6 +131,7 @@ main(int argc, char **argv)
> bindtextdomain(PACKAGE, LOCALEDIR);
> textdomain(PACKAGE);
> memset(&mount, 0, sizeof(mount));
> + print_exit = 1; /* -e is now default. specify -c to override */
>
> progname = basename(argv[0]);
> while ((c = getopt(argc, argv, "bC:cdefl:iqnors:tDVv")) != EOF) {
> @@ -152,7 +152,7 @@ main(int argc, char **argv)
> case 'e':
> /* -e is now default
> */
> - print_exit++;
> + print_exit = 1;
> break;
> case 'C':
> print_operation = OP_COPY;
> diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
> index 3375e08..1cd399d 100644
> --- a/mdrestore/xfs_mdrestore.c
> +++ b/mdrestore/xfs_mdrestore.c
> @@ -7,7 +7,6 @@
> #include "libxfs.h"
> #include "xfs_metadump.h"
>
> -char *progname;
> static int show_progress = 0;
> static int show_info = 0;
> static int progress_since_warning = 0;
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] xfsprogs: do not redeclare globals provided by libraries
2020-01-28 3:29 ` Darrick J. Wong
@ 2020-01-28 14:48 ` Eric Sandeen
2020-01-28 22:26 ` Dave Chinner
0 siblings, 1 reply; 15+ messages in thread
From: Eric Sandeen @ 2020-01-28 14:48 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On 1/27/20 9:29 PM, Darrick J. Wong wrote:
> On Mon, Jan 27, 2020 at 04:56:02PM -0600, Eric Sandeen wrote:
>> From: Eric Sandeen <sandeen@redhat.com>
>>
>> In each of these cases, db, logprint, and mdrestore are redeclaring
>> as a global variable something which was already provided by a
>> library they link with.
>
> Er... which library?
libxfs and libxlog ...
File Line
0 libxlog/util.c 10 int print_exit;
1 logprint/logprint.c 27 int print_exit = 1;
File Line
0 db/init.c 30 libxfs_init_t x;
1 libxlog/util.c 13 libxfs_init_t x;
File Line
0 fsr/xfs_fsr.c 31 char *progname;
1 io/init.c 14 char *progname;
2 libxfs/init.c 28 char *progname = "libxfs";
3 mdrestore/xfs_mdrestore.c 10 char *progname;
(fsr & io don't link w/ libxfs; mdrestore does)
>
> Also, uh...maybe we shouldn't be exporting globals across libraries?
>
> (He says having not looked for how many there are lurki... ye gods)
Well, it's ugly for sure.
We could either try to re-architect this to
1) pass stuff like progname all over the place, or
2) consistently make the library provide it as a global, or
3) consistently make utils provide it to the library as a global (?)
choose your poison?
>
> --D
>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/db/init.c b/db/init.c
>> index 455220a..0ac3736 100644
>> --- a/db/init.c
>> +++ b/db/init.c
>> @@ -27,7 +27,6 @@ static int force;
>> static struct xfs_mount xmount;
>> struct xfs_mount *mp;
>> static struct xlog xlog;
>> -libxfs_init_t x;
>> xfs_agnumber_t cur_agno = NULLAGNUMBER;
>>
>> static void
>> diff --git a/logprint/logprint.c b/logprint/logprint.c
>> index 7754a2a..511a32a 100644
>> --- a/logprint/logprint.c
>> +++ b/logprint/logprint.c
>> @@ -24,7 +24,6 @@ int print_buffer;
>> int print_overwrite;
>> int print_no_data;
>> int print_no_print;
>> -int print_exit = 1; /* -e is now default. specify -c to override */
>> static int print_operation = OP_PRINT;
>>
>> static void
>> @@ -132,6 +131,7 @@ main(int argc, char **argv)
>> bindtextdomain(PACKAGE, LOCALEDIR);
>> textdomain(PACKAGE);
>> memset(&mount, 0, sizeof(mount));
>> + print_exit = 1; /* -e is now default. specify -c to override */
>>
>> progname = basename(argv[0]);
>> while ((c = getopt(argc, argv, "bC:cdefl:iqnors:tDVv")) != EOF) {
>> @@ -152,7 +152,7 @@ main(int argc, char **argv)
>> case 'e':
>> /* -e is now default
>> */
>> - print_exit++;
>> + print_exit = 1;
>> break;
>> case 'C':
>> print_operation = OP_COPY;
>> diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
>> index 3375e08..1cd399d 100644
>> --- a/mdrestore/xfs_mdrestore.c
>> +++ b/mdrestore/xfs_mdrestore.c
>> @@ -7,7 +7,6 @@
>> #include "libxfs.h"
>> #include "xfs_metadump.h"
>>
>> -char *progname;
>> static int show_progress = 0;
>> static int show_info = 0;
>> static int progress_since_warning = 0;
>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] xfsprogs: do not redeclare globals provided by libraries
2020-01-28 14:48 ` Eric Sandeen
@ 2020-01-28 22:26 ` Dave Chinner
2020-01-28 22:28 ` Eric Sandeen
0 siblings, 1 reply; 15+ messages in thread
From: Dave Chinner @ 2020-01-28 22:26 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Darrick J. Wong, linux-xfs
On Tue, Jan 28, 2020 at 08:48:40AM -0600, Eric Sandeen wrote:
> On 1/27/20 9:29 PM, Darrick J. Wong wrote:
> > On Mon, Jan 27, 2020 at 04:56:02PM -0600, Eric Sandeen wrote:
> > > From: Eric Sandeen <sandeen@redhat.com>
> > >
> > > In each of these cases, db, logprint, and mdrestore are redeclaring
> > > as a global variable something which was already provided by a
> > > library they link with.
> >
> > Er... which library?
>
> libxfs and libxlog ...
>
>
> File Line
> 0 libxlog/util.c 10 int print_exit;
> 1 logprint/logprint.c 27 int print_exit = 1;
>
> File Line
> 0 db/init.c 30 libxfs_init_t x;
> 1 libxlog/util.c 13 libxfs_init_t x;
>
> File Line
> 0 fsr/xfs_fsr.c 31 char *progname;
> 1 io/init.c 14 char *progname;
> 2 libxfs/init.c 28 char *progname = "libxfs";
> 3 mdrestore/xfs_mdrestore.c 10 char *progname;
>
> (fsr & io don't link w/ libxfs; mdrestore does)
>
>
> >
> > Also, uh...maybe we shouldn't be exporting globals across libraries?
> >
> > (He says having not looked for how many there are lurki... ye gods)
>
> Well, it's ugly for sure.
>
> We could either try to re-architect this to
>
> 1) pass stuff like progname all over the place, or
> 2) consistently make the library provide it as a global, or
> 3) consistently make utils provide it to the library as a global (?)
IIRC, I chose #2 way back when I was consolidating all the libxfs
library code. There was code that declared libxfs_init_t x; on
stack, as local globals, in the libraries, etc. So I simply made the
library global the One True Global, and then had everyone use it
everywhere.
That was just the simplest solution at the time to minimise the
amount of work to get userspace up to date with the kernel to allow
integration of the CRC work (userspace was years out of date at that
point). It was not pretty (like a lot of my code), but it worked.
Feel free to do what you think is best :)
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] xfsprogs: do not redeclare globals provided by libraries
2020-01-28 22:26 ` Dave Chinner
@ 2020-01-28 22:28 ` Eric Sandeen
0 siblings, 0 replies; 15+ messages in thread
From: Eric Sandeen @ 2020-01-28 22:28 UTC (permalink / raw)
To: Dave Chinner; +Cc: Darrick J. Wong, linux-xfs
On 1/28/20 4:26 PM, Dave Chinner wrote:
> On Tue, Jan 28, 2020 at 08:48:40AM -0600, Eric Sandeen wrote:
>> On 1/27/20 9:29 PM, Darrick J. Wong wrote:
>>> On Mon, Jan 27, 2020 at 04:56:02PM -0600, Eric Sandeen wrote:
>>>> From: Eric Sandeen <sandeen@redhat.com>
>>>>
>>>> In each of these cases, db, logprint, and mdrestore are redeclaring
>>>> as a global variable something which was already provided by a
>>>> library they link with.
>>>
>>> Er... which library?
>>
>> libxfs and libxlog ...
>>
>>
>> File Line
>> 0 libxlog/util.c 10 int print_exit;
>> 1 logprint/logprint.c 27 int print_exit = 1;
>>
>> File Line
>> 0 db/init.c 30 libxfs_init_t x;
>> 1 libxlog/util.c 13 libxfs_init_t x;
>>
>> File Line
>> 0 fsr/xfs_fsr.c 31 char *progname;
>> 1 io/init.c 14 char *progname;
>> 2 libxfs/init.c 28 char *progname = "libxfs";
>> 3 mdrestore/xfs_mdrestore.c 10 char *progname;
>>
>> (fsr & io don't link w/ libxfs; mdrestore does)
>>
>>
>>>
>>> Also, uh...maybe we shouldn't be exporting globals across libraries?
>>>
>>> (He says having not looked for how many there are lurki... ye gods)
>>
>> Well, it's ugly for sure.
>>
>> We could either try to re-architect this to
>>
>> 1) pass stuff like progname all over the place, or
>> 2) consistently make the library provide it as a global, or
>> 3) consistently make utils provide it to the library as a global (?)
>
> IIRC, I chose #2 way back when I was consolidating all the libxfs
> library code. There was code that declared libxfs_init_t x; on
> stack, as local globals, in the libraries, etc. So I simply made the
> library global the One True Global, and then had everyone use it
> everywhere.
>
> That was just the simplest solution at the time to minimise the
> amount of work to get userspace up to date with the kernel to allow
> integration of the CRC work (userspace was years out of date at that
> point). It was not pretty (like a lot of my code), but it worked.
>
> Feel free to do what you think is best :)
I think the patch I sent is best - not quite sure how the stray globals
snuck in as problems (or maybe got missed the first time) but here we are.
gcc 10 complains about them btw, I should have mentioned that.
All I need now is a review. :D
-Eric
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] xfsprogs: do not redeclare globals provided by libraries
2020-01-27 22:56 [PATCH] xfsprogs: do not redeclare globals provided by libraries Eric Sandeen
2020-01-28 3:29 ` Darrick J. Wong
@ 2020-01-28 19:42 ` Eric Sandeen
2020-01-29 15:16 ` [PATCH V2] " Eric Sandeen
2020-01-29 18:05 ` [PATCH] " Christoph Hellwig
3 siblings, 0 replies; 15+ messages in thread
From: Eric Sandeen @ 2020-01-28 19:42 UTC (permalink / raw)
To: linux-xfs
On 1/27/20 4:56 PM, Eric Sandeen wrote:
> From: Eric Sandeen <sandeen@redhat.com>
>
> In each of these cases, db, logprint, and mdrestore are redeclaring
> as a global variable something which was already provided by a
> library they link with.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
(sigh, it appears I have whitespace mangling gremlins back again,
sorry - was temporarily on a different system w/ my normal email tools
absent.)
-Eric
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V2] xfsprogs: do not redeclare globals provided by libraries
2020-01-27 22:56 [PATCH] xfsprogs: do not redeclare globals provided by libraries Eric Sandeen
2020-01-28 3:29 ` Darrick J. Wong
2020-01-28 19:42 ` Eric Sandeen
@ 2020-01-29 15:16 ` Eric Sandeen
2020-01-29 16:01 ` Darrick J. Wong
` (2 more replies)
2020-01-29 18:05 ` [PATCH] " Christoph Hellwig
3 siblings, 3 replies; 15+ messages in thread
From: Eric Sandeen @ 2020-01-29 15:16 UTC (permalink / raw)
To: linux-xfs
In each of these cases, db, logprint, and mdrestore are redeclaring
as a global variable something which was already provided by a
library they link with.
gcc now defaults to -fno-common and trips over these global variables
which are declared in utilities as well as in libxfs and libxlog, and
it causes the build to fail.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
V2: unmangle whitespace, I'm a n00b.
diff --git a/db/init.c b/db/init.c
index 455220a..0ac3736 100644
--- a/db/init.c
+++ b/db/init.c
@@ -27,7 +27,6 @@ static int force;
static struct xfs_mount xmount;
struct xfs_mount *mp;
static struct xlog xlog;
-libxfs_init_t x;
xfs_agnumber_t cur_agno = NULLAGNUMBER;
static void
diff --git a/logprint/logprint.c b/logprint/logprint.c
index 7754a2a..5809af9 100644
--- a/logprint/logprint.c
+++ b/logprint/logprint.c
@@ -24,7 +24,6 @@ int print_buffer;
int print_overwrite;
int print_no_data;
int print_no_print;
-int print_exit = 1; /* -e is now default. specify -c to override */
static int print_operation = OP_PRINT;
static void
@@ -132,6 +131,7 @@ main(int argc, char **argv)
bindtextdomain(PACKAGE, LOCALEDIR);
textdomain(PACKAGE);
memset(&mount, 0, sizeof(mount));
+ print_exit = 1; /* -e is now default. specify -c to override */
progname = basename(argv[0]);
while ((c = getopt(argc, argv, "bC:cdefl:iqnors:tDVv")) != EOF) {
@@ -152,7 +152,7 @@ main(int argc, char **argv)
case 'e':
/* -e is now default
*/
- print_exit++;
+ print_exit = 1;
break;
case 'C':
print_operation = OP_COPY;
diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
index 3375e08..1cd399d 100644
--- a/mdrestore/xfs_mdrestore.c
+++ b/mdrestore/xfs_mdrestore.c
@@ -7,7 +7,6 @@
#include "libxfs.h"
#include "xfs_metadump.h"
-char *progname;
static int show_progress = 0;
static int show_info = 0;
static int progress_since_warning = 0;
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH V2] xfsprogs: do not redeclare globals provided by libraries
2020-01-29 15:16 ` [PATCH V2] " Eric Sandeen
@ 2020-01-29 16:01 ` Darrick J. Wong
2020-01-29 16:57 ` Eric Sandeen
2020-01-29 18:06 ` Christoph Hellwig
2020-01-29 19:47 ` [PATCH V3] " Eric Sandeen
2 siblings, 1 reply; 15+ messages in thread
From: Darrick J. Wong @ 2020-01-29 16:01 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
On Wed, Jan 29, 2020 at 09:16:58AM -0600, Eric Sandeen wrote:
> In each of these cases, db, logprint, and mdrestore are redeclaring
> as a global variable something which was already provided by a
> library they link with.
>
> gcc now defaults to -fno-common and trips over these global variables
> which are declared in utilities as well as in libxfs and libxlog, and
> it causes the build to fail.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> V2: unmangle whitespace, I'm a n00b.
BOFH FTW
> diff --git a/db/init.c b/db/init.c
> index 455220a..0ac3736 100644
> --- a/db/init.c
> +++ b/db/init.c
> @@ -27,7 +27,6 @@ static int force;
> static struct xfs_mount xmount;
> struct xfs_mount *mp;
> static struct xlog xlog;
> -libxfs_init_t x;
> xfs_agnumber_t cur_agno = NULLAGNUMBER;
>
> static void
> diff --git a/logprint/logprint.c b/logprint/logprint.c
> index 7754a2a..5809af9 100644
> --- a/logprint/logprint.c
> +++ b/logprint/logprint.c
> @@ -24,7 +24,6 @@ int print_buffer;
> int print_overwrite;
> int print_no_data;
> int print_no_print;
> -int print_exit = 1; /* -e is now default. specify -c to override */
> static int print_operation = OP_PRINT;
>
> static void
> @@ -132,6 +131,7 @@ main(int argc, char **argv)
> bindtextdomain(PACKAGE, LOCALEDIR);
> textdomain(PACKAGE);
> memset(&mount, 0, sizeof(mount));
> + print_exit = 1; /* -e is now default. specify -c to override */
With the trailing whitespace after the comment fixed,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Though given your earlier comment on IRC, maybe we should investigate
why -fno-common would be useful (since Fedora turned it on??) or if it
should be in the regular build to catch multiply defined global vars?
--D
>
> progname = basename(argv[0]);
> while ((c = getopt(argc, argv, "bC:cdefl:iqnors:tDVv")) != EOF) {
> @@ -152,7 +152,7 @@ main(int argc, char **argv)
> case 'e':
> /* -e is now default
> */
> - print_exit++;
> + print_exit = 1;
> break;
> case 'C':
> print_operation = OP_COPY;
> diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
> index 3375e08..1cd399d 100644
> --- a/mdrestore/xfs_mdrestore.c
> +++ b/mdrestore/xfs_mdrestore.c
> @@ -7,7 +7,6 @@
> #include "libxfs.h"
> #include "xfs_metadump.h"
>
> -char *progname;
> static int show_progress = 0;
> static int show_info = 0;
> static int progress_since_warning = 0;
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH V2] xfsprogs: do not redeclare globals provided by libraries
2020-01-29 16:01 ` Darrick J. Wong
@ 2020-01-29 16:57 ` Eric Sandeen
2020-01-29 19:29 ` Darrick J. Wong
0 siblings, 1 reply; 15+ messages in thread
From: Eric Sandeen @ 2020-01-29 16:57 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On 1/29/20 10:01 AM, Darrick J. Wong wrote:
> On Wed, Jan 29, 2020 at 09:16:58AM -0600, Eric Sandeen wrote:
>> In each of these cases, db, logprint, and mdrestore are redeclaring
>> as a global variable something which was already provided by a
>> library they link with.
>>
>> gcc now defaults to -fno-common and trips over these global variables
>> which are declared in utilities as well as in libxfs and libxlog, and
>> it causes the build to fail.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
...
>> + print_exit = 1; /* -e is now default. specify -c to override */
>
> With the trailing whitespace after the comment fixed,
oh burn. ;)
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>
> Though given your earlier comment on IRC, maybe we should investigate
> why -fno-common would be useful (since Fedora turned it on??) or if it
> should be in the regular build to catch multiply defined global vars?
That's a good idea. FWIW my understanding is that it's a new default in
gcc10.
Docs say w.r.t. -fcommon: "This behavior is inconsistent with C++, and on many targets implies a speed and code size penalty on global variable references. It is mainly useful to enable legacy code to link without errors."
Want me to send a V3 w/ -fno-common explicitly set?
-Eric
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2] xfsprogs: do not redeclare globals provided by libraries
2020-01-29 16:57 ` Eric Sandeen
@ 2020-01-29 19:29 ` Darrick J. Wong
0 siblings, 0 replies; 15+ messages in thread
From: Darrick J. Wong @ 2020-01-29 19:29 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
On Wed, Jan 29, 2020 at 10:57:48AM -0600, Eric Sandeen wrote:
> On 1/29/20 10:01 AM, Darrick J. Wong wrote:
> > On Wed, Jan 29, 2020 at 09:16:58AM -0600, Eric Sandeen wrote:
> >> In each of these cases, db, logprint, and mdrestore are redeclaring
> >> as a global variable something which was already provided by a
> >> library they link with.
> >>
> >> gcc now defaults to -fno-common and trips over these global variables
> >> which are declared in utilities as well as in libxfs and libxlog, and
> >> it causes the build to fail.
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
>
> ...
>
> >> + print_exit = 1; /* -e is now default. specify -c to override */
> >
> > With the trailing whitespace after the comment fixed,
>
> oh burn. ;)
>
> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > Though given your earlier comment on IRC, maybe we should investigate
> > why -fno-common would be useful (since Fedora turned it on??) or if it
> > should be in the regular build to catch multiply defined global vars?
>
> That's a good idea. FWIW my understanding is that it's a new default in
> gcc10.
>
> Docs say w.r.t. -fcommon: "This behavior is inconsistent with C++, and on many targets implies a speed and code size penalty on global variable references. It is mainly useful to enable legacy code to link without errors."
>
> Want me to send a V3 w/ -fno-common explicitly set?
Yes, that sounds like a better place to start a conversation about ...
whatever that option is. :)
--D
> -Eric
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2] xfsprogs: do not redeclare globals provided by libraries
2020-01-29 15:16 ` [PATCH V2] " Eric Sandeen
2020-01-29 16:01 ` Darrick J. Wong
@ 2020-01-29 18:06 ` Christoph Hellwig
2020-01-29 19:47 ` [PATCH V3] " Eric Sandeen
2 siblings, 0 replies; 15+ messages in thread
From: Christoph Hellwig @ 2020-01-29 18:06 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
And with unmangled whitespaces it looks even better :)
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V3] xfsprogs: do not redeclare globals provided by libraries
2020-01-29 15:16 ` [PATCH V2] " Eric Sandeen
2020-01-29 16:01 ` Darrick J. Wong
2020-01-29 18:06 ` Christoph Hellwig
@ 2020-01-29 19:47 ` Eric Sandeen
2020-01-29 21:32 ` Christoph Hellwig
2 siblings, 1 reply; 15+ messages in thread
From: Eric Sandeen @ 2020-01-29 19:47 UTC (permalink / raw)
To: linux-xfs
In each of these cases, db, logprint, and mdrestore are redeclaring
as a global variable something which was already provided by a
library they link with.
gcc now defaults to -fno-common and trips over these global variables
which are declared in utilities as well as in libxfs and libxlog, and
it causes the build to fail.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
V2: unmangle whitespace, I'm a n00b.
V3: ditto, plus enforce -fno-common so older gcc users will play
by the same rules as gcc 10
diff --git a/db/init.c b/db/init.c
index 455220a8..0ac37368 100644
--- a/db/init.c
+++ b/db/init.c
@@ -27,7 +27,6 @@ static int force;
static struct xfs_mount xmount;
struct xfs_mount *mp;
static struct xlog xlog;
-libxfs_init_t x;
xfs_agnumber_t cur_agno = NULLAGNUMBER;
static void
diff --git a/include/builddefs.in b/include/builddefs.in
index 4700b527..0a5cce32 100644
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -13,7 +13,7 @@ OPTIMIZER = @opt_build@
MALLOCLIB = @malloc_lib@
LOADERFLAGS = @LDFLAGS@
LTLDFLAGS = @LDFLAGS@
-CFLAGS = @CFLAGS@ -D_FILE_OFFSET_BITS=64
+CFLAGS = @CFLAGS@ -D_FILE_OFFSET_BITS=64 -fno-common
BUILD_CFLAGS = @BUILD_CFLAGS@ -D_FILE_OFFSET_BITS=64
LIBRT = @librt@
diff --git a/logprint/logprint.c b/logprint/logprint.c
index 7754a2a6..11100569 100644
--- a/logprint/logprint.c
+++ b/logprint/logprint.c
@@ -24,7 +24,6 @@ int print_buffer;
int print_overwrite;
int print_no_data;
int print_no_print;
-int print_exit = 1; /* -e is now default. specify -c to override */
static int print_operation = OP_PRINT;
static void
@@ -132,6 +131,7 @@ main(int argc, char **argv)
bindtextdomain(PACKAGE, LOCALEDIR);
textdomain(PACKAGE);
memset(&mount, 0, sizeof(mount));
+ print_exit = 1; /* -e is now default. specify -c to override */
progname = basename(argv[0]);
while ((c = getopt(argc, argv, "bC:cdefl:iqnors:tDVv")) != EOF) {
@@ -152,7 +152,7 @@ main(int argc, char **argv)
case 'e':
/* -e is now default
*/
- print_exit++;
+ print_exit = 1;
break;
case 'C':
print_operation = OP_COPY;
diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
index 3375e080..1cd399db 100644
--- a/mdrestore/xfs_mdrestore.c
+++ b/mdrestore/xfs_mdrestore.c
@@ -7,7 +7,6 @@
#include "libxfs.h"
#include "xfs_metadump.h"
-char *progname;
static int show_progress = 0;
static int show_info = 0;
static int progress_since_warning = 0;
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH V3] xfsprogs: do not redeclare globals provided by libraries
2020-01-29 19:47 ` [PATCH V3] " Eric Sandeen
@ 2020-01-29 21:32 ` Christoph Hellwig
2020-01-29 22:20 ` Eric Sandeen
0 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2020-01-29 21:32 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
On Wed, Jan 29, 2020 at 01:47:49PM -0600, Eric Sandeen wrote:
> V3: ditto, plus enforce -fno-common so older gcc users will play
> by the same rules as gcc 10
I don't mind adding -fno-common, but it has aboslutely nothing to do
with removing pointless duplicate variable defintions.
I'd say apply v2 and make this a separate patch.
> @@ -13,7 +13,7 @@ OPTIMIZER = @opt_build@
> MALLOCLIB = @malloc_lib@
> LOADERFLAGS = @LDFLAGS@
> LTLDFLAGS = @LDFLAGS@
> -CFLAGS = @CFLAGS@ -D_FILE_OFFSET_BITS=64
> +CFLAGS = @CFLAGS@ -D_FILE_OFFSET_BITS=64 -fno-common
Are there any compilers not supporting -fno-common? (And do we care?)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V3] xfsprogs: do not redeclare globals provided by libraries
2020-01-29 21:32 ` Christoph Hellwig
@ 2020-01-29 22:20 ` Eric Sandeen
0 siblings, 0 replies; 15+ messages in thread
From: Eric Sandeen @ 2020-01-29 22:20 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On 1/29/20 3:32 PM, Christoph Hellwig wrote:
> On Wed, Jan 29, 2020 at 01:47:49PM -0600, Eric Sandeen wrote:
>> V3: ditto, plus enforce -fno-common so older gcc users will play
>> by the same rules as gcc 10
>
> I don't mind adding -fno-common, but it has aboslutely nothing to do
> with removing pointless duplicate variable defintions.
>
> I'd say apply v2 and make this a separate patch.
ok
>> @@ -13,7 +13,7 @@ OPTIMIZER = @opt_build@
>> MALLOCLIB = @malloc_lib@
>> LOADERFLAGS = @LDFLAGS@
>> LTLDFLAGS = @LDFLAGS@
>> -CFLAGS = @CFLAGS@ -D_FILE_OFFSET_BITS=64
>> +CFLAGS = @CFLAGS@ -D_FILE_OFFSET_BITS=64 -fno-common
>
> Are there any compilers not supporting -fno-common? (And do we care?)
not sure if it'd vary by version or by arch, but
gcc version 4.4.7
has it on RHEL7
-Eric
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] xfsprogs: do not redeclare globals provided by libraries
2020-01-27 22:56 [PATCH] xfsprogs: do not redeclare globals provided by libraries Eric Sandeen
` (2 preceding siblings ...)
2020-01-29 15:16 ` [PATCH V2] " Eric Sandeen
@ 2020-01-29 18:05 ` Christoph Hellwig
3 siblings, 0 replies; 15+ messages in thread
From: Christoph Hellwig @ 2020-01-29 18:05 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
On Mon, Jan 27, 2020 at 04:56:02PM -0600, Eric Sandeen wrote:
> From: Eric Sandeen <sandeen@redhat.com>
>
> In each of these cases, db, logprint, and mdrestore are redeclaring
> as a global variable something which was already provided by a
> library they link with.
Independent of any better way to handle the library interaction this
looks like an improvement on its own:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 15+ messages in thread