Re: [LTP] [PATCH v2 1/3] lib: adding .arch field in tst_test structure

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> > least array of supported architectures in the test_test structure would
>> > be a good addition.
>> >
>> 
>> I guess defining .arch as a string and making a valid check will be enough.
>> Array for that sounds a bit complicated in use.
>
> Quite the opposite, it should be an array of strings, so that it's easy
> to work with such as:
>
> 	.supported_archs = (const char *const []){"x86_64", "ppc64le", NULL},
>
> We can put it into a single string delimited by a space, but that would
> be more complicated to work with.
>
>> > However the hard part would be keeping the actual code and metadata in
>> > sync, we still have to keep the ifdefs in the code.
>> >
>> 
>> Yes, some inline assemble require ifdefs.
>> 
>> Btw, I look back at the reviews and find Jan said:
>>     "I can see how tst_on_arch() would be useful. Test is valid
>>      on all arches, but needs different input/constants/code/etc."
>> 
>> That may be a slight reason for keeping tst_on_arch.
>
> I guess that we should reviewe the code we have, I guess that there are
> a few tests where we can get rid of a few ifdefs by doing the checks
> dynamically.
>
> Also I guess that it would be slightly easier to work with as an enum,
> so that we can do:
>
> 	switch (tst_arch) {
> 	case TST_X86_64:
> 		...
> 	break;
> 	case TST_PPC64_LE:

I prefer enum as well. As an aside, we don't want to include LE in
ppc64. If someone finds that the byte order is significant for a test
then we can add ppc64le or ppc64be. Also at some point we may need to
add a "machine" field for e.g. POWER8, i386 etc.

Which btw, I have some buildroot and QEMU scripts which can be used to
test ppc64 BE and any other machine you have the hardware or QEMU
emulator for.

https://gitlab.com/Palethorpe/cross

> 		...
> 	break;
> 	default:
> 		...
> 	break;
> 	}
>
> instead of:
> 	if (!strcmp(tst_arch, "x86_64"))
> 		...
> 	else if (!strmcp(tst_arch, ...)))
> 		...
> 	else
> 		...


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp