On Tue, Jul 25, 2017 at 11:32:23AM -0500, Eric Sandeen wrote:
> On 07/25/2017 11:27 AM, Darrick J. Wong wrote:
> > In patch 4944defad4 ("xfs_db: redirect printfs when metadumping to
> > stdout"), we solved the problem of xfs_db printfs ending up in the
> > metadump stream by reassigning stdout for the duration of a stdout
> > metadump. Unfortunately, musl doesn't allow stdout to be reassigned (in
> > their view "extern FILE *stdout" means "extern FILE * const stdout"), so
> > we abandon the old approach in favor of playing games with dup() to
> > switch the raw file descriptors.
> >
> > While we're at it, fix a regression where an unconverted outf test
> > allows progress info to end up in the metadump stream.
>
> "while we're at it" usually indicates the need for a separate patch ;)
Ok, though the "while we're at it" fix is only necessary if we keep the
stdout redirection stuff.
> So what if we just converted dbprintf to do fprintf, with the destination
> held in some global variable that metadump can change? Seems like that
> might be less tricksy, and xfs_db is no stranger to globals.
>
> there are about 5 bare printfs in xfs_db, but none should be encountered
> during a metadump (well ... maybe the one in set_cur).
Less tricksy, but more of a maintenance burden for us, because now we
can't ever allow printf() or fprintf(stdout) in any code path that gets
called from metadump. The reviewers will have to remember (and document
for future reviewers).
OTOH, if we let printfs slip in to something that isn't called by
metadump and then a later patch moves it into the metadump call graph,
now we have a logic bomb that will manifest itself in silently corrupted
metadumps again.
So that's why I went with playing games making stdout (or now just fd 1)
dump to stderr -- the buffoonery required to make metadumping to stdout
work properly is contained in db/metadump.c.
--D
>
> -eric
>
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > db/metadump.c | 47 ++++++++++++++++++++++++++++++++++++-----------
> > 1 file changed, 36 insertions(+), 11 deletions(-)
> >
> > diff --git a/db/metadump.c b/db/metadump.c
> > index b58acef..8cdcb37 100644
> > --- a/db/metadump.c
> > +++ b/db/metadump.c
> > @@ -78,6 +78,7 @@ static int obfuscate = 1;
> > static int zero_stale_data = 1;
> > static int show_warnings = 0;
> > static int progress_since_warning = 0;
> > +static bool stdout_metadump;
> >
> > void
> > metadump_init(void)
> > @@ -137,7 +138,7 @@ print_progress(const char *fmt, ...)
> > va_end(ap);
> > buf[sizeof(buf)-1] = '\0';
> >
> > - f = (outf == stdout) ? stderr : stdout;
> > + f = stdout_metadump ? stderr : stdout;
> > fprintf(f, "\r%-59s", buf);
> > fflush(f);
> > progress_since_warning = 1;
> > @@ -2874,7 +2875,8 @@ metadump_f(
> > xfs_agnumber_t agno;
> > int c;
> > int start_iocur_sp;
> > - bool stdout_metadump = false;
> > + int outfd = -1;
> > + int ret;
> > char *p;
> >
> > exitcode = 1;
> > @@ -2994,16 +2996,35 @@ metadump_f(
> > * metadump operation so that dbprintf and other messages
> > * are sent to the console instead of polluting the
> > * metadump stream.
> > + *
> > + * We get to do this the hard way because musl doesn't
> > + * allow reassignment of stdout.
> > */
> > - outf = stdout;
> > - stdout = stderr;
> > + fflush(stdout);
> > + outfd = dup(STDOUT_FILENO);
> > + if (outfd < 0) {
> > + perror("opening dump stream");
> > + goto out;
> > + }
> > + ret = dup2(STDERR_FILENO, STDOUT_FILENO);
> > + if (ret < 0) {
> > + perror("redirecting stdout");
> > + close(outfd);
> > + goto out;
> > + }
> > + outf = fdopen(outfd, "a");
> > + if (outf == NULL) {
> > + fprintf(stderr, "cannot create dump stream\n");
> > + dup2(outfd, 1);
> > + close(outfd);
> > + goto out;
> > + }
> > stdout_metadump = true;
> > } else {
> > outf = fopen(argv[optind], "wb");
> > if (outf == NULL) {
> > print_warning("cannot create dump file");
> > - free(metablock);
> > - return 0;
> > + goto out;
> > }
> > }
> >
> > @@ -3031,15 +3052,19 @@ metadump_f(
> > if (progress_since_warning)
> > fputc('\n', stdout_metadump ? stderr : stdout);
> >
> > - if (stdout_metadump)
> > - stdout = outf;
> > - else
> > - fclose(outf);
> > + if (stdout_metadump) {
> > + fflush(outf);
> > + fflush(stdout);
> > + ret = dup2(outfd, STDOUT_FILENO);
> > + if (ret < 0)
> > + perror("un-redirecting stdout");
> > + }
> > + fclose(outf);
> >
> > /* cleanup iocur stack */
> > while (iocur_sp > start_iocur_sp)
> > pop_cur();
> > -
> > +out:
> > free(metablock);
> >
> > return 0;
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
