[PATCH v3 1/5] libcsupport: Added futimens() and utimensat()
Ryan Long
ryan.long at oarcorp.com
Fri May 14 14:48:54 UTC 2021
Reply is below.
-----Original Message-----
From: Gedare Bloom <gedare at rtems.org>
Sent: Wednesday, May 12, 2021 12:34 PM
To: Ryan Long <ryan.long at oarcorp.com>
Cc: devel at rtems.org
Subject: Re: [PATCH v3 1/5] libcsupport: Added futimens() and utimensat()
On Wed, May 12, 2021 at 7:33 AM Ryan Long <ryan.long at oarcorp.com> wrote:
>
> Created futimens.c and utimensat.c to add support for the POSIX
> methods futimens() and utimensat().
>
> utime() and utimes() are considered obsolote by POSIX, but RTEMS will
> continue to support them.
>
> Closes #4396
> ---
> cpukit/Makefile.am | 2 +
> cpukit/include/rtems/libio_.h | 64 ++++++++-
> cpukit/include/rtems/score/timespec.h | 16 ++-
> cpukit/libcsupport/src/futimens.c | 87 ++++++++++++
> cpukit/libcsupport/src/utimensat.c | 224 +++++++++++++++++++++++++++++++
> cpukit/score/src/timespecisnonnegative.c | 35 +++++
> spec/build/cpukit/librtemscpu.yml | 3 +
> 7 files changed, 426 insertions(+), 5 deletions(-) create mode
> 100644 cpukit/libcsupport/src/futimens.c create mode 100644
> cpukit/libcsupport/src/utimensat.c
> create mode 100644 cpukit/score/src/timespecisnonnegative.c
>
> diff --git a/cpukit/Makefile.am b/cpukit/Makefile.am index
> b0df610..29b4207 100644
> --- a/cpukit/Makefile.am
> +++ b/cpukit/Makefile.am
> @@ -262,6 +262,8 @@ librtemscpu_a_SOURCES += libcsupport/src/unmount.c
> librtemscpu_a_SOURCES += libcsupport/src/__usrenv.c
> librtemscpu_a_SOURCES += libcsupport/src/utime.c
> librtemscpu_a_SOURCES += libcsupport/src/utimes.c
> +librtemscpu_a_SOURCES += libcsupport/src/futimens.c
> +librtemscpu_a_SOURCES += libcsupport/src/utimensat.c
> librtemscpu_a_SOURCES += libcsupport/src/utsname.c
> librtemscpu_a_SOURCES += libcsupport/src/vprintk.c
> librtemscpu_a_SOURCES += libcsupport/src/write.c diff --git
> a/cpukit/include/rtems/libio_.h b/cpukit/include/rtems/libio_.h index
> e9eb462..b7455ef 100644
> --- a/cpukit/include/rtems/libio_.h
> +++ b/cpukit/include/rtems/libio_.h
> @@ -2,13 +2,12 @@
> * @file
> *
> * @brief LibIO Internal Interface
> - *
> + *
> * This file is the libio internal interface.
> */
>
> /*
> - * COPYRIGHT (c) 1989-2011.
> - * On-Line Applications Research Corporation (OAR).
> + * COPYRIGHT (C) 1989, 2021 On-Line Applications Research Corporation (OAR).
> *
> * Modifications to support reference counting in the file system are
> * Copyright (c) 2012 embedded brains GmbH.
> @@ -30,6 +29,7 @@
> #include <rtems/libio.h>
> #include <rtems/seterr.h>
> #include <rtems/score/assert.h>
> +#include <rtems/score/timespec.h>
>
> #ifdef __cplusplus
> extern "C" {
> @@ -357,6 +357,64 @@ static inline void rtems_filesystem_instance_unlock(
> (*mt_entry->ops->unlock_h)( mt_entry ); }
>
> +/**
> + * @brief Checks the tv_nsec member of a timespec struct
> + *
> + * This function is used with utimensat() and futimens() only. This
> +ensures
> + * that the value in the tv_nsec member is equal to either UTIME_NOW,
> + * UTIME_OMIT, or a value greater-than or equal to zero and less than
> +a
> + * billion.
> + *
> + * @param[in] time The timespec struct to be validated
> + *
> + * @retval Returns true if tv_nsec member is a valid value, otherwise false.
> + */
> +bool rtems_filesystem_utime_tv_nsec_valid( struct timespec time );
> +
> +/**
> + * @brief Determines if the process has write permissions to a file
I think it is more specific than that. Also, missing period.
> + *
> + * The timespec at index 0 is the access time, the timespec at index
> + 1 is the
> + * modification time.
Should this note be on the @param[in] times below?
[Ryan Long] Makes sense. I'll move it there.
> + *
> + * This function is only used with utimensat() and futimens(). This
> + checks
> + * whether the process has write access to a file, and checks for
> + EACCES
> + * and EPERM errors depending on what values are in @times and if the
> + process
> + * has write permissions to the file.
> + *
> + * @param[in] currentloc The current location to a file
> + * @param[in] times The timespec instance that will update the
> + timestamps
update? nothing about this function seems to be an update?
[Ryan Long] I'll change this to just say something like "The timespec instance used to check for errors". Is there another name for this function that you think would fit better?
While it is checking for the appropriate permissions, it's also checking for errors, so I'm not sure if this function name fits anymore.
> + *
> + * @retval Returns 0 if the process has write permissions, otherwise -1.
> + */
> +int rtems_filesystem_utime_check_permissions(
> + const rtems_filesystem_location_info_t *currentloc,
> + const struct timespec times[2]
> +);
> +
> +/**
> + * @brief Checks @times and fills @new_times with the time to be
> +written
> + *
> + * For each of the arguments, the timespec at index 0 is the access
> +time, and
> + * the timespec at index 1 is the modification time.
> + *
> + * This function is only used with utimensat() and futimens(). @times
> +contains
> + * the constant values passed into utimensat/futimens. @new_times
> +contains the
> + * values that will be written to the file. These values depend on
> + at times. If
> + * @times is NULL, or either of its elements' tv_nsec members are
> +UTIME_NOW,
> + * the current elapsed time in nanoseconds will be saved in the
> +corresponding
> + * location in @new_times.
> + *
> + * @param[in] times The timespecs to be checked
> + * @param[in,out] new_times The timespecs containing the time to be
> +written
I don't think new_times is an 'in' param?
> + *
> + * @retval Returns 0 if @times is valid, otherwise -1.
> + */
> +int rtems_filesystem_utime_check_times(
this function name is strange. A "check" normally doesn't have side effects? This seems to be updating the new_times.
[Ryan Long] It is. Since this is a function that's only used to make the code easier to follow, would it be fine to just name it "rtems_filesystem_utime_fill_new_times()"? Or is there something else you suggest?
> + const struct timespec times[2],
> + struct timespec new_times[2]
> +);
> +
> /*
> * File Descriptor Routine Prototypes
> */
> diff --git a/cpukit/include/rtems/score/timespec.h
> b/cpukit/include/rtems/score/timespec.h
> index 314d804..3671067 100644
> --- a/cpukit/include/rtems/score/timespec.h
> +++ b/cpukit/include/rtems/score/timespec.h
> @@ -8,8 +8,7 @@
> */
>
> /*
> - * COPYRIGHT (c) 1989-2009.
> - * On-Line Applications Research Corporation (OAR).
> + * COPYRIGHT (C) 1989, 2021 On-Line Applications Research Corporation (OAR).
> *
> * The license and distribution terms for this file may be
> * found in the file LICENSE in this distribution or at @@ -106,6
> +105,19 @@ uint64_t _Timespec_Get_as_nanoseconds( );
>
> /**
> + * @brief Checks the tv_sec member of a timespec struct
clarify this brief.
> + *
> + * @param[in] time is the timespec instance to validate.
> + *
> + * Ensures that the values in @a time are non-negative.
This would be a better brief, except it should be "checks if" rather than "ensures that"
> + *
> + * @retval Returns true if the tv_sec member is a valid value, otherwise false.
this is not the right way to write retval, the value should come first
* @retval true The tv_sec member is a valid value.
* @retval false The tv_sec member is not a valid value.
This problem exists elsewhere/throughout.
If our doxygen guidelines are not clear, we should update them.
[Ryan Long] I think I had changed this based on a comment from Sebastian, but I guess he was just throwing the idea out there that he thought this would be a better approach. I'll change this back.
> + */
> +bool _Timespec_Is_non_negative(
> + const struct timespec *time
> +);
> +
> +/**
> * @brief Checks if timespec is valid.
> *
> * This method determines the validity of a timespec.
> diff --git a/cpukit/libcsupport/src/futimens.c
> b/cpukit/libcsupport/src/futimens.c
> new file mode 100644
> index 0000000..44ec03d
> --- /dev/null
> +++ b/cpukit/libcsupport/src/futimens.c
> @@ -0,0 +1,87 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +
> +/**
> + * @file
> + *
> + * @ingroup libcsupport
> + *
> + * @brief Set file access and modification times based on file
> +descriptor in
> + * nanoseconds.
> + */
> +
> +/*
> + * COPYRIGHT (C) 2021 On-Line Applications Research Corporation (OAR).
> + *
> + * 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 <sys/stat.h>
> +#include <rtems/libio_.h>
> +
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +/**
> + *
> +https://pubs.opengroup.org/onlinepubs/9699919799.2008edition/function
> +s/futimens.html
> + *
> + * Set file access and modification times */ int futimens(
> + int fd,
> + const struct timespec times[2]
> +)
> +{
> + int rv;
> + rtems_libio_t *iop;
> + struct timespec new_times[2];
> + const rtems_filesystem_location_info_t *currentloc = NULL;
> +
> + LIBIO_GET_IOP_WITH_ACCESS( fd, iop, LIBIO_FLAGS_READ, EBADF );
> +
> + currentloc = &iop->pathinfo;
> +
> + rv = rtems_filesystem_utime_check_times( times, new_times ); if (
> + rv != 0 ) {
> + rtems_libio_iop_drop( iop );
> + return rv;
> + }
> +
> + rv = rtems_filesystem_utime_check_permissions( currentloc, times );
> + if ( rv != 0 ) {
> + rtems_libio_iop_drop( iop );
> + return rv;
> + }
> +
> + rv = (*currentloc->mt_entry->ops->utimens_h)(
I don't think this utimens_h exists yet. How did you compile RTEMS successfully with this patch applied, before applying Patches 2 and 3?
[Ryan Long] Good catch. I'll take this out and put it in the right commit.
> + currentloc,
> + new_times
> + );
> +
> + rtems_libio_iop_drop( iop );
> +
> + return rv;
> +}
> diff --git a/cpukit/libcsupport/src/utimensat.c
> b/cpukit/libcsupport/src/utimensat.c
> new file mode 100644
> index 0000000..59ba369
> --- /dev/null
> +++ b/cpukit/libcsupport/src/utimensat.c
> @@ -0,0 +1,224 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +
> +/**
> + * @file
> + *
> + * @ingroup libcsupport
> + *
> + * @brief Set file access and modification times in nanoseconds.
> + */
> +
> +/*
> + * COPYRIGHT (C) 2021 On-Line Applications Research Corporation (OAR).
> + *
> + * 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 <rtems/libio_.h>
> +
> +#include <fcntl.h>
> +#include <string.h>
> +
> +/*
> + * Make sure that tv_nsec is either UTIME_NOW, UTIME_OMIT, a value
> + * greater than zero, or a value less-than a billion.
> + *
> + * These guidelines come from the description of the EINVAL errors on
> + *
> +https://pubs.opengroup.org/onlinepubs/9699919799/functions/futimens.h
> +tml
> + */
> +bool rtems_filesystem_utime_tv_nsec_valid(struct timespec time) {
> + if ( time.tv_nsec == UTIME_NOW ) {
> + return true;
> + }
> +
> + if ( time.tv_nsec == UTIME_OMIT ) {
> + return true;
> + }
> +
> + if ( time.tv_nsec < 0 ) {
> + return false;
> + }
> +
> + if ( time.tv_nsec > 999999999 ) {
Elsewhere we have instead used
nsec >= TOD_NANOSECONDS_PER_SECOND
That would be preferred here also.
> + return false;
> + }
> +
> + return true;
> +}
> +
> +/* Determine whether the access and modified timestamps can be
> +updated */ int rtems_filesystem_utime_check_permissions(
> + const rtems_filesystem_location_info_t * currentloc,
> + const struct timespec times[2]
> +)
> +{
> + struct stat st = {};
> + int rv;
> + bool write_access;
> +
> + rv = (*currentloc->handlers->fstat_h)( currentloc, &st ); if ( rv
> + != 0 ) {
> + rtems_set_errno_and_return_minus_one( ENOENT ); }
> +
> + write_access = rtems_filesystem_check_access(
> + RTEMS_FS_PERMS_WRITE,
> + st.st_mode,
> + st.st_uid,
> + st.st_gid
> + );
> +
> + /*
> + * The logic for the EACCES error is an inverted subset of the EPERM
> + * conditional according to the POSIX standard.
> + */
> + if ( (times == NULL) ||
Did you test the times == NULL case?
> + ( (times[0].tv_nsec == UTIME_NOW) && (times[1].tv_nsec == UTIME_NOW) )) {
> + if ( !write_access ) {
> + rtems_set_errno_and_return_minus_one( EACCES );
> + }
> + } else {
> + if ( times[0].tv_nsec != UTIME_OMIT || times[1].tv_nsec !=
> + UTIME_OMIT ) {
What happens here if times==NULL?
> + if ( !write_access ) {
> + rtems_set_errno_and_return_minus_one( EPERM );
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Determine if the current time needs to be gotten, and then check
> + * whether the values in times are valid.
> + */
> +int rtems_filesystem_utime_check_times(
> + const struct timespec times[2],
> + struct timespec new_times[2]
> +)
> +{
> + bool got_time = false;
> + struct timespec now;
> +
> + /*
> + * If times is NULL, it's equivalent to adding UTIME_NOW in both time
> + * elements
> + */
> + if ( times == NULL ) {
> + _Timespec_Set( &new_times[0], 0, UTIME_NOW );
> + _Timespec_Set( &new_times[1], 0, UTIME_NOW ); } else {
> + new_times[0] = times[0];
> + new_times[1] = times[1];
> + }
> +
> + if ( new_times[0].tv_nsec == UTIME_NOW ) {
> + clock_gettime( CLOCK_REALTIME, &now );
> + new_times[0] = now;
> + got_time = true;
> + }
> +
> + if ( new_times[1].tv_nsec == UTIME_NOW ) {
> + if ( !got_time ) {
> + clock_gettime( CLOCK_REALTIME, &now );
> + }
> + new_times[1] = now;
> + }
> +
> + if ( !_Timespec_Is_non_negative( &new_times[0] ) ) {
> + rtems_set_errno_and_return_minus_one( EINVAL ); }
> +
> + if ( !_Timespec_Is_non_negative( &new_times[1] ) ) {
> + rtems_set_errno_and_return_minus_one( EINVAL ); }
> +
> + if ( !rtems_filesystem_utime_tv_nsec_valid( new_times[0] ) ) {
> + rtems_set_errno_and_return_minus_one( EINVAL ); }
> +
> + if ( !rtems_filesystem_utime_tv_nsec_valid( new_times[1] ) ) {
> + rtems_set_errno_and_return_minus_one( EINVAL ); }
> +
> + return 0;
> +}
> +
> +/**
> + *
> +https://pubs.opengroup.org/onlinepubs/9699919799.2008edition/function
> +s/futimens.html
> + *
> + * Set file access and modification times */ int utimensat(
> + int fd,
> + const char *path,
> + const struct timespec times[2],
> + int flag
> +)
> +{
> + int rv = 0;
> + rtems_filesystem_eval_path_context_t ctx;
> + int eval_flags = RTEMS_FS_FOLLOW_LINK;
> + const rtems_filesystem_location_info_t *currentloc = NULL;
> + struct timespec new_times[2];
> +
> + /*
> + * RTEMS does not currently support operating on a real file descriptor
> + */
> + if ( fd != AT_FDCWD ) {
> + rtems_set_errno_and_return_minus_one( ENOSYS ); }
> +
> + /*
> + * RTEMS does not currently support AT_SYMLINK_NOFOLLOW
> + */
> + if ( flag != 0 ) {
> + rtems_set_errno_and_return_minus_one( ENOSYS ); }
> +
> + rv = rtems_filesystem_utime_check_times( times, new_times ); if (
> + rv != 0 ) {
> + return rv;
> + }
> +
> + currentloc = rtems_filesystem_eval_path_start( &ctx, path,
> + eval_flags );
> +
> + rv = rtems_filesystem_utime_check_permissions( currentloc, times
> + );
indent wrong here
> + if ( rv != 0 ) {
> + rtems_filesystem_eval_path_cleanup( &ctx );
> + return rv;
> + }
> +
> + rv = (*currentloc->mt_entry->ops->utimens_h)(
> + currentloc,
> + new_times
> + );
> +
> + rtems_filesystem_eval_path_cleanup( &ctx );
> +
> + return rv;
> +}
> diff --git a/cpukit/score/src/timespecisnonnegative.c
> b/cpukit/score/src/timespecisnonnegative.c
> new file mode 100644
> index 0000000..080e961
> --- /dev/null
> +++ b/cpukit/score/src/timespecisnonnegative.c
> @@ -0,0 +1,35 @@
spdx
> +/**
> + * @file
> + *
> + * @ingroup RTEMSScoreTimespec
> + *
> + * @brief This source file contains the implementation of
> + * _Timespec_Is_non_negative().
> + */
> +
> +/*
> + * COPYRIGHT (C) 2021 On-Line Applications Research Corporation (OAR).
> + *
> + * The license and distribution terms for this file may be
> + * found in the file LICENSE in this distribution or at
> + * http://www.rtems.org/license/LICENSE.
2-bsd?
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include "config.h"
> +#endif
> +
> +#include <rtems/score/timespec.h>
> +
> +bool _Timespec_Is_non_negative(
> + const struct timespec *time
> +)
> +{
> + if ( time->tv_sec < 0 )
> + return false;
> +
> + if ( time->tv_nsec < 0 )
> + return false;
> +
> + return true;
> +}
> diff --git a/spec/build/cpukit/librtemscpu.yml
> b/spec/build/cpukit/librtemscpu.yml
> index a3a9ee4..7f36ef7 100644
> --- a/spec/build/cpukit/librtemscpu.yml
> +++ b/spec/build/cpukit/librtemscpu.yml
> @@ -761,6 +761,8 @@ source:
> - cpukit/libcsupport/src/unmount.c
> - cpukit/libcsupport/src/utime.c
> - cpukit/libcsupport/src/utimes.c
> +- cpukit/libcsupport/src/futimens.c
> +- cpukit/libcsupport/src/utimensat.c
> - cpukit/libcsupport/src/utsname.c
> - cpukit/libcsupport/src/vprintk.c
> - cpukit/libcsupport/src/write.c
> @@ -1558,6 +1560,7 @@ source:
> - cpukit/score/src/threadyield.c
> - cpukit/score/src/timespecaddto.c
> - cpukit/score/src/timespecdivide.c
> +- cpukit/score/src/timespecisnonnegative.c
> - cpukit/score/src/timespecdividebyinteger.c
> - cpukit/score/src/timespecfromticks.c
> - cpukit/score/src/timespecgetasnanoseconds.c
> --
> 1.8.3.1
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
More information about the devel
mailing list