validation: add odp_system test

Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
---
 test/validation/.gitignore   |  1 +
 test/validation/Makefile.am  |  4 ++-
 test/validation/odp_system.c | 73 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 1 deletion(-)
 create mode 100644 test/validation/odp_system.c

On 5 December 2014 at 13:10, Stuart Haslam <stuart.haslam@arm.com> wrote:

> On Thu, Dec 04, 2014 at 05:50:59PM +0000, Mike Holmes wrote:
> > Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
> > ---
> >  test/validation/.gitignore   |  1 +
> >  test/validation/Makefile.am  |  4 ++-
> >  test/validation/odp_system.c | 73
> ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 77 insertions(+), 1 deletion(-)
> >  create mode 100644 test/validation/odp_system.c
> >
>
> I do wonder whether some of these APIs should exist at all. but anyway
>

The push is to cover the current repo with tests The 1.0 Delta
<https://docs.google.com/a/linaro.org/document/d/1BRVyW8IIVMTq4nhB_vUz5y-te6TEdu5g1XgolujjY6c/edit>
doc has a note in there now as a question for Petri on keeping these public.


> I've made some comments below.
>
> > diff --git a/test/validation/.gitignore b/test/validation/.gitignore
> > index 37e2594..e3b86f6 100644
> > --- a/test/validation/.gitignore
> > +++ b/test/validation/.gitignore
> > @@ -4,3 +4,4 @@ odp_init
> >  odp_queue
> >  odp_crypto
> >  odp_shm
> > +odp_system
> > diff --git a/test/validation/Makefile.am b/test/validation/Makefile.am
> > index 8547085..f85166a 100644
> > --- a/test/validation/Makefile.am
> > +++ b/test/validation/Makefile.am
> > @@ -6,13 +6,14 @@ AM_LDFLAGS += -static
> >  if ODP_CUNIT_ENABLED
> >  TESTS = ${bin_PROGRAMS}
> >  check_PROGRAMS = ${bin_PROGRAMS}
> > -bin_PROGRAMS = odp_init odp_queue odp_crypto odp_shm
> > +bin_PROGRAMS = odp_init odp_queue odp_crypto odp_shm odp_system
> >  odp_init_LDFLAGS = $(AM_LDFLAGS)
> >  odp_queue_LDFLAGS = $(AM_LDFLAGS)
> >  odp_crypto_CFLAGS = $(AM_CFLAGS) -I$(srcdir)/crypto
> >  odp_crypto_LDFLAGS = $(AM_LDFLAGS)
> >  odp_shm_CFLAGS = $(AM_CFLAGS)
> >  odp_shm_LDFLAGS = $(AM_LDFLAGS)
> > +odp_system_LDFLAGS = $(AM_LDFLAGS)
> >  endif
> >
> >  dist_odp_init_SOURCES = odp_init.c
> > @@ -22,3 +23,4 @@ dist_odp_crypto_SOURCES =
> crypto/odp_crypto_test_async_inp.c \
> >                         crypto/odp_crypto_test_rng.c \
> >                         odp_crypto.c common/odp_cunit_common.c
> >  dist_odp_shm_SOURCES = odp_shm.c common/odp_cunit_common.c
> > +dist_odp_system_SOURCES = odp_system.c common/odp_cunit_common.c
> > diff --git a/test/validation/odp_system.c b/test/validation/odp_system.c
> > new file mode 100644
> > index 0000000..193b8f3
> > --- /dev/null
> > +++ b/test/validation/odp_system.c
> > @@ -0,0 +1,73 @@
> > +/* Copyright (c) 2014, Linaro Limited
> > + * All rights reserved.
> > + *
> > + * SPDX-License-Identifier:     BSD-3-Clause
> > + */
> > +
> > +#include "odp.h"
> > +#include "odp_cunit_common.h"
> > +
> > +static void test_odp_sys_core_count(void)
> > +{
> > +     int cores;
> > +
> > +     cores = odp_sys_core_count();
> > +     CU_ASSERT(0 < cores)
>
> missing ; here and on all other asserts. I guess it works like this but
> my brain thinks it's a syntax error, besides we should be consistent and
> it seems other tests add the ;
>

Will fix

>
> > +}
> > +
> > +static void test_odp_sys_cache_line_size(void)
> > +{
> > +     uint64_t cache_size;
>
> API returns an int so a -ve return value would overflow and pass the
> assert below.


Will fix


>


> > +
> > +     cache_size = odp_sys_cache_line_size();
> > +     CU_ASSERT(0 < cache_size)
> > +}
> > +
> > +static void test_odp_sys_cpu_model_str(void)
> > +{
> > +     char model[128];
> > +
> > +     strcpy(model, odp_sys_cpu_model_str());
>
> This may overrun as you've no idea how long the string is, and is it OK
> for the function to return NULL or an empty string?
>
>
The API does not specify, but linux-generic allocates 128 chars and assigns
0 to the last byte.
If we are ok with it I will test for that specification and create a patch
to update the docs.


> > +     CU_ASSERT(strlen(model) > 0)
> > +     CU_ASSERT(strlen(model) < 127)
> > +}
> > +
> > +static void test_odp_sys_page_size(void)
> > +{
> > +     uint64_t page;
> > +
> > +     page = odp_sys_page_size();
> > +     CU_ASSERT(0 < page)
> > +}
> > +
> > +static void test_odp_sys_huge_page_size(void)
> > +{
> > +     uint64_t page;
> > +
> > +     page = odp_sys_huge_page_size();
> > +     CU_ASSERT(0 < page)
>
> Difficult to know what should be tested here as the API isn't well
> defined, perhaps 0 would mean there are no hugepages which I think
> should be valid.
>

I can update the docs with the clarification


>
> I suppose you could also check that it's either 0 or >= odp_sys_page_size()
>

Do we need to do that, don't we trust the implementation on the actual
value as long as it is
a legal number?
i.e. Are we saying that on all systems you must have larger or equal
hugepages ? makes sense but should we enforce it ?


>
> (although surely the page size APIs should be made internal.)
>

It is in the delta doc as a suggestion for completion by 1.0


>
> > +}
> > +
> > +static void test_odp_sys_cpu_hz(void)
> > +{
> > +     uint64_t hz;
> > +
> > +     hz = odp_sys_cpu_hz();
> > +     CU_ASSERT(0 < hz)
> > +}
> > +
> > +CU_TestInfo test_odp_system[] = {
> > +     {"odp_sys_core_count",  test_odp_sys_core_count},
> > +     {"odp_sys_cache_line_size",  test_odp_sys_cache_line_size},
> > +     {"odp_sys_cpu_model_str",  test_odp_sys_cpu_model_str},
> > +     {"odp_sys_page_size",  test_odp_sys_page_size},
> > +     {"odp_sys_huge_page_size",  test_odp_sys_huge_page_size},
> > +     {"odp_sys_cpu_hz",  test_odp_sys_cpu_hz},
> > +     CU_TEST_INFO_NULL,
> > +};
> > +
> > +CU_SuiteInfo odp_testsuites[] = {
> > +             {"System Info", NULL, NULL, NULL, NULL,
> > +              test_odp_system},
> > +              CU_SUITE_INFO_NULL,
> > +};
> > --
> > 2.1.0
> >
>
> --
> Stuart.
>
>