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

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello Li,

Li Wang <liwang@redhat.com> writes:

> Hi Richard,
>  
>  > +const struct tst_arch tst_arch = {
>  > +#if defined(__x86_64__)
>  > +        .name = "x86_64",
>
>  Writing out these string constants multiple times is error
>  prone. Perhaps arch_type_list can be indexed with enum tst_arch_type and
>  then name can be ".name = arch_type_list[TST_X86]"?
>
> Right, that will more flexible but you know, all arches we have are just
> those, and we write them only once in the LTP test library.
>
> I slightly wanted to keep string constants to make it more
> simple/readable.

It's a minor thing, but IMO the extra complication is worth it to
eliminate typos.

Although the preprocessor could still hide errors on some arch until
someone tries to compile it.

>  
>  > +static const char *const arch_type_list[] = {
>  > +     "i386",
>  > +     "i586",
>  > +     "i686",
>
>  These are not valid arch names AFAICT. There is no mapping from these to
>  x86 in the tst_arch table above.
>
>  Perhaps we could replace them all with x86?
>
> Yes, that is also the unsure part I was concerned about in patch v4. 
> Because x86 is also an invalid arch (it is just a conventional name),
> if we use it in the arch_type_list we have to recognize it as a valid arch
> name in test case as well. I'm not sure that will be accepted by other
> people.

The folder in the kernel tree is arch/x86.

-- 
Thank you,
Richard.

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