Message ID | 20190615042048.29839-1-liwang@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series | [v2,1/3] lib: adding .arch field in tst_test structure | expand |
Hi Li, > Testcases for specified arch should be limited on that only being supported > platform to run, we now create a function tst_on_arch to achieve this new > feature in LTP library. All you need to run a test on the expected arch is > to set the '.arch' string in the 'struct tst_test' to choose the required > arch list. e.g. '.arch = "x86_64 i386"'. > Signed-off-by: Li Wang <liwang@redhat.com> Reviewed-by: Petr Vorel <pvorel@suse.cz> LGTM. FYI there is also HOST_CPU variable (since 00ff2c348f), but that does not help your patch. > --- > doc/test-writing-guidelines.txt | 26 ++++++++++ > include/tst_arch.h | 16 ++++++ > include/tst_test.h | 7 ++- > lib/tst_arch.c | 92 +++++++++++++++++++++++++++++++++ > 4 files changed, 140 insertions(+), 1 deletion(-) > create mode 100644 include/tst_arch.h > create mode 100644 lib/tst_arch.c > diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt > index f1912dc12..b4fba0190 100644 > --- a/doc/test-writing-guidelines.txt > +++ b/doc/test-writing-guidelines.txt > @@ -1668,6 +1668,32 @@ sturct tst_test test = { > }; > ------------------------------------------------------------------------------- > +2.2.30 Testing on specified architecture ^ maybe specific? Kind regards, Petr
Hi Li, > Testcases for specified arch should be limited on that only being supported > platform to run, we now create a function tst_on_arch to achieve this new > feature in LTP library. All you need to run a test on the expected arch is > to set the '.arch' string in the 'struct tst_test' to choose the required > arch list. e.g. '.arch = "x86_64 i386"'. Just one note, strings are error prone. I wonder if defining constants and using array would be too complicated for this use case. Kind regards, Petr
On Tue, Jun 18, 2019 at 5:46 AM Petr Vorel <pvorel@suse.cz> wrote: > Hi Li, > > > Testcases for specified arch should be limited on that only being > supported > > platform to run, we now create a function tst_on_arch to achieve this new > > feature in LTP library. All you need to run a test on the expected arch > is > > to set the '.arch' string in the 'struct tst_test' to choose the required > > arch list. e.g. '.arch = "x86_64 i386"'. > > > Signed-off-by: Li Wang <liwang@redhat.com> > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > LGTM. FYI there is also HOST_CPU variable (since 00ff2c348f), > but that does not help your patch. > > > --- > > doc/test-writing-guidelines.txt | 26 ++++++++++ > > include/tst_arch.h | 16 ++++++ > > include/tst_test.h | 7 ++- > > lib/tst_arch.c | 92 +++++++++++++++++++++++++++++++++ > > 4 files changed, 140 insertions(+), 1 deletion(-) > > create mode 100644 include/tst_arch.h > > create mode 100644 lib/tst_arch.c > > > diff --git a/doc/test-writing-guidelines.txt > b/doc/test-writing-guidelines.txt > > index f1912dc12..b4fba0190 100644 > > --- a/doc/test-writing-guidelines.txt > > +++ b/doc/test-writing-guidelines.txt > > @@ -1668,6 +1668,32 @@ sturct tst_test test = { > > }; > > > ------------------------------------------------------------------------------- > > > +2.2.30 Testing on specified architecture > ^ maybe specific? > Agree.
On Tue, Jun 18, 2019 at 5:49 AM Petr Vorel <pvorel@suse.cz> wrote: > Hi Li, > > > Testcases for specified arch should be limited on that only being > supported > > platform to run, we now create a function tst_on_arch to achieve this new > > feature in LTP library. All you need to run a test on the expected arch > is > > to set the '.arch' string in the 'struct tst_test' to choose the required > > arch list. e.g. '.arch = "x86_64 i386"'. > Just one note, strings are error prone. I wonder if defining constants and > using > array would be too complicated for this use case. > I thought about that array way, but it seems a bit complicted in using. Yes, strings are easy to involve typo but we can make a check in library. Thanks for review.
Hi Li, > > > Testcases for specified arch should be limited on that only being > > supported > > > platform to run, we now create a function tst_on_arch to achieve this new > > > feature in LTP library. All you need to run a test on the expected arch > > is > > > to set the '.arch' string in the 'struct tst_test' to choose the required > > > arch list. e.g. '.arch = "x86_64 i386"'. > > Just one note, strings are error prone. I wonder if defining constants and > > using > > array would be too complicated for this use case. > I thought about that array way, but it seems a bit complicted in using. > Yes, strings are easy to involve typo but we can make a check in library. OK, agree :). > Thanks for review. Kind regards, Petr
Hello Li, Li Wang <liwang@redhat.com> writes: > Testcases for specified arch should be limited on that only being supported > platform to run, we now create a function tst_on_arch to achieve this new > feature in LTP library. All you need to run a test on the expected arch is > to set the '.arch' string in the 'struct tst_test' to choose the required > arch list. e.g. '.arch = "x86_64 i386"'. What is the status of this patch series? Is there a V3? Maybe another option would be to check the kernel config?
Hi Richard, On Wed, Nov 3, 2021 at 8:10 PM Richard Palethorpe <rpalethorpe@suse.de> wrote: > Hello Li, > > Li Wang <liwang@redhat.com> writes: > > > Testcases for specified arch should be limited on that only being > supported > > platform to run, we now create a function tst_on_arch to achieve this new > > feature in LTP library. All you need to run a test on the expected arch > is > > to set the '.arch' string in the 'struct tst_test' to choose the required > > arch list. e.g. '.arch = "x86_64 i386"'. > > What is the status of this patch series? Is there a V3? > Um, I can't recall why the V3 development for .arch was suspended. Maybe we thought there is not much sense to replace ifdef __arch__ from code at that moment. Now, if that can benefit the tst_test metadata more in the next runltp-ng. Should we keep going to work out the patch V3? @Cyril Hrubis <chrubis@suse.cz> any suggestions? > > Maybe another option would be to check the kernel config? > That maybe works, but I'm not sure if that can distinguish i386/x86_64 or ppc64/ppc64le.
Hi! > Um, I can't recall why the V3 development for .arch > was suspended. > > Maybe we thought there is not much sense to replace > ifdef __arch__ from code at that moment. > > Now, if that can benefit the tst_test metadata more in the next runltp-ng. > Should we keep going to work out the patch V3? > > @Cyril Hrubis <chrubis@suse.cz> any suggestions? Hmm, I guess that I said that it still makes to add the metadata, at least array of supported architectures in the test_test structure would be a good addition. However the hard part would be keeping the actual code and metadata in sync, we still have to keep the ifdefs in the code.
On Wed, Nov 3, 2021 at 10:09 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > Um, I can't recall why the V3 development for .arch > > was suspended. > > > > Maybe we thought there is not much sense to replace > > ifdef __arch__ from code at that moment. > > > > Now, if that can benefit the tst_test metadata more in the next > runltp-ng. > > Should we keep going to work out the patch V3? > > > > @Cyril Hrubis <chrubis@suse.cz> any suggestions? > > Hmm, I guess that I said that it still makes to add the metadata, at > Agreed. > 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. > > 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.
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: ... break; default: ... break; } instead of: if (!strcmp(tst_arch, "x86_64")) ... else if (!strmcp(tst_arch, ...))) ... else ...
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 > ...
> > 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 > Sure, but I'm now thinking to extend the tst_arch as a structure so that could also be used in a string: enum tst_arch_type { TST_I386, TST_X86_64, ... TST_SPARC, }; /* * This tst_arch is to save the system architecture for * using in the whole test case. */ extern struct arch { const char name[16]; enum tst_arch_type type; } tst_arch; then we just can do simply in case: switch (tst_arch.type) { case TST_X86_64: ... break; > ppc64. If someone finds that the byte order is significant for a test > Yes, or we can read info via uname() into 'utsname.machine' for ppc64le if really needed. > 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. > Adding a new field '.machine' maybe not be necessary if just for POWER8/9/10, or can we find a way to combine them together with .supported_arch? Umm, I'm still hesitating. > > 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 Thanks for sharing.
Hell Li, Li Wang <liwang@redhat.com> writes: > > > > 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 > > Sure, but I'm now thinking to extend the tst_arch as a structure > so that could also be used in a string: +1 > > enum tst_arch_type { > TST_I386, > TST_X86_64, > ... > TST_SPARC, > }; > > /* > * This tst_arch is to save the system architecture for > * using in the whole test case. > */ > extern struct arch { > const char name[16]; > enum tst_arch_type type; > } tst_arch; > > then we just can do simply in case: > > switch (tst_arch.type) { > case TST_X86_64: > ... > break; > > > ppc64. If someone finds that the byte order is significant for a test > > Yes, or we can read info via uname() into 'utsname.machine' for > ppc64le if really needed. > > 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. > > Adding a new field '.machine' maybe not be necessary if just > for POWER8/9/10, or can we find a way to combine them together > with .supported_arch? Umm, I'm still hesitating. If it's required then I guess you could add it to the tst_arch_type as an optional field. Perhaps as cpu_model. Or it could be added to a separate section for required hardware. > > > 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 > > Thanks for sharing.
Hi! > > > 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 > > > > Sure, but I'm now thinking to extend the tst_arch as a structure > so that could also be used in a string: > > enum tst_arch_type { > TST_I386, > TST_X86_64, > ... > TST_SPARC, > }; > > /* > * This tst_arch is to save the system architecture for > * using in the whole test case. > */ > extern struct arch { > const char name[16]; > enum tst_arch_type type; > } tst_arch; Or we can add a function to translate the enums into strings as: const char *tst_arch_name(enum tst_arch arch); > then we just can do simply in case: > > switch (tst_arch.type) { > case TST_X86_64: > ... > break; > > > > > ppc64. If someone finds that the byte order is significant for a test > > > > Yes, or we can read info via uname() into 'utsname.machine' for > ppc64le if really needed. > > > > 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. > > > > Adding a new field '.machine' maybe not be necessary if just > for POWER8/9/10, or can we find a way to combine them together > with .supported_arch? Umm, I'm still hesitating. This is going to get complicated quite fast. I do not think that we need such granularity now, but if we ever do I think it can be done as a subtype. E.g.: enum tst_arch { TST_X86, TST_X86_64, TST_PPC64, }; enum tst_x86_subarch { TST_386, TST_486, TST_686, }; enum tst_ppc_subarch { { TST_POWER8, TST_POWER9, TST_POWER10, };
diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt index f1912dc12..b4fba0190 100644 --- a/doc/test-writing-guidelines.txt +++ b/doc/test-writing-guidelines.txt @@ -1668,6 +1668,32 @@ sturct tst_test test = { }; ------------------------------------------------------------------------------- +2.2.30 Testing on specified architecture +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Testcases for specified arch should be limited on that only being supported +platform to run, we now create a function tst_on_arch to achieve this new +feature in LTP library. All you need to run a test on the expected arch is +to set the '.arch' string in the 'struct tst_test' to choose the required +arch list. e.g. '.arch = "x86_64 i386"'. + +[source,c] +------------------------------------------------------------------------------- +#include "tst_test.h" + +static void setup(void) +{ + ... +} + +static struct tst_test test = { + ... + .setup = setup, + .arch = "x86_64 i386", + ... +} +------------------------------------------------------------------------------- + 2.3 Writing a testcase in shell ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/include/tst_arch.h b/include/tst_arch.h new file mode 100644 index 000000000..7bf0493ce --- /dev/null +++ b/include/tst_arch.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later + * Copyright (c) 2019 Li Wang <liwang@redhat.com> + */ + +#ifndef TST_ARCH_H__ +#define TST_ARCH_H__ + +/* + * Check if test platform is in the given arch list. If yes return 1, + * otherwise return 0. + * + * @arch, NULL or vliad arch list + */ +int tst_on_arch(const char *arch); + +#endif /* TST_ARCH_H__ */ diff --git a/include/tst_test.h b/include/tst_test.h index 8bdf38482..cafcb1a89 100644 --- a/include/tst_test.h +++ b/include/tst_test.h @@ -28,6 +28,7 @@ #include "tst_atomic.h" #include "tst_kvercmp.h" #include "tst_clone.h" +#include "tst_arch.h" #include "tst_kernel.h" #include "tst_minmax.h" #include "tst_get_bad_addr.h" @@ -114,6 +115,8 @@ struct tst_test { const char *min_kver; + const char *arch; + /* If set the test is compiled out */ const char *tconf_msg; @@ -253,7 +256,6 @@ const char *tst_strstatus(int status); unsigned int tst_timeout_remaining(void); void tst_set_timeout(int timeout); - /* * Returns path to the test temporary directory in a newly allocated buffer. */ @@ -265,6 +267,9 @@ static struct tst_test test; int main(int argc, char *argv[]) { + if (!tst_on_arch(test.arch)) + tst_brk(TCONF, "Test needs running on %s arch!", test.arch); + tst_run_tcases(argc, argv, &test); } diff --git a/lib/tst_arch.c b/lib/tst_arch.c new file mode 100644 index 000000000..abc6f0722 --- /dev/null +++ b/lib/tst_arch.c @@ -0,0 +1,92 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later + * Copyright (c) 2019 Li Wang <liwang@redhat.com> + */ + +#include <string.h> +#include <stdlib.h> + +#define TST_NO_DEFAULT_MAIN +#include "tst_arch.h" +#include "tst_test.h" + +static const char *const arch_type_list[] = { + "i386", + "x86", + "x86_64", + "ia64", + "ppc", + "ppc64", + "s390", + "s390x", + "arm", + "aarch64", + "sparc", + NULL +}; + +static int is_matched_arch(const char *arch, const char *tst_arch) +{ + char *dup_arch, *p; + const char *delim = " "; + + dup_arch = strdup(arch); + + p = strtok(dup_arch, delim); + if (strcmp(p, tst_arch) == 0) + return 1; + + while ((p = strtok(NULL, delim))) { + if (strcmp(p, tst_arch) == 0) + return 1; + } + + free(dup_arch); + return 0; +} + +static int is_valid_arch(const char *arch) +{ + unsigned int i; + + for (i = 0; arch_type_list[i]; i++) { + if (is_matched_arch(arch, arch_type_list[i])) + return 1; + } + + return 0; +} + +int tst_on_arch(const char *arch) +{ +#if defined(__i386__) + char *tst_arch = "i386"; +#elif defined(__x86__) + char *tst_arch = "x86"; +#elif defined(__x86_64__) + char *tst_arch = "x86_64"; +#elif defined(__ia64__) + char *tst_arch = "ia64"; +#elif defined(__powerpc__) + char *tst_arch = "ppc"; +#elif defined(__powerpc64__) + char *tst_arch = "ppc64"; +#elif defined(__s390__) + char *tst_arch = "s390"; +#elif defined(__s390x__) + char *tst_arch = "s390x"; +#elif defined(__arm__) + char *tst_arch = "arm"; +#elif defined(__arch64__) + char *tst_arch = "aarch64"; +#elif defined(__sparc__) + char *tst_arch = "sparc"; +#endif + + if (arch != NULL && !is_valid_arch(arch)) + tst_brk(TBROK, "please set valid arches!"); + + if (arch == NULL || is_matched_arch(arch, tst_arch)) + return 1; + + return 0; +}
Testcases for specified arch should be limited on that only being supported platform to run, we now create a function tst_on_arch to achieve this new feature in LTP library. All you need to run a test on the expected arch is to set the '.arch' string in the 'struct tst_test' to choose the required arch list. e.g. '.arch = "x86_64 i386"'. Signed-off-by: Li Wang <liwang@redhat.com> --- doc/test-writing-guidelines.txt | 26 ++++++++++ include/tst_arch.h | 16 ++++++ include/tst_test.h | 7 ++- lib/tst_arch.c | 92 +++++++++++++++++++++++++++++++++ 4 files changed, 140 insertions(+), 1 deletion(-) create mode 100644 include/tst_arch.h create mode 100644 lib/tst_arch.c