| Message ID | 0892b951-ac99-9f84-9c65-421798daa547@sandeen.net (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfsprogs: do not redeclare globals provided by libraries | expand |
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; >
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; >> >
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
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.
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
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>
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;