<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jun 12, 2019 at 11:49 PM Cyril Hrubis <<a href="mailto:chrubis@suse.cz">chrubis@suse.cz</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi!<br>
> Testcases for specified arch should be limited on that only being supported<br>
> platform to run, we now create a function tst_on_arch to achieve this new<br>
> feature in LTP library. All you need to run a test on the expected arch is<br>
> to set the '.arch' string in the 'struct tst_test' to choose the required<br>
> arch list. e.g. '.arch = "x86_64, i386"'.<br>
<br>
There is no point in adding the coma between the architectures, i.e.<br>
this should be just .arch = "x86_64 i386".<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Agree.</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> For more complicated usages such as customize your test code for a special<br>
> arch, the tst_on_arch function could be invoked in the test directly.<br>
> <br>
> Signed-off-by: Li Wang <<a href="mailto:liwang@redhat.com" target="_blank">liwang@redhat.com</a>><br>
> ---<br>
> doc/test-writing-guidelines.txt | 54 +++++++++++++++++++++++++<br>
> include/tst_arch.h | 16 ++++++++<br>
> include/tst_test.h | 7 +++-<br>
> lib/tst_arch.c | 71 +++++++++++++++++++++++++++++++++<br>
> 4 files changed, 147 insertions(+), 1 deletion(-)<br>
> create mode 100644 include/tst_arch.h<br>
> create mode 100644 lib/tst_arch.c<br>
> <br>
> diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt<br>
> index f1912dc12..10442d756 100644<br>
> --- a/doc/test-writing-guidelines.txt<br>
> +++ b/doc/test-writing-guidelines.txt<br>
> @@ -1668,6 +1668,60 @@ sturct tst_test test = {<br>
> };<br>
> -------------------------------------------------------------------------------<br>
> <br>
> +2.2.30 Testing on specified architecture<br>
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^<br>
> +<br>
> +Testcases for specified arch should be limited on that only being supported<br>
> +platform to run, we now create a function tst_on_arch to achieve this new<br>
> +feature in LTP library. All you need to run a test on the expected arch is<br>
> +to set the '.arch' string in the 'struct tst_test' to choose the required<br>
> +arch list. e.g. '.arch = "x86_64, i386"'.<br>
> +<br>
> +[source,c]<br>
> +-------------------------------------------------------------------------------<br>
> +#include "tst_test.h"<br>
> +<br>
> +static void setup(void)<br>
> +{<br>
> + ...<br>
> +}<br>
> +<br>
> +static struct tst_test test = {<br>
> + ...<br>
> + .setup = setup,<br>
> + .arch = "x86_64, i386",<br>
> + ...<br>
> +}<br>
> +-------------------------------------------------------------------------------<br>
> +<br>
> +For more complicated usages such as customize your test code for a special<br>
> +arch, the tst_on_arch function could be invoked in the test directly.<br>
> +<br>
> +[source,c]<br>
> +-------------------------------------------------------------------------------<br>
> +#include "tst_test.h"<br>
> +<br>
> +static void do_test(void)<br>
> +{<br>
> + if (tst_on_arch("x86_64, i386")) {<br>
> + /* code for x86_64, i386 */<br>
> + ...<br>
> + } else if (tst_on_arch("ppc64")) {<br>
> + /* code for ppc64 */<br>
> + ...<br>
> + } else if (tst_on_arch("s390, s390x")) {<br>
> + /* code for s390x */<br>
> + ...<br>
> + }<br>
<br>
What is the point of the runtime detection here?<br>
<br>
It's not like we can run s390x binary on i386, i.e. we know for which<br>
architecture we are compiling for at the compile time.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">You are right. But we still have some chance to do analysis at runtime, if you take a look at patch 2/3, e.g. to parse '/proc/<pid>/maps' in max_map_count.c can be done at runtime detection. That's what I thought we can export the tst_on_arch() as a global function. </div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +}<br>
> +<br>
> +static struct tst_test test = {<br>
> + ...<br>
> + .test_all = do_test,<br>
> + ...<br>
> +}<br>
> +-------------------------------------------------------------------------------<br>
> +<br>
> <br>
> 2.3 Writing a testcase in shell<br>
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~<br>
> diff --git a/include/tst_arch.h b/include/tst_arch.h<br>
> new file mode 100644<br>
> index 000000000..7bf0493ce<br>
> --- /dev/null<br>
> +++ b/include/tst_arch.h<br>
> @@ -0,0 +1,16 @@<br>
> +/* SPDX-License-Identifier: GPL-2.0-or-later<br>
> + * Copyright (c) 2019 Li Wang <<a href="mailto:liwang@redhat.com" target="_blank">liwang@redhat.com</a>><br>
> + */<br>
> +<br>
> +#ifndef TST_ARCH_H__<br>
> +#define TST_ARCH_H__<br>
> +<br>
> +/*<br>
> + * Check if test platform is in the given arch list. If yes return 1,<br>
> + * otherwise return 0.<br>
> + *<br>
> + * @arch, NULL or vliad arch list<br>
> + */<br>
> +int tst_on_arch(const char *arch);<br>
> +<br>
> +#endif /* TST_ARCH_H__ */<br>
> diff --git a/include/tst_test.h b/include/tst_test.h<br>
> index 8bdf38482..cafcb1a89 100644<br>
> --- a/include/tst_test.h<br>
> +++ b/include/tst_test.h<br>
> @@ -28,6 +28,7 @@<br>
> #include "tst_atomic.h"<br>
> #include "tst_kvercmp.h"<br>
> #include "tst_clone.h"<br>
> +#include "tst_arch.h"<br>
> #include "tst_kernel.h"<br>
> #include "tst_minmax.h"<br>
> #include "tst_get_bad_addr.h"<br>
> @@ -114,6 +115,8 @@ struct tst_test {<br>
> <br>
> const char *min_kver;<br>
> <br>
> + const char *arch;<br>
> +<br>
> /* If set the test is compiled out */<br>
> const char *tconf_msg;<br>
> <br>
> @@ -253,7 +256,6 @@ const char *tst_strstatus(int status);<br>
> unsigned int tst_timeout_remaining(void);<br>
> void tst_set_timeout(int timeout);<br>
> <br>
> -<br>
> /*<br>
> * Returns path to the test temporary directory in a newly allocated buffer.<br>
> */<br>
> @@ -265,6 +267,9 @@ static struct tst_test test;<br>
> <br>
> int main(int argc, char *argv[])<br>
> {<br>
> + if (!tst_on_arch(test.arch))<br>
> + tst_brk(TCONF, "Test needs running on %s arch!", test.arch);<br>
> +<br>
> tst_run_tcases(argc, argv, &test);<br>
> }<br>
<br>
This may be a bit cleaner that compiling the test out, but will not save<br>
us from arch specific ifdefs completely so I'm not sure it's worth the<br>
trouble.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Indeed, I also realized that after signing off this patch, we can't replace ifdefs completely via a simple function, since it occurring in the compiling early phase. But anyway I roll out this for comments in case we could find an improved way to do better.</div><div class="gmail_default" style="font-size:small"> </div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> diff --git a/lib/tst_arch.c b/lib/tst_arch.c<br>
> new file mode 100644<br>
> index 000000000..a9f2775b4<br>
> --- /dev/null<br>
> +++ b/lib/tst_arch.c<br>
> @@ -0,0 +1,71 @@<br>
> +/* SPDX-License-Identifier: GPL-2.0-or-later<br>
> + * Copyright (c) 2019 Li Wang <<a href="mailto:liwang@redhat.com" target="_blank">liwang@redhat.com</a>><br>
> + */<br>
> +<br>
> +#include <string.h><br>
> +<br>
> +#define TST_NO_DEFAULT_MAIN<br>
> +#include "tst_arch.h"<br>
> +#include "tst_test.h"<br>
> +<br>
> +static const char *const arch_type_list[] = {<br>
> + "i386",<br>
> + "x86",<br>
> + "x86_64",<br>
> + "ia64",<br>
> + "ppc",<br>
> + "ppc64",<br>
> + "s390",<br>
> + "s390x",<br>
> + "arm",<br>
> + "aarch64",<br>
> + "sparc",<br>
> + NULL<br>
> +};<br>
> +<br>
> +static int is_valid_arch(const char *arch)<br>
> +{<br>
> + unsigned int i;<br>
> +<br>
> + for (i = 0; arch_type_list[i]; i++) {<br>
> + if (strstr(arch, arch_type_list[i]))<br>
> + return 1;<br>
> + }<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +int tst_on_arch(const char *arch)<br>
> +{<br>
> +#if defined(__i386__)<br>
> + char *tst_arch = "i386";<br>
> +#elif defined(__x86__)<br>
> + char *tst_arch = "x86";<br>
> +#elif defined(__x86_64__)<br>
> + char *tst_arch = "x86_64";<br>
> +#elif defined(__ia64__)<br>
> + char *tst_arch = "ia64";<br>
> +#elif defined(__powerpc__)<br>
> + char *tst_arch = "ppc";<br>
> +#elif defined(__powerpc64__)<br>
> + char *tst_arch = "ppc64";<br>
> +#elif defined(__s390__)<br>
> + char *tst_arch = "s390";<br>
> +#elif defined(__s390x__)<br>
> + char *tst_arch = "s390x";<br>
> +#elif defined(__arm__)<br>
> + char *tst_arch = "arm";<br>
> +#elif defined(__arch64__)<br>
> + char *tst_arch = "aarch64";<br>
> +#elif defined(__sparc__)<br>
> + char *tst_arch = "sparc";<br>
> +#endif<br>
> +<br>
> + if (arch != NULL && !is_valid_arch(arch))<br>
> + tst_brk(TBROK, "please set valid arches!");<br>
> +<br>
> + if (arch == NULL || strstr(arch, tst_arch))<br>
> + return 1;<br>
<br>
Isn't using strstr() completely broken here?<br>
<br>
Couple of the architecture names are prefixes of the 64bit variant, also<br>
validating the architecture by strstr() is kind of pointless, it will<br>
match any garbage that contains one of the substrings.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Yes, that's true.</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
If nothing else we should strdup() the string and then loop over strtok().<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Good suggestion!</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + return 0;<br>
> +}<br>
> -- <br>
> 2.17.0<br>
> <br>
> <br>
> -- <br>
> Mailing list info: <a href="https://lists.linux.it/listinfo/ltp" rel="noreferrer" target="_blank">https://lists.linux.it/listinfo/ltp</a><br>
<br>
-- <br>
Cyril Hrubis<br>
<a href="mailto:chrubis@suse.cz" target="_blank">chrubis@suse.cz</a><br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Regards,<br></div><div>Li Wang<br></div></div></div></div>