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@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>>> ---
>>> 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?
-Eric
