[PATCH v2 1/5] libcsupport: Added futimens() and utimensat()

Ryan Long ryan.long at oarcorp.com
Mon May 10 21:15:21 UTC 2021


Reply is below.

-----Original Message-----
From: Gedare Bloom <gedare at rtems.org> 
Sent: Monday, May 10, 2021 12:36 PM
To: Ryan Long <ryan.long at oarcorp.com>
Cc: devel at rtems.org
Subject: Re: [PATCH v2 1/5] libcsupport: Added futimens() and utimensat()

On Thu, May 6, 2021 at 1:51 PM 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      |  60 +++++++++-
>  cpukit/libcsupport/src/futimens.c  |  87 ++++++++++++++  
> cpukit/libcsupport/src/utimensat.c | 239 +++++++++++++++++++++++++++++++++++++
>  spec/build/cpukit/librtemscpu.yml  |   2 +
>  5 files changed, 387 insertions(+), 3 deletions(-)  create mode 
> 100644 cpukit/libcsupport/src/futimens.c  create mode 100644 
> cpukit/libcsupport/src/utimensat.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..7a0a169 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.
> @@ -357,6 +356,61 @@ static inline void rtems_filesystem_instance_unlock(
>    (*mt_entry->ops->unlock_h)( mt_entry );  }
>
> +/* Prototypes for functions used between utimensat() and futimens() 
> +*/
> +
We don't usually have this kind of separator comment.

> +/**
> + * @brief Checks the tv_sec member of a timespec struct
> + *
> + * @param[in] time The timespec struct to be validated
> + *
> + * Ensures that the value in the tv_sec member is non-negative.
> + *
> + * @retval Returns true if the tv_sec member is a valid value, otherwise false.
> + */
> +bool rtems_filesystem_utime_tv_sec_valid( struct timespec time );
> +
Should this be a timespec helper instead? It seems like a straightforward helper.
[Ryan Long] Do you mean just change the name of the function to "timespec_helper_tv_sec_valid" or what?

> +/**
> + * @brief Checks the tv_nsec member of a timespec struct
> + *
> + * 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
> + *
> + * Checks that the process has the same userID as the file and 
> +whether the
> + * file has write permissions.
> + *
> + * @param[in] currentloc The current location to a file
> + * @param[in] fstat_h The file handler of @currentloc
> + *
> + * @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]
> +);
> +
The function name is overly broad. it seems like there should be a filesystem helper that checks for write permission (or ownership).
I don't know why times[] is passed? What is @param[in] fstat_h?

Passing an array is a bit unusual.
[Ryan Long] Forgot to update the Doxygen for that, but I got rid of the ftat_h argument because I can just use currentloc to get what was being passed in. 
                      The array of timespec structures is passed in to check for the conditions for the EACCES and EPERM errors.

> +/**
> + * @brief Checks @times and updates @new_times
> + *
> + * @param[in] times The timespec struct to be checked
> + * @param[in,out] new_times The timespec struct to contain the 
> +updated time
> + *
> + * @retval Returns 0 if the time was update, otherwise -1.
> + */
> +int rtems_filesystem_utime_check_times(
> +  const struct timespec times[2],
> +  struct timespec new_times[2]
> +);
Updates to what?
[Ryan Long] It updates to the values in times, or if times is NULL, it fills new_times with the current time. I'll change that in the description.

It may be simpler internally to explicitly refer to "access" and "modified" times instead of times[0] and times[1]?
[Ryan Long] That would make it easier to follow. I guess this would need to be changed everywhere it was used?

> +
>  /*
>   *  File Descriptor Routine Prototypes
>   */
> diff --git a/cpukit/libcsupport/src/futimens.c 
> b/cpukit/libcsupport/src/futimens.c
> new file mode 100644
> index 0000000..c2ef9da
> --- /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) 1989, 2021 On-Line Applications Research Corporation (OAR).
This is a new file. How can it have a copyright starting in 1989?

> + *
> + * 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)(
> +    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..1cd42cb
> --- /dev/null
> +++ b/cpukit/libcsupport/src/utimensat.c
> @@ -0,0 +1,239 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +
> +/**
> + *  @file
> + *
> + *  @ingroup libcsupport
> + *
> + *  @brief Set file access and modification times in nanoseconds.
> + */
> +
> +/*
> + * COPYRIGHT (C) 1989, 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>
> +
> +/* Verify that the seconds value is non-negative */ bool 
> +rtems_filesystem_utime_tv_sec_valid(struct timespec time) {
> +  if ( time.tv_sec < 0 ) {
> +    return false;
> +  }
> +
> +  return true;
> +}
> +
> +/*
> + * 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 ) {
> +    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;
> +
> +  memset( &st, 0, sizeof(st) );
nit: this is not necessary, you can use
     struct stat st = {};

> +
> +  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) ||
> +     ( (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 ) {
> +      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 ) {
> +    new_times[0].tv_sec = 0;
> +    new_times[0].tv_nsec = UTIME_NOW;
> +    new_times[1].tv_sec = 0;
> +    new_times[1].tv_nsec = 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 ( !rtems_filesystem_utime_tv_sec_valid( new_times[0] ) ) {
> +    rtems_set_errno_and_return_minus_one( EINVAL );  }
> +
> +  if ( !rtems_filesystem_utime_tv_sec_valid( 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 );  }
> +
> +  currentloc = rtems_filesystem_eval_path_start( &ctx, path, 
> + eval_flags );
> +
> +  rv = rtems_filesystem_utime_check_times( times, new_times );
would it be simpler and still correct to put this check before the eval path?

> +  if ( rv != 0 ) {
> +    rtems_filesystem_eval_path_cleanup( &ctx );
> +    return rv;
> +  }
> +
> +  rv = rtems_filesystem_utime_check_permissions( currentloc, times );  
> + 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/spec/build/cpukit/librtemscpu.yml 
> b/spec/build/cpukit/librtemscpu.yml
> index a3a9ee4..9639051 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
> --
> 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