Re: [PATCH v2 12/17] xfs_scrub: report optional features in version string

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Feb 25, 2022 at 04:04:21PM -0800, Darrick J. Wong wrote:
> On Fri, Feb 25, 2022 at 04:14:13PM -0600, Eric Sandeen wrote:
> > On 1/19/22 7:32 PM, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Ted Ts'o reported brittleness in the fstests logic in generic/45[34] to
> > > detect whether or not xfs_scrub is capable of detecting Unicode mischief
> > > in directory and xattr names.  This is a compile-time feature, since we
> > > do not assume that all distros will want to ship xfsprogs with libicu.
> > > 
> > > Rather than relying on ldd tests (which don't work at all if xfs_scrub
> > > is compiled statically), let's have -V print whether or not the feature
> > > is built into the tool.  Phase 5 still requires the presence of "UTF-8"
> > > in LC_MESSAGES to enable Unicode confusable detection; this merely makes
> > > the feature easier to discover.
> > > 
> > > Reported-by: Theodore Ts'o <tytso@mit.edu>
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > > v2: correct the name of the reporter
> > > ---
> > 
> > Hum, every single other utility just does "$progname version $version"
> > and I'm not that keen to tack on something for everyone, if it won't
> > really mean anything to anyone except xfstests scripts ;)
> > 
> > What about adding an "-F" to display features, and xfstests can use that,
> > and xfs_scrub -V will keep acting like every other utility?
> > 
> > Other utilities could use this too if we ever cared (though xfs_db
> > and xfs_io already have an "-F" option ... we could choose -Z for
> > featureZ, which is unused as a primary option anywhere ...)
> > 
> > like so:
> > 
> > ===
> > 
> > diff --git a/man/man8/xfs_scrub.8 b/man/man8/xfs_scrub.8
> > index e881ae76..65d8f4a2 100644
> > --- a/man/man8/xfs_scrub.8
> > +++ b/man/man8/xfs_scrub.8
> > @@ -8,7 +8,7 @@ xfs_scrub \- check and repair the contents of a mounted XFS filesystem
> >  ]
> >  .I mount-point
> >  .br
> > -.B xfs_scrub \-V
> > +.B xfs_scrub \-V | \-F
> >  .SH DESCRIPTION
> >  .B xfs_scrub
> >  attempts to check and repair all metadata in a mounted XFS filesystem.
> > @@ -76,6 +76,9 @@ If
> >  is given, no action is taken if errors are found; this is the default
> >  behavior.
> >  .TP
> > +.B \-F
> > +Prints the version number along with optional build-time features and exits.
> > +.TP
> >  .B \-k
> >  Do not call TRIM on the free space.
> >  .TP
> > diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
> > index bc2e84a7..9e9a098c 100644
> > --- a/scrub/xfs_scrub.c
> > +++ b/scrub/xfs_scrub.c
> > @@ -582,6 +582,13 @@ report_outcome(
> >  	}
> >  }
> >  
> > +/* Compile-time features discoverable via version strings */
> > +#ifdef HAVE_LIBICU
> > +# define XFS_SCRUB_HAVE_UNICODE	"+"
> > +#else
> > +# define XFS_SCRUB_HAVE_UNICODE	"-"
> > +#endif
> > +
> >  int
> >  main(
> >  	int			argc,
> > @@ -613,7 +620,7 @@ main(
> >  	pthread_mutex_init(&ctx.lock, NULL);
> >  	ctx.mode = SCRUB_MODE_REPAIR;
> >  	ctx.error_action = ERRORS_CONTINUE;
> > -	while ((c = getopt(argc, argv, "a:bC:de:km:nTvxV")) != EOF) {
> > +	while ((c = getopt(argc, argv, "a:bC:de:Fkm:nTvxV")) != EOF) {
> >  		switch (c) {
> >  		case 'a':
> >  			ctx.max_errors = cvt_u64(optarg, 10);
> > @@ -654,6 +661,12 @@ main(
> >  				usage();
> >  			}
> >  			break;
> > +		case 'F':
> > +			fprintf(stdout, _("%s version %s %sUnicode\n"),
> > +					progname, VERSION,
> > +					XFS_SCRUB_HAVE_UNICODE);
> > +			fflush(stdout);
> > +			return SCRUB_RET_SUCCESS;
> 
> Works for me!

Actually, I take it back, let's keep -F unused for now and simply make
'-VV' the magic command that triggers the feature reporting.  I'll send
a v3 with this.

--D

> 
> --D
> 
> >  		case 'k':
> >  			want_fstrim = false;
> >  			break;