Quoting Claudio Imbrenda (2022-12-13 18:27:56)
[...]
> > diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> > index b7bd82581abe..9b9a45f4ad3b 100644
> > --- a/s390x/migration-skey.c
> > +++ b/s390x/migration-skey.c
[...]
> > +/*
> > + * Verify storage keys on pagebuf.
> > + * Storage keys must have been set by skey_set_test_pattern on pagebuf before.
> > + * skey_set_keys must have been called with the same seed value.
> > + *
> > + * If storage keys match the expected result, will return a skey_verify_result
> > + * with verify_failed false. All other fields are then invalid.
> > + * If there is a mismatch, returned struct will have verify_failed true and will
> > + * be filled with the details on the first mismatch encountered.
> > + */
> > +static struct skey_verify_result skey_verify_test_pattern(uint8_t *pagebuf, unsigned long page_count, unsigned char seed)
>
> this line is a little too long, even for the KVM unit tests; find a way
> to shorten it by a couple of bytes (it would look better to keep it on
> one line)
>
> (rename pagebuf? use size_t or uint64_t instead of unsigned long? use
> uint8_t for seed? shorten the name of struct verify_result?)
Well since all these functions are static again, I will:
- remove the skey_ prefix
- get rid of the pagebuf argument and just use the global variable.
That makes everything nice and short.
[...]
> > +static void migrate_once(void)
> > +{
> > + static bool migrated;
> > +
> > + if (migrated)
> > + return;
> > +
> > + migrated = true;
> > + puts("Please migrate me, then press return\n");
> > + (void)getchar();
> > }
>
> you don't need this any more, since your migration patches are
> upstream now :)
Yes, rebased.
[...]
> > +static void print_usage(void)
> > +{
> > + report_info("Usage: migration-skey [parallel|sequential]");
> > +}
> > +
> > +int main(int argc, char **argv)
> > {
> > report_prefix_push("migration-skey");
> > if (test_facility(169)) {
> > report_skip("storage key removal facility is active");
> > + goto error;
> > + }
> >
> > - /*
> > - * If we just exit and don't ask migrate_cmd to migrate us, it
> > - * will just hang forever. Hence, also ask for migration when we
> > - * skip this test altogether.
> > - */
> > - puts("Please migrate me, then press return\n");
> > - (void)getchar();
> > + if (argc < 2) {
> > + print_usage();
> > + goto error;
> > + }
> > +
> > + if (!strcmp("parallel", argv[1])) {
> > + test_skey_migration_parallel();
> > + } else if (!strcmp("sequential", argv[1])) {
> > + test_skey_migration_sequential();
> > } else {
> > - test_migration();
> > + print_usage();
>
> hmm I don't like this whole main.
>
> I think the best approach would be to have some kind of separate
> parse_args function that will set some flags (or one flag, in this case)
>
> then the main becomes just a bunch of if/else, something like this:
>
> parse_args(argc, argv);
> if (test_facility(169))
> skip
> else if (parallel_flag)
> parallel();
> else
> sequential();
>
> also, default to sequential if there are no parameters, and use
> posix-style parameters (--parallel, --sequential)
>
> only if you get an invalid parameter you can print the usage and fail
> optionally if you really want you can print the usage info when called
> without parameter and inform that the test will default to sequential
>
>
> one of these days I have to write an argument parsing library :)
Alright, done.
