On Mon, Oct 29, 2018 at 01:30:32PM -0500, Eric Sandeen wrote:
> On 10/29/18 1:11 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Add a self test for crc32 into xfs_io 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_io.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > include/crc32selftest.h | 706 +++++++++++++++++++++++++++++++++++++++++++++++
> > io/Makefile | 8 -
> > io/crc32selftest.c | 51 +++
> > io/init.c | 1
> > io/io.h | 1
> > libfrog/crc32.c | 669 ---------------------------------------------
>
> needs a manpage change as well I think.
Dangit, I forgot to port that over when I moved the command.
> > 6 files changed, 764 insertions(+), 672 deletions(-)
> > create mode 100644 include/crc32selftest.h
> > create mode 100644 io/crc32selftest.c
> >
>
> ...
>
> > diff --git a/io/crc32selftest.c b/io/crc32selftest.c
> > new file mode 100644
> > index 00000000..c0b4b2f4
> > --- /dev/null
> > +++ b/io/crc32selftest.c
> > @@ -0,0 +1,51 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 Oracle, Inc.
> > + * All Rights Reserved.
> > + */
> > +
> > +#include "platform_defs.h"
> > +#include "command.h"
> > +#include "init.h"
> > +#include "io.h"
> > +#include "crc32c.h"
> > +#include "crc32selftest.h"
> > +
> > +static int
> > +crc32selftest_f(
> > + int argc,
> > + char **argv)
> > +{
> > + int c;
> > + int errors;
> > +
> > + while ((c = getopt(argc, argv, "")) != EOF) {
> > + switch (c) {
> > + default:
> > + fprintf(stderr,
> > +_("Bad option for crc32selftest command.\n"));
> > + return 0;
> > + }
> > + }
>
> I think we can drop the getopt bits altogether because:
>
> > + errors = crc32c_test();
> > + return errors == 0 ? 0 : 1;
> > +}
> > +
> > +static const cmdinfo_t crc32selftest_cmd = {
> > + .name = "crc32selftest",
> > + .cfunc = crc32selftest_f,
> > + .argmin = 0,
> > + .argmax = 0,
>
> This command structure doesn't even let us get there:
>
> # io/xfs_io -c "crc32selftest foo"
> bad argument count 1 to crc32selftest, expected 0 arguments
> # io/xfs_io -c "crc32selftest -f"
> bad argument count 1 to crc32selftest, expected 0 arguments
>
> and other "argmax" functions (freeze, thaw, close, munmap...) don't bother
> with getopt at all.
ok.
> > + .canpush = 0,
> > + .args = "",
>
> drop this I think, otherwise we get a weird space in usage:
>
> crc32selftest -- crc32c self test
>
> vs
>
> crc32selftest -- crc32c self test
>
> because
>
> if (ct->args)
> printf("%s ", ct->args);
> printf("-- %s\n", ct->oneline);
>
Aha, that's why it does that...
> > + .flags = CMD_FLAG_ONESHOT | CMD_FLAG_FOREIGN_OK |
> > + CMD_NOFILE_OK | CMD_NOMAP_OK,
> > + .oneline = N_("crc32c self test"),
>
> > +};
> > +
> > +void
> > +crc32selftest_init(void)
> > +{
> > + add_command(&crc32selftest_cmd);
> > +}
>
> ok is it better to call this crc32c selftest or crc32 selftest? Seems a little inconsistent in the naming.
crc32cselftest it is then.
--D
> > diff --git a/io/init.c b/io/init.c
> > index b5eade39..53b754e4 100644
> > --- a/io/init.c
> > +++ b/io/init.c
> > @@ -85,6 +85,7 @@ init_commands(void)
> > sync_range_init();
> > truncate_init();
> > utimes_init();
> > + crc32selftest_init();
> > }
> >
> > /*
> > diff --git a/io/io.h b/io/io.h
> > index 9278ad00..847b4759 100644
> > --- a/io/io.h
> > +++ b/io/io.h
> > @@ -182,3 +182,4 @@ extern void log_writes_init(void);
> >
> > extern void scrub_init(void);
> > extern void repair_init(void);
> > +extern void crc32selftest_init(void);
>
>
