[PATCH 1/1] newlib01: Check exit processing for file objects

[Sebastian Huber]

On 23/03/2022 12:36, Matthew Joyce wrote:
> From: Matt Joyce <matthew.joyce at embedded-brains.de>
> 
> ---
>   testsuites/libtests/newlib01/init.c | 131 ++++++++++++++++++++++++++--
>   1 file changed, 124 insertions(+), 7 deletions(-)
> 
> diff --git a/testsuites/libtests/newlib01/init.c b/testsuites/libtests/newlib01/init.c
> index 5864047a80..58757a7676 100644
> --- a/testsuites/libtests/newlib01/init.c
> +++ b/testsuites/libtests/newlib01/init.c
> @@ -1,5 +1,5 @@
>   /*
> - * Copyright (c) 2014 embedded brains GmbH.  All rights reserved.
> + * Copyright (c) 2014, 2022 embedded brains GmbH.  All rights reserved.
>    *
>    * The license and distribution terms for this file may be
>    * found in the file LICENSE in this distribution or at
> @@ -11,13 +11,16 @@
>   #endif
>   
>   #include <stdio.h>
> +#include <errno.h>
>   
>   #include <sys/reent.h>
> +#include <sys/stat.h>
>   
>   #include <rtems.h>
>   #include <rtems/console.h>
>   #include <rtems/imfs.h>
>   #include <rtems/libcsupport.h>
> +#include <rtems/sysinit.h>
>   
>   #include "tmacros.h"
>   
> @@ -25,6 +28,15 @@ const char rtems_test_name[] = "NEWLIB 1";
>   
>   static const char file_path[] = "/file";
>   
> +static void test_sysinit_handler1(void);
> +
> +/*
> + * Test Newlib exit procedures. Allows us to place an exit handler at position
> + * 0, which is called after rtems_stdio_exit by Newlib's __call_exitprocs().
> + */
> +RTEMS_SYSINIT_ITEM( test_sysinit_handler1, RTEMS_SYSINIT_STD_FILE_DESCRIPTORS,
> +   RTEMS_SYSINIT_ORDER_FIRST );
> +

Please move the RTEMS_SYSINIT_ITEM() below the test_sysinit_handler1() 
definition. Only use 1 if there is at least a 2. Maybe name the handler 
register_exit_handler_before_libio_exit().

