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