On Fri, Apr 17, 2020 at 11:38 PM Gedare Bloom <ged...@rtems.org> wrote:
> On Fri, Apr 17, 2020 at 11:48 AM Eshan Dhawan <eshandhawa...@gmail.com>
> wrote:
> >
> >
> >
> > On Fri, Apr 17, 2020 at 9:49 PM Gedare Bloom <ged...@rtems.org> wrote:
> >>
> >> On Fri, Apr 17, 2020 at 8:31 AM Eshan dhawan <eshandhawa...@gmail.com>
> wrote:
> >> >
> >> > ---
> >> > testsuites/psxtests/Makefile.am | 10 ++
> >> > testsuites/psxtests/configure.ac | 1 +
> >> > testsuites/psxtests/psxgetcpuclockid01/init.c | 149
> ++++++++++++++++++
> >> > .../psxgetcpuclockid01/psxgetcpuclockid01.doc | 23 +++
> >> > .../psxgetcpuclockid01/psxgetcpuclockid01.scn | 4 +
> >> > 5 files changed, 187 insertions(+)
> >> > create mode 100644 testsuites/psxtests/psxgetcpuclockid01/init.c
> >> > create mode 100644
> testsuites/psxtests/psxgetcpuclockid01/psxgetcpuclockid01.doc
> >> > create mode 100644
> testsuites/psxtests/psxgetcpuclockid01/psxgetcpuclockid01.scn
> >> >
> >> > diff --git a/testsuites/psxtests/Makefile.am
> b/testsuites/psxtests/Makefile.am
> >> > index 1f9e4233ec..ffadff16e7 100755
> >> > --- a/testsuites/psxtests/Makefile.am
> >> > +++ b/testsuites/psxtests/Makefile.am
> >> > @@ -462,6 +462,16 @@ psxgetattrnp01_CPPFLAGS = $(AM_CPPFLAGS)
> $(TEST_FLAGS_psxgetattrnp01) \
> >> > $(support_includes) -I$(top_srcdir)/include
> >> > endif
> >> >
> >> > +if TEST_psxgetcpuclockid01
> >> > +psx_tests += psxgetcpuclockid01
> >> > +psx_screens += psxgetcpuclockid01/psxgetcpuclockid01.scn
> >> > +psx_docs += psxgetcpuclockid01/psxgetcpuclockid01.doc
> >> > +psxgetcpuclockid01_SOURCES = psxgetcpuclockid01/init.c
> >> > +psxgetcpuclockid01_CPPFLAGS = $(AM_CPPFLAGS)
> $(TEST_FLAGS_psxgetcpuclockid01) \
> >> > + $(support_includes)
> >> > +endif
> >> > +
> >> > +
> >> extra blank
> >>
> >> > if TEST_psxgetrusage01
> >> > psx_tests += psxgetrusage01
> >> > psx_screens += psxgetrusage01/psxgetrusage01.scn
> >> > diff --git a/testsuites/psxtests/configure.ac b/testsuites/psxtests/
> configure.ac
> >> > index 139787cccb..c509086abc 100644
> >> > --- a/testsuites/psxtests/configure.ac
> >> > +++ b/testsuites/psxtests/configure.ac
> >> > @@ -91,6 +91,7 @@ RTEMS_TEST_CHECK([psxfile01])
> >> > RTEMS_TEST_CHECK([psxfile02])
> >> > RTEMS_TEST_CHECK([psxfilelock01])
> >> > RTEMS_TEST_CHECK([psxgetattrnp01])
> >> > +RTEMS_TEST_CHECK([psxgetcpuclockid01])
> >> > RTEMS_TEST_CHECK([psxgetrusage01])
> >> > RTEMS_TEST_CHECK([psxglobalcon01])
> >> > RTEMS_TEST_CHECK([psxglobalcon02])
> >> > diff --git a/testsuites/psxtests/psxgetcpuclockid01/init.c
> b/testsuites/psxtests/psxgetcpuclockid01/init.c
> >> > new file mode 100644
> >> > index 0000000000..a2a6476d96
> >> > --- /dev/null
> >> > +++ b/testsuites/psxtests/psxgetcpuclockid01/init.c
> >> > @@ -0,0 +1,149 @@
> >> > +/*
> >> > + * @file
> >> > + * @brief Test suite for getcpuclockid methods
> >> > + */
> >> > +
> >> > +/*
> >> > + * SPDX-License-Identifier: BSD-2-Clause
> >> > + *
> >> > + * Copyright (C) 2020 Eshan Dhawan, Vaibhav Gupta
> >> > + *
> >> > + * Redistribution and use in source and binary forms, with or without
> >> > + * modification, are permitted provided that the following conditions
> >> > + * are met:
> >> > + * 1. Redistributions of source code must retain the above copyright
> >> > + * notice, this list of conditions and the following disclaimer.
> >> > + * 2. Redistributions in binary form must reproduce the above
> copyright
> >> > + * notice, this list of conditions and the following disclaimer
> in the
> >> > + * documentation and/or other materials provided with the
> distribution.
> >> > + *
> >> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> CONTRIBUTORS "AS IS"
> >> > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> TO, THE
> >> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> PARTICULAR PURPOSE
> >> > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
> CONTRIBUTORS BE
> >> > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY,
> OR
> >> > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT
> OF
> >> > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> BUSINESS
> >> > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
> WHETHER IN
> >> > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
> OTHERWISE)
> >> > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
> ADVISED OF THE
> >> > + * POSSIBILITY OF SUCH DAMAGE.
> >> > + */
> >> > +
> >> > +
> >> extra blank
> >> > +#ifdef HAVE_CONFIG_H
> >> > +#include "config.h"
> >> > +#endif
> >> > +
> >> > +#include <errno.h>
> >> > +#include <pthread.h>
> >> > +#include <stdint.h>
> >> > +#include <stdio.h>
> >> > +#include <rtems/test.h>
> >> > +#include <time.h>
> >> > +#include <unistd.h>
> >> > +
> >> > +#include "tmacros.h"
> >> > +#include "test_support.h"
> >> > +
> >> > +const char rtems_test_name[] = "PSXGETCPUCLOCKID 1";
> >> > +
> >> > +/* Forward declaration to avoid warnings */
> >> > +void *POSIX_Init (void * argument);
> >> > +void *test_pthread(void *);
> >> > +/*structure timespec */
> >> > +
> >> > +/* test_pthread function begins */
> >> > +void *test_pthread(void *arg)
> >> The fn name should reflect the job it does. This isn't testing pthread.
> >> test_pthread_getcpuclockid()?
> >>
> >>
> >> > +{
> >> > + int r;
> >> > + clockid_t cid;
> >> > + struct timespec start_cpu, start_wall, finish;
> >> > + long time_passed_wall, time_passed_cpu ;
> >> > +
> >> > + r = pthread_getcpuclockid( pthread_self(), &cid );
> >> > + if ( r ) {
> >> > + printf( "pthread_getcpuclockid : %d", r );
> >> > + }
> >> > + rtems_test_assert( r == 0 );
> >> > +
> >> > + r = clock_gettime( cid, &start_cpu );
> >> > + if ( r ) {
> >> > + printf( "clock_gettime : %d \n", r );
> >> Maybe use %s and strerror(r)? There are other examples in the psxtests
> of this.
> >>
> >> > + if ( r == EINVAL || r == ESRCH ) {
> >> > + printf( "the clockid returned is invalid" );
> >> > + }
> >> I don't think this is necessary especially if you print the strerror()
> version
> >>
> >> > + }
> >> > + rtems_test_assert( r == 0 );
> >> > +
> >> > + r = clock_gettime( CLOCK_REALTIME, &start_wall );
> >> > + if ( r ) {
> >> > + printf( "clock_gettime : %d \n", r );
> >> > + if ( r == EINVAL || r == ESRCH ) {
> >> > + printf( "the clockid returned is invalid" );
> >> > + }
> >> ditto
> >>
> >> > + }
> >> > + rtems_test_assert( r == 0 );
> >> > + while (1) {
> >> > + clock_gettime( CLOCK_REALTIME, &finish );
> >> > + time_passed_wall = finish.tv_nsec - start_wall.tv_nsec ;
> >> > + if(time_passed >= 500000000){
> >> > + break;
> >> Why break a while(1) loop on a condition?
> >>
> >> What is this testing?
> >>
> >> We would like tests to finish quickly whenever possible. How long
> >> would this test expect to take, and is that necessary?
> >>
> > Here I tried to test the clock_id provided by the
> pthread_getcpuclockid() function
> > this test was suggested by Dr Joel in the patch V1 so I just improved it
> in this version
> > An I wasn't able to fully understand in the previous patch.
> > you can read more details for the test in this mail
> > https://lists.rtems.org/pipermail/devel/2020-April/059292.html
>
> If you don't understand a request from someone, please ask them for
> more details. It is a waste of everyone's time if you go write code
> without understanding the requirements.
>
> I also see that Joel complained that this test won't run. You need to
> add such comments in your commit message or email introduction if a
> patch is not expected to work yet, or if you haven't tested it
> yourself. In general, sending us patches to review that you haven't
> tested is also wasting our time. We don't like to waste our time.
>
I had asked for elaboration but got no reply
https://lists.rtems.org/pipermail/devel/2020-April/059299.html
Also, I tested the code and it worked fine I had told that
before adding the time part.
https://lists.rtems.org/pipermail/devel/2020-April/059294.html
> >>
> >> > + }
> >> > + }
> >> > + printf( "wall_time = %li\n", time_passed_wall );
> >> > + clock_gettime( cid, &finish );
> >> > + time_passed_cpu = finish.tv_nsec - start_cpu.tv_nsec ;
> >> > + printf( "CPU_time = %li\n", time_passed );
> >> > + pthread_exit(0);
> >> > + return NULL;
> >> dead code and not strictly correct.
> >>
> This "return NULL;" will never be executed. pthread_exit() does not return.
>
> NULL has a type (void *). This function is declared void, it should
> return nothing.
>
> Thus, an explicit return statement should just be:
> return;
> while the fall-out of the end of the function can be simply the
> closing french brace.
>
> this was there in other test suites as well
> >> > +}
> >> > +
> >> > +/* init test function begins */
> >> > +void *POSIX_Init (void * argument)
> >> > +{
> >> > + TEST_BEGIN();
> >> > +
> >> > + int r;
> >> > + clockid_t cid;
> >> > + pthread_t thread_id;
> >> > +
> >> > + r= pthread_create( &thread_id, NULL, test_pthread, NULL );
> >> Why using another pthread?
> >
> > So that when I join that pthread and the thread_id becomes invalid and
> could be used for
> > testing the ESRCH exception
>
> I still don't understand why it is necessary to do this test in a
> separate pthread.
>
Testing on the other pthread isn't important
just another pthread is required
> >>
> >> > + if( r )
> >> > + {
> Missed this before, put the { on same line as if ( r ) and check white
> space after if.
>
> >> > + printf( "pthread_create : %d", r );
> >> > + }
> >> > + rtems_test_assert( r == 0 );
> >> > + /*
> >> > + r = pthread_join( thread_id, NULL );
> >> > + if( r )
> >> > + {
> >> > + printf( "pthread_join : %d", r );
> >> > + }
> >> > + rtems_test_assert( r == 0 );
> >> > +
> >> > + r = pthread_getcpuclockid( thread_id, &cid );
> >> > + if( r != ESRCH )
> >> > + {
> >> > + printf( "pthread_getcpuclockid : %d", r );
> >> > + }
> >> > + rtems_test_assert( r == ESRCH );
> >> > +*/
> Missed this before also, do not comment-out code. Code is either
> necessary or unnecessary.
>
This is a necessary part of the code.
I left the comments there by mistake.
I apologize for that.
>
> >> > + TEST_END();
> >> > + rtems_test_exit(0);
> >> > +}
> >> > +
> >> > +#define CONFIGURE_APPLICATION_NEEDS_CLOCK_DRIVER
> >> > +#define CONFIGURE_APPLICATION_NEEDS_SIMPLE_CONSOLE_DRIVER
> >> > +
> >> > +#define CONFIGURE_MAXIMUM_POSIX_THREADS 2
> >> > +#define CONFIGURE_MAXIMUM_TASKS 1
> >> > +
> >> > +#define CONFIGURE_INITIAL_EXTENSIONS RTEMS_TEST_INITIAL_EXTENSION
> >> > +
> >> > +#define CONFIGURE_POSIX_INIT_THREAD_TABLE
> >> > +
> >> > +#define CONFIGURE_INIT
> >> > +
> >> > +#include <rtems/confdefs.h>
> >> > +
> >> > diff --git
> a/testsuites/psxtests/psxgetcpuclockid01/psxgetcpuclockid01.doc
> b/testsuites/psxtests/psxgetcpuclockid01/psxgetcpuclockid01.doc
> >> > new file mode 100644
> >> > index 0000000000..ef148182d5
> >> > --- /dev/null
> >> > +++ b/testsuites/psxtests/psxgetcpuclockid01/psxgetcpuclockid01.doc
> >> > @@ -0,0 +1,23 @@
> >> > +# COPYRIGHT (c) 2020
> >> > +# On-Line Applications Research Corporation (OAR).
> >> > +#
> >> > +# SPDX-License-Identifier: BSD-2-Clause
> >> > +#
> >> > +
> >> > +This file describes the directives and concepts tested by this test
> set.
> >> > +
> >> > +test set name: psxgetcpuclockid 1
> >> > +
> >> > +Directives:
> >> > + Pthread_getcpuclockid()
> >> > +
> >> > +Concepts:
> >> > +
> >> > ++ This test exercises the getcpuclockid methods.
> >> > +
> >> > ++ For pthread_getcpuclockid it created a pthread using
> pthread_create and
> >> > + then finds the clockid_t of that thread then used gettime() add a
> pause of .5 seconds
> >> > + and then ends the thread.
> >> > + After the thread is ended it again tried to find the clockid_t of
> the thread to
> >> > + check for ESRCH exception.
> >> > +
> >> > diff --git
> a/testsuites/psxtests/psxgetcpuclockid01/psxgetcpuclockid01.scn
> b/testsuites/psxtests/psxgetcpuclockid01/psxgetcpuclockid01.scn
> >> > new file mode 100644
> >> > index 0000000000..937a908fd5
> >> > --- /dev/null
> >> > +++ b/testsuites/psxtests/psxgetcpuclockid01/psxgetcpuclockid01.scn
> >> > @@ -0,0 +1,4 @@
> >> > +*** BEGIN OF TEST PSXGETCLOCKID 1 ***
> >> > +
> >> > +*** END OF TEST PSXGETCPUCLOCKID 1 ***
> >> > +
> >> > --
> >> > 2.17.1
> >> >
>
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