>   typedef enum {
>     INIT,
>     OPEN,
> @@ -247,7 +259,45 @@ static const IMFS_node_control node_control = IMFS_GENERIC_INITIALIZER(
>     IMFS_node_destroy_default
>   );
>   
> -static void test(void)
> +/*
> + * This exit handler will be called last among the functions registered with
> + * _atexit. Check that stdio file descriptors are closed. The cleanup handler
> + * has not yet run, so the stdio file objects themselves are still open.
> + */
> +static void test_exit_handler1(void)
> +{
> +  struct stat buffer;
> +  int status;
> +
> +  status = fstat(0, &buffer);
> +  rtems_test_assert(status == -1);
> +  rtems_test_assert(errno == EBADF);

If you test an errno value, always set it before the function to some 
other value, for example

errno = 0;
status = fstat(0, &buffer);

> +
> +  status = fstat(1, &buffer);
> +  rtems_test_assert(status == -1);
> +  rtems_test_assert(errno == EBADF);
> +
> +  status = fstat(2, &buffer);
> +  rtems_test_assert(status == -1);
> +  rtems_test_assert(errno == EBADF);
> +
> +  rtems_test_assert( stdin->_flags != 0 );
> +  rtems_test_assert( stdout->_flags != 0 );
> +  rtems_test_assert( stderr->_flags != 0 );
> +}
> +
> +/*
> + * Register test_exit_handler1 at position 0 in the _atexit _fns array.
> + * Thus, test_exit_handler1 will be called last--after rtems_libio_exit().
> + */
> +static void test_sysinit_handler1(void)
> +{
> +  int rv;
> +  rv = atexit(test_exit_handler1);
> +  rtems_test_assert(rv == 0);
> +}

Maybe rename test_exit_handler1() in check_after_libio_exit().

> +
> +static void test_thread_specific_close(void)
>   {
>     test_context *ctx = &test_instance;
>     rtems_status_code sc;
> @@ -297,14 +347,79 @@ static void test(void)
>     rtems_test_assert(rtems_resource_snapshot_check(&snapshot));
>   }
>   
> +/*
> + * At this point, neither the _atexit functions nor __cleanup have been
> + * called. Therefore, stdio file descriptors should be open and stdio file
> + * object flags should be non-zero.
> + */
> +static void test_exit_handling(void)
> +{
> +  struct stat buffer;
> +  int status;
> +
> +  status = fstat(0, &buffer);
> +  rtems_test_assert(status == 0);
> +  rtems_test_assert(errno == 0);

There is no need to check for errno == 0. In fact, since fstat() is not 
supposed to write to errno if it is successful, this check could fail 
depending on what the test did before.

> +
> +  status = fstat(1, &buffer);
> +  rtems_test_assert(status == 0);
> +  rtems_test_assert(errno == 0);
> +
> +  status = fstat(2, &buffer);
> +  rtems_test_assert(status == 0);
> +  rtems_test_assert(errno == 0);
> +
> +  rtems_test_assert( stdin->_flags != 0 );
> +  rtems_test_assert( stdout->_flags != 0 );
> +  rtems_test_assert( stderr->_flags != 0 );
> +
> +  /* Run exit handlers and __cleanup */
> +  exit(0);
> +}
> +
>   static void Init(rtems_task_argument arg)
>   {
>     TEST_BEGIN();
> +  test_thread_specific_close();
> +  test_exit_handling();
> +}
>   
> -  test();
> -
> -  TEST_END();
> -  rtems_test_exit(0);
> +static void fatal_extension(
> +  rtems_fatal_source source,
> +  bool always_set_to_false,
> +  rtems_fatal_code error
> +)
> +{
> +  if (
> +    source == RTEMS_FATAL_SOURCE_EXIT
> +      && !always_set_to_false
> +      && error == 0
> +  ) {
> +
> +    /*
> +     * Final conditions check after exit handlers and __cleanup have run.
> +     * File descriptors and file objects themselves are closed.
> +     */

Please remove all Newlib internal names from the comments to avoid out 
dated comments if the Newlib internals change.

> +    struct stat buffer;
> +    int status;
> +
> +    status = fstat(0, &buffer);
> +    rtems_test_assert(status == -1);
> +    rtems_test_assert(errno == EBADF);
> +
> +    status = fstat(1, &buffer);
> +    rtems_test_assert(status == -1);
> +    rtems_test_assert(errno == EBADF);
> +
> +    status = fstat(2, &buffer);
> +    rtems_test_assert(status == -1);
> +    rtems_test_assert(errno == EBADF);
> +
> +    rtems_test_assert( stdin->_flags == 0 );
> +    rtems_test_assert( stdout->_flags == 0 );
> +    rtems_test_assert( stderr->_flags == 0 );
> +    TEST_END();
> +  }
>   }
>   
>   #define CONFIGURE_APPLICATION_NEEDS_CLOCK_DRIVER
> @@ -314,7 +429,9 @@ static void Init(rtems_task_argument arg)
>   
>   #define CONFIGURE_MAXIMUM_TASKS 2
>   
> -#define CONFIGURE_INITIAL_EXTENSIONS RTEMS_TEST_INITIAL_EXTENSION
> +#define CONFIGURE_INITIAL_EXTENSIONS \
> +  { .fatal = fatal_extension }, \
> +  RTEMS_TEST_INITIAL_EXTENSION
>   
>   #define CONFIGURE_RTEMS_INIT_TASKS_TABLE
>   

-- 
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber at embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/