Re: [PATCH] xfs_db: add crc32 self test

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Mon, Oct 29, 2018 at 11:28:53AM -0500, Eric Sandeen wrote:
> 
> 
> On 10/26/18 3:16 PM, Darrick J. Wong wrote:
> > On Fri, Oct 26, 2018 at 03:06:06PM -0500, Eric Sandeen wrote:
> >> On 10/23/18 10:57 AM, Darrick J. Wong wrote:
> >>> From: Darrick J. Wong <darrick.wong@oracle.com>
> >>>
> >>> Add a self test for crc32 into xfs_db so that xfstests can check the
> >>> operation of the (potentially cross-compiled) package binaries by
> >>> isolating the self test code to a header file that can be included by
> >>> the build system self test and xfs_db.
> >>>
> >>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> >>> ---
> >>>  db/command.c            |    1 
> >>>  db/crc.c                |   36 ++
> >>>  db/crc.h                |    1 
> >>>  include/crc32selftest.h |  706 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>  libfrog/crc32.c         |  669 ---------------------------------------------
> >>>  man/man8/xfs_db.8       |    5 
> >>>  6 files changed, 750 insertions(+), 668 deletions(-)
> >>>  create mode 100644 include/crc32selftest.h
> >>>
> >>> diff --git a/db/command.c b/db/command.c
> >>> index c7c52342..7b727986 100644
> >>> --- a/db/command.c
> >>> +++ b/db/command.c
> >>> @@ -139,4 +139,5 @@ init_commands(void)
> >>>  	write_init();
> >>>  	dquot_init();
> >>>  	fuzz_init();
> >>> +	crc32selftest_init();
> >>>  }
> >>> diff --git a/db/crc.c b/db/crc.c
> >>> index b6775bc7..93745003 100644
> >>> --- a/db/crc.c
> >>> +++ b/db/crc.c
> >>> @@ -17,6 +17,7 @@
> >>>  #include "output.h"
> >>>  #include "bit.h"
> >>>  #include "print.h"
> >>> +#include "crc32selftest.h"
> >>>  
> >>>  static int crc_f(int argc, char **argv);
> >>>  static void crc_help(void);
> >>> @@ -175,3 +176,38 @@ crc_f(
> >>>  	flist_free(fl);
> >>>  	return 0;
> >>>  }
> >>> +
> >>> +static int
> >>> +crc32selftest_f(
> >>> +	int		argc,
> >>> +	char		**argv)
> >>> +{
> >>> +	int		c;
> >>> +	int		errors;
> >>> +
> >>> +	while ((c = getopt(argc, argv, "")) != EOF) {
> >>> +		switch (c) {
> >>> +		default:
> >>> +			dbprintf(_("Bad option for crc32selftest command.\n"));
> >>> +			return 0;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	if (argc != optind) {
> >>> +		dbprintf(_("The crc32selftest command takes no arguments.\n"));
> >>> +		return 0;
> >>> +	}
> >>
> >> Isn't this redundant w/ the first "Bad option" message - can we even 
> >> get here?
> > 
> > Hmm, I guess not.
> > 
> >>> +
> >>> +	errors = crc32c_test();
> >>> +	return errors == 0 ? 0 : 1;
> >>
> >> Oh, ok, it can return number of errors, fine.
> > 
> > We might as well stop if crc32 is broken. :)
> 
> Ok, so I think the way this is structured is a bit weird.
> 
> We have to specify a device even though it's not used for
> this command:
> 
> # xfs_db -x -c crc32selftest
> Usage: xfs_db [-ifFrxV] [-p prog] [-l logdev] [-c cmd]... device
> 
> Also, patching to return a forced error still returns 1 on the xfs_db cmdline:
> 
> #xfs_db -x -c crc32selftest fsfile
> crc32c: 1 self tests failed
> # echo $?
> 0
> 
> is that intentional or expected? (I know our subcommand error handling is a mess).
> 
> I guess it does stop processing as expected:
> 
> # xfs_db -x -c crc32selftest -c "sb 0" -c p fsfile
> crc32c: 1 self tests failed
> #
> 
> But maybe this shouldn't be considered a command, and more like a weird bolt-on
> next to 'V' i.e. a top-level option which tests the crc and exits with status.
> I can't think of any reason it needs to be a chained command, can you?

<urk> I forgot that I moved crc32 to libfrog, so yes this should be a
CMD_NOFILE_OK xfs_io command and then we can do:

$ xfs_io -c crc32selftest

Will fix and resubmit.

--D

> -Eric