Review feedback should stay on the list. :)
On Sat, Sep 12, 2020 at 4:29 PM Eshan Dhawan <eshandhawa...@gmail.com>
wrote:
>
>
>
> On Sat, Sep 12, 2020 at 2:45 AM Joel Sherrill <j...@rtems.org> wrote:
>
>> This looks close to me but I see some minor issues. I have tried to
>> annotate them.
>>
>> The spacing ones could be spaces vs tabs. Not sure.
>>
>> On Thu, Sep 10, 2020 at 3:48 PM Eshan dhawan <eshandhawa...@gmail.com>
>> wrote:
>>
>>> Adds Confstr file To RTEMS with test
>>>
>>> Signed-off-by: Eshan dhawan <eshandhawa...@gmail.com>
>>> ---
>>> cpukit/Makefile.am | 1 +
>>> cpukit/posix/src/confstr.c | 105 +++++++++++++++
>>> testsuites/psxtests/Makefile.am | 9 ++
>>> testsuites/psxtests/configure.ac | 1 +
>>> testsuites/psxtests/psxconfstr/init.c | 125 ++++++++++++++++++
>>> testsuites/psxtests/psxconfstr/psxconfstr.doc | 17 +++
>>> testsuites/psxtests/psxconfstr/psxconfstr.scn | 4 +
>>> 7 files changed, 262 insertions(+)
>>> create mode 100644 cpukit/posix/src/confstr.c
>>> create mode 100644 testsuites/psxtests/psxconfstr/init.c
>>> create mode 100644 testsuites/psxtests/psxconfstr/psxconfstr.doc
>>> create mode 100644 testsuites/psxtests/psxconfstr/psxconfstr.scn
>>>
>>> diff --git a/cpukit/Makefile.am b/cpukit/Makefile.am
>>> index e5009e53c9..1c9e4697f1 100644
>>> --- a/cpukit/Makefile.am
>>> +++ b/cpukit/Makefile.am
>>> @@ -509,6 +509,7 @@ librtemscpu_a_SOURCES += posix/src/condsignalsupp.c
>>> librtemscpu_a_SOURCES += posix/src/condtimedwait.c
>>> librtemscpu_a_SOURCES += posix/src/condwait.c
>>> librtemscpu_a_SOURCES += posix/src/condwaitsupp.c
>>> +librtemscpu_a_SOURCES += posix/src/confstr.c
>>> librtemscpu_a_SOURCES += posix/src/_execve.c
>>> librtemscpu_a_SOURCES += posix/src/fork.c
>>> librtemscpu_a_SOURCES += posix/src/key.c
>>> diff --git a/cpukit/posix/src/confstr.c b/cpukit/posix/src/confstr.c
>>> new file mode 100644
>>> index 0000000000..71041db462
>>> --- /dev/null
>>> +++ b/cpukit/posix/src/confstr.c
>>> @@ -0,0 +1,105 @@
>>> +/*-
>>> + * SPDX-License-Identifier: BSD-3-Clause
>>> + *
>>> + * Copyright (c) 1993
>>> + * The Regents of the University of California. All rights
>>> reserved.
>>> + *
>>> + * 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.
>>> + * 3. Neither the name of the University nor the names of its
>>> contributors
>>> + * may be used to endorse or promote products derived from this
>>> software
>>> + * without specific prior written permission.
>>> + *
>>> + * THIS SOFTWARE IS PROVIDED BY THE REGENTS 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 REGENTS 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.
>>> + */
>>> +
>>> +#include <sys/cdefs.h>
>>> +__SCCSID("@(#)confstr.c 8.1 (Berkeley) 6/4/93");
>>> +__FBSDID("$FreeBSD$");
>>>
>>
>> Probably worth adding a comment above that the FreeBSD version
>> was reduced to what made sense on RTEMS.
>>
> OK
>
>>
>> +
>>> +#include <sys/param.h>
>>> +
>>> +#include <errno.h>
>>> +#include <limits.h>
>>> +#include <paths.h>
>>> +#include <string.h>
>>> +#include <unistd.h>
>>>
>>> If you do that, you won't need all these headers.
>>
> I left the headers there if someone adds an implementation or extends it.
> If it made sense
>
That makes sense but remove them anyway. One day we may be able to
automatically check if something is really needed.
>
>>
>>> +size_t
>>> +confstr(int name, char *buf, size_t len)
>>> +{
>>> + const char *p;
>>> + const char UPE[] = "unsupported programming environment";
>>> +
>>> + switch (name) {
>>> +#ifndef __rtems__
>>> + case _CS_PATH:
>>> + p = _PATH_STDPATH;
>>> + goto docopy;
>>> +#endif
>>>
>>
>> It looks like the body of the other cases were gutted from the freebsd
>> code. Why keep an ifndef RTEMS here? This is just going to produce
>> an error for an unknown name via the default case. If we are going to
>> delete all non-FreeBSD code from this and make it a more RTEMS
>> specific implementation, you might as well just do this:
>>
>> case _CS_PATH:
>> errno = EINVAL;
>> return (0);
>>
>> That keeps the order like the FreeBSD without ifdefs.
>>
> OK
>
>>
>>
>>> + /*
>>> + * POSIX/SUS ``Programming Environments'' stuff
>>> + *
>>> + * We don't support more than one programming environment
>>> + * on any platform (yet), so we just return the empty
>>> + * string for the environment we are compiled for,
>>> + * and the string "unsupported programming environment"
>>> + * for anything else. (The Standard says that if these
>>> + * values are used on a system which does not support
>>> + * this environment -- determined via sysconf() -- then
>>> + * the value we return is unspecified. So, we return
>>> + * something which will cause obvious breakage.)
>>> + */
>>> + case _CS_POSIX_V6_ILP32_OFF32_CFLAGS:
>>> + case _CS_POSIX_V6_ILP32_OFF32_LDFLAGS:
>>> + case _CS_POSIX_V6_ILP32_OFF32_LIBS:
>>> + case _CS_POSIX_V6_LPBIG_OFFBIG_CFLAGS:
>>> + case _CS_POSIX_V6_LPBIG_OFFBIG_LDFLAGS:
>>> + case _CS_POSIX_V6_LPBIG_OFFBIG_LIBS:
>>> +
>>> + p = UPE;
>>> + goto docopy;
>>> +
>>> + case _CS_POSIX_V6_ILP32_OFFBIG_CFLAGS:
>>> + case _CS_POSIX_V6_ILP32_OFFBIG_LDFLAGS:
>>> + case _CS_POSIX_V6_ILP32_OFFBIG_LIBS:
>>> +
>>> + p = UPE;
>>> + goto docopy;
>>>
>>
>>
>> Spacing.
>>
> The spacing is coming from freebsd . I will update that :)
>
>> +
>>> + case _CS_POSIX_V6_LP64_OFF64_CFLAGS:
>>> + case _CS_POSIX_V6_LP64_OFF64_LDFLAGS:
>>> + case _CS_POSIX_V6_LP64_OFF64_LIBS:
>>> +
>>> + p = UPE;
>>> + goto docopy;
>>>
>>
>> Spacing.
>>
>>> +
>>> +
>>> +docopy:
>>> + if (len != 0 && buf != NULL)
>>> + strlcpy(buf, p, len);
>>> + return (strlen(p) + 1);
>>> +
>>> + default:
>>> + errno = EINVAL;
>>> + return (0);
>>> + }
>>> + /* NOTREACHED */
>>> +}
>>> diff --git a/testsuites/psxtests/Makefile.am
>>> b/testsuites/psxtests/Makefile.am
>>> index aae5fe764a..f0910330a9 100755
>>> --- a/testsuites/psxtests/Makefile.am
>>> +++ b/testsuites/psxtests/Makefile.am
>>> @@ -358,6 +358,15 @@ psxconfig01_CPPFLAGS = $(AM_CPPFLAGS)
>>> $(TEST_FLAGS_psxconfig01) \
>>> $(support_includes) -I$(top_srcdir)/include
>>> endif
>>>
>>> +if TEST_psxgetcpuclockid01
>>> +psx_tests += psxconfstr
>>> +psx_screens += psxconfstr/psxconfstr.scn
>>> +psx_docs += psxconfstr/psxconfstr.doc
>>> +psxconfstr_SOURCES = psxconfstr/init.c
>>> +psxconfstr_CPPFLAGS = $(AM_CPPFLAGS) $(TEST_FLAGS_psxgetcpuclockid01) \
>>> + $(support_includes)
>>> +endif
>>> +
>>> if TEST_psxdevctl01
>>> psx_tests += psxdevctl01
>>> psx_screens += psxdevctl01/psxdevctl01.scn
>>> diff --git a/testsuites/psxtests/configure.ac b/testsuites/psxtests/
>>> configure.ac
>>> index c509086abc..96c937cb4d 100644
>>> --- a/testsuites/psxtests/configure.ac
>>> +++ b/testsuites/psxtests/configure.ac
>>> @@ -80,6 +80,7 @@ RTEMS_TEST_CHECK([psxconcurrency01])
>>> RTEMS_TEST_CHECK([psxcond01])
>>> RTEMS_TEST_CHECK([psxcond02])
>>> RTEMS_TEST_CHECK([psxconfig01])
>>> +RTEMS_TEST_CHECK([psxconfstr])
>>> RTEMS_TEST_CHECK([psxdevctl01])
>>> RTEMS_TEST_CHECK([psxeintr_join])
>>> RTEMS_TEST_CHECK([psxenosys])
>>> diff --git a/testsuites/psxtests/psxconfstr/init.c
>>> b/testsuites/psxtests/psxconfstr/init.c
>>> new file mode 100644
>>> index 0000000000..7219470559
>>> --- /dev/null
>>> +++ b/testsuites/psxtests/psxconfstr/init.c
>>> @@ -0,0 +1,125 @@
>>> +/*
>>> + * @file
>>> + * @brief Test suite for confstr method
>>> + */
>>> +
>>> +/*
>>> + * SPDX-License-Identifier: BSD-2-Clause
>>> + *
>>> + * Copyright (C) 2020 Eshan Dhawan, Sebastian Huber
>>> + *
>>> + * 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.
>>> + */
>>> +
>>> +#ifdef HAVE_CONFIG_H
>>> +#include "config.h"
>>> +#endif
>>> +
>>> +#include <errno.h>
>>> +#include <stdio.h>
>>> +#include <string.h>
>>> +#include <rtems.h>
>>> +#include <rtems/test.h>
>>> +#include <unistd.h>
>>> +#include <rtems/bspIo.h>
>>> +
>>> +#include "tmacros.h"
>>> +#include "test_support.h"
>>> +
>>> +const char rtems_test_name[] = "PSXCONFSTR";
>>> +
>>> +/* Forward declaration to avoid warnings */
>>> +rtems_task Init( rtems_task_argument ignored );
>>> +
>>> +static char buffer[512];
>>> +
>>> +static const T_action actions[] = {
>>> + T_report_hash_sha256,
>>> + T_check_task_context,
>>> + T_check_file_descriptors,
>>> + T_check_rtems_barriers,
>>> + T_check_rtems_extensions,
>>> + T_check_rtems_message_queues,
>>> + T_check_rtems_partitions,
>>> + T_check_rtems_periods,
>>> + T_check_rtems_regions,
>>> + T_check_rtems_semaphores,
>>> + T_check_rtems_tasks,
>>> + T_check_rtems_timers,
>>> + T_check_posix_keys
>>> +};
>>>
>>> Is this a remnant of a copied test? Is it used?
>>
> I don't actually know its effect its the part of the generic runner
> provided by sebastian for the new test framework.
>
>>
>>
>>> +static const T_config config = {
>>> + .name = "psxconfstr",
>>> + .buf = buffer,
>>> + .putchar = rtems_put_char,
>>> + .buf_size = sizeof(buffer),
>>> + .verbosity = T_VERBOSE,
>>> + .now = T_now_clock,
>>> + .action_count = T_ARRAY_SIZE(actions),
>>> + .actions = actions
>>> +};
>>> +
>>> +/* init test function begins */
>>> +T_TEST_CASE(confstr)
>>> +{
>>> +
>>> +int r;
>>> + char * buf ;
>>> + const char UPE[] = "unsupported programming environment";
>>> + size_t len1;
>>> + len1 = strlen(UPE) + 1;
>>> + r = confstr(_CS_PATH, buf, sizeof(buf));
>>> +
>>> + T_quiet_psx_success(r);
>>> + r = confstr(_CS_POSIX_V6_ILP32_OFFBIG_CFLAGS, buf, sizeof(buf));
>>> + T_quiet_eq_int(r, len1);
>>> +
>>> +}
>>>
>>> You should exercise every known good value for a name. Plus a randomly
>> bad one.
>>
> OK, I will update this
>
>>
>>
>>> +rtems_task Init(rtems_task_argument ignored)
>>> +{
>>> + int exit_code;
>>> +
>>> + TEST_BEGIN();
>>> +
>>> + T_register();
>>> + exit_code = T_main(&config);
>>> + if (exit_code == 0) {
>>> + TEST_END();
>>> + }
>>> +
>>> + rtems_test_exit(exit_code);
>>> +
>>> +}
>>> +#define CONFIGURE_APPLICATION_DOES_NOT_NEED_CLOCK_DRIVER
>>> +#define CONFIGURE_APPLICATION_NEEDS_CONSOLE_DRIVER
>>> +
>>> +#define CONFIGURE_MAXIMUM_TASKS 1
>>> +
>>> +#define CONFIGURE_INITIAL_EXTENSIONS RTEMS_TEST_INITIAL_EXTENSION
>>> +
>>> +#define CONFIGURE_RTEMS_INIT_TASKS_TABLE
>>> +#define CONFIGURE_INIT
>>> +
>>> +#include <rtems/confdefs.h>
>>> +/* end of file */
>>> diff --git a/testsuites/psxtests/psxconfstr/psxconfstr.doc
>>> b/testsuites/psxtests/psxconfstr/psxconfstr.doc
>>> new file mode 100644
>>> index 0000000000..e353f74ae7
>>> --- /dev/null
>>> +++ b/testsuites/psxtests/psxconfstr/psxconfstr.doc
>>> @@ -0,0 +1,17 @@
>>> +# 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: psxconfstr
>>> +
>>> +Directives:
>>> + Pthread_getcpuclockid()
>>>
>>
>> Not right.
>>
> this is a remenant from other test case :)
>
I expected that. Just review and double check. I might have missed others.
>
>>
>>> +
>>> +Concepts:
>>> +
>>> ++ This test exercises the confstr method
>>> +
>>> diff --git a/testsuites/psxtests/psxconfstr/psxconfstr.scn
>>> b/testsuites/psxtests/psxconfstr/psxconfstr.scn
>>> new file mode 100644
>>> index 0000000000..705a08d51b
>>> --- /dev/null
>>> +++ b/testsuites/psxtests/psxconfstr/psxconfstr.scn
>>> @@ -0,0 +1,4 @@
>>> +*** BEGIN OF TEST PSXGETCPUCLOCKID 1 ***
>>> +
>>> +*** END OF TEST PSXGETCPUCLOCKID 1 ***
>>> +
>>>
>>
>> I'm betting this isn't right either. :)
>>
> This is the remnant of the other test case :)
> will update it in the next version :)
>
>>
>> --joel
>>
>>
>>> --
>>> 2.17.1
>>>
>>>
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
