Re: [PATCH 3/4] vfio: selftests: Allow a size for vfio_dma_mapping_perf_test

> > +             shift = 10;
> > +             break;
> > +     case 'b':
> > +     case '\0':
> > +             shift = 0;
> > +             break;
> > +     default:
> > +             VFIO_FAIL("Unknown size letter '%c'.", *scale);
> > +     }
> > +
> > +     VFIO_ASSERT_TRUE((base << shift) >> shift == base,
> > +                      "Overflow scaling size!");
> > +
> > +     return base << shift;
> > +}
>
> I'd like to put this in the library but it's going to conflict with KVM
> selftests parse_size() helper when we link libvfio into KVM selftests.
>
> I'd like to unify the common helpers between libvfio and KVM selftests
> library but that is future work. In the meantime I guess we can keep
> this local.
>

Agreed, this would be good to do as a follow up.

> > +
> > +static void help(char *name)
> > +{
> > +     puts("");
> > +     printf("usage: %s [-h] [-b bytes] [-a \"test harness args\"]\n", name);
> > +     puts("");
> > +     printf(" -h: Display this help message.\n"
> > +            " -b: Specify the size of the DMA region to be mapped\n"
> > +            "     and unmapped. e.g. 16M or 8G, (default: 1G)\n"
> > +            " -a: Args that are forwarded to the test harness,\n"
> > +            "     e.g. -a \"-t dma_map_unmap_from_file\"\n");
>
> Should this also call test_harness_run(1, "-h") to print its help
> message?
>
> > +}
> > +
> > +struct harness_args
> > +{
> > +     int argc;
> > +     char **argv;
> > +     wordexp_t exp;
> > +};
> > +
> > +static void populate_harness_args(struct harness_args *args, const char *argv_0,
> > +                               const char *cmdlne)
> > +{
> > +     if (wordexp(argv_0, &args->exp, WRDE_NOCMD) == 0 &&
> > +         wordexp(cmdlne, &args->exp, WRDE_APPEND | WRDE_NOCMD) == 0) {
> > +             args->argc = args->exp.we_wordc;
> > +             args->argv = args->exp.we_wordv;
> > +     }
>
> Is this missing error handling? Also maybe add some comments since
> wordexp() is not a very commonly used helper.
>
> > +}
> > +
> > +static void setup_test(struct harness_args *args, int argc, char *argv[])
> > +{
> > +     int opt;
> > +
> > +     test_params = (struct test_params) {
> > +             .size = SZ_1G,
> > +     };
>
> Do this where test_params is declared, not here.
>
>   static struct test_params test_params = {
>           .size = SZ_1G,
>   };
>
> BTW, this change will force all tests to use 1GiB instead of their
> current defaults (PAGE_SIZE, 2M, and 1G respectively).
>
> So this change is not purely adding a command line option. I think this
> change should probably go into the first commit that introduces the
> test, not this one.

The tests were explicitly set to 1GB in the previous commits for all
variants.  This commit allows the user to change that to a user
defined value, with the default still set to 1GB.  So it really should
just be additive.

I left this as its own commit because it is only additive and should
be easier to review on its own.  If you still think it should be a
part of the first commit I can do that.

>
> > +
> > +     while ((opt = getopt(argc, argv, "a:b:h")) != -1) {
> > +             switch (opt) {
> > +             case 'a':
> > +                     populate_harness_args(args, argv[0], optarg);
> > +                     break;
> > +             case 'b':
> > +                     test_params.size = parse_size(optarg);
> > +                     break;
> > +             case 'h':