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