[PATCH] xfs_db: fix metadump redirection (again)

[PATCH] xfs_db: fix metadump redirection (again)

[Darrick J. Wong]

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.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 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;

Re: [PATCH] xfs_db: fix metadump redirection (again)

[Eric Sandeen]

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 ;)

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).

-eric

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] xfs_db: fix metadump redirection (again)

[Darrick J. Wong]

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@oracle.com>
> > ---
> >  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@vger.kernel.org
> > 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs_db: fix metadump redirection (again)

[Eric Sandeen]

On 07/25/2017 11:44 AM, Darrick J. Wong wrote:
> 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.

It's just that that is a fix for the last patch, and the rest is a new approach
which is still under debate. </pedantic>
 
>> 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).

Well, history shows that this isn't much of a problem.  There are over 500 calls
to dbprintf, and 6 to printf - including some which are probably appropriate,
i.e. to print version number and exit.

> 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.

yeah, I can see that POV too.  I'll see if anyone else really cares one way
or another.

-Eric

Re: [PATCH] xfs_db: fix metadump redirection (again)

[Jory A. Pratt]

[-- Attachment #1.1: Type: text/plain, Size: 3718 bytes --]

On 07/25/17 11:27, 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.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  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;
> 
I have tested and works. I appreciate you taking the time to readdress
the issue.

Thank You,

-- 
=======================================================
Jory A. Pratt
Gentoo Linux Developer [Mozilla Lead]
E-Mail    : anarchy@gentoo.org
GnuPG FP  : D4AC 8D63 0B16 F7C9 08E9  B909 A0CC C3BA B4D0 88B4
GnuPG ID  : B4D088B4
=======================================================


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]