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

Ryan Long ryan.long at oarcorp.com
Wed May 5 17:03:27 UTC 2021


Reply is below.

-----Original Message-----
From: Gedare Bloom <gedare at rtems.org> 
Sent: Tuesday, May 4, 2021 11:27 AM
To: Ryan Long <ryan.long at oarcorp.com>
Cc: Sebastian Huber <sebastian.huber at embedded-brains.de>; devel at rtems.org
Subject: Re: [PATCH v1 1/5] libcsupport: Added futimens() and utimensat()

On Tue, May 4, 2021 at 10:04 AM Ryan Long <ryan.long at oarcorp.com> wrote:
>
>
>
> -----Original Message-----
> From: Sebastian Huber <sebastian.huber at embedded-brains.de>
> Sent: Tuesday, May 4, 2021 12:04 AM
> To: Ryan Long <ryan.long at oarcorp.com>; devel at rtems.org
> Subject: Re: [PATCH v1 1/5] libcsupport: Added futimens() and 
> utimensat()
>
> On 03/05/2021 15:40, Ryan Long 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      |  61 +++++++++-
> >   cpukit/libcsupport/src/futimens.c  |  91 +++++++++++++++
> >   cpukit/libcsupport/src/utimensat.c | 229 +++++++++++++++++++++++++++++++++++++
> >   spec/build/cpukit/librtemscpu.yml  |   2 +
> >   5 files changed, 384 insertions(+), 1 deletion(-)
> >   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..fc7a1e4 100644
> > --- a/cpukit/include/rtems/libio_.h
> > +++ b/cpukit/include/rtems/libio_.h
> > @@ -7,7 +7,7 @@
> >    */
> >
> >   /*
> > - *  COPYRIGHT (c) 1989-2011.
> > + *  COPYRIGHT (c) 1989-2011, 2021.
> >    *  On-Line Applications Research Corporation (OAR).
> >    *
> >    *  Modifications to support reference counting in the file system 
> > are @@ -357,6 +357,65 @@ static inline void rtems_filesystem_instance_unlock(
> >     (*mt_entry->ops->unlock_h)( mt_entry );
> >   }
> >
> > +/* Prototypes for functions used between utimensat() and futimens() 
> > +*/
> > +
> > +/**
> > + * @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 true If the tv_sec member is a valid value
> > + * @retval false If the tv_sec member is not a valid value  */ bool 
> > +rtems_filesystem_utime_tv_sec_valid( struct timespec time );
> > +
> > +/**
> > + * @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 true If the tv_nsec member is a valid value
> > + * @retval false If the tv_nsec member is not a valid value  */ 
> > +bool rtems_filesystem_utime_tv_nsec_valid( struct timespec time );
>
> Are these two functions ever used individually? Do they modify the time?
> Maybe we should add a standard phase for boolean type returns:
>
> @return Returns true, if the X is valid, otherwise false.
>
> [Ryan Long] These functions are only used in rtems_filesystem_utime_check_times(). They don't modify the time, they just ensure that the members of the timespec struct are valid values. Should the arguments for those functions be changed to be a const since they're not being modified? And sure, I can consolidate the @retval statements like that.
>
> > +
> > +/**
> > + * @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
> Why is this an extra parameter?
> [Ryan Long] This was Joel's suggestion, but I didn't think about just using the currentloc argument. I'll change that in V2.
> > + *
> > + * @retval 0 If the process has write permissions
> > + * @retval -1 If the process does not have write permissions
> Why 0 and -1, what about errno? Is it set by the function or not?
> [Ryan Long] It's 0 and -1 because it uses the rtems_set_errno_and_return_minus_one() macro.
> > + */
> > +int rtems_filesystem_utime_check_permissions(
> > +  const rtems_filesystem_location_info_t *currentloc,
> > +  rtems_filesystem_fstat_t fstat_h
> > +);
> > +
> > +/**
> > + * @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 0 If the time was able to be updated
> > + * @retval -1 If the time was not able to be updated  */ int 
> > +rtems_filesystem_utime_check_times(
> > +  const struct timespec times[2],
> > +  struct timespec new_times[2]
> > +);
> > +
> >   /*
> >    *  File Descriptor Routine Prototypes
> >    */
> > diff --git a/cpukit/libcsupport/src/futimens.c
> > b/cpukit/libcsupport/src/futimens.c
> > new file mode 100644
> > index 0000000..33ae469
> > --- /dev/null
> > +++ b/cpukit/libcsupport/src/futimens.c
> > @@ -0,0 +1,91 @@
> > +/* 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-2009, 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/functi
> > +on
> > +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_permissions(
> > +    currentloc,
> > +    (*currentloc->handlers->fstat_h)  );  if( rv == -1 ) {
>
> Which coding style should this file and the other new files have?
>
> [...]
>
> > +}
> > +
> > +/* Determine whether the access and modified timestamps can be 
> > +updated */ int rtems_filesystem_utime_check_permissions(
> > +  const rtems_filesystem_location_info_t * currentloc,
> > +  rtems_filesystem_fstat_t fstat_h
> > +)
> > +{
> > +  struct stat st;
> > +  volatile int rv;
> volatile?
> [Ryan Long] Forgot to take this out after debugging. Will be removed in V2.
> > +  bool write_access;
> > +
> > +  memset( &st, 0, sizeof(st) );
> > +
> > +  rv = (*fstat_h)( currentloc, &st );  if( rv != 0 ) {
> > +    rtems_set_errno_and_return_minus_one( ENOENT );  }
> > +
> > +  /*
> > +   * Check that the user ID and the user ID of the file match
> > +   */
> > +  if( geteuid() != st.st_uid ) {
> > +    rtems_set_errno_and_return_minus_one( EACCES );  }
> Is this really what POSIX specified?
> [Ryan Long] Yes, on https://pubs.opengroup.org/onlinepubs/9699919799.2008edition/functions/futimens.html they said return this if 'the effective user ID of the process does not match the owner of the file and write access is denied.'
> > +

But you haven't checked write access yet. I think you haven't parsed the specification correctly.
[Ryan Long] So I should move the assignment of write_access above this check, and change the check to be this?

if( geteuid() != st.st_uid) {
  if( !write_access) {
    rtems_set_errno_and_return_minus_one( EACCES );
  }
}

> > +  write_access = rtems_filesystem_check_access(
> > +    RTEMS_FS_PERMS_WRITE,
> > +    st.st_mode,
> > +    st.st_uid,
> > +    st.st_gid
> > +  );
> > +  if( !write_access ) {
> > +    rtems_set_errno_and_return_minus_one( EACCES );  }
> > +
> > +  return 0;
> > +}
>
> > [...]
> > +
> > +
> > +/**
> > + *  
> > +https://pubs.opengroup.org/onlinepubs/9699919799.2008edition/functi
> > +ons/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_permissions(
> > +    currentloc,
> > +    (*currentloc->handlers->fstat_h)  );  if( rv == -1 ) {
> Better use the success code, for example rv != 0.
> > +    rtems_filesystem_eval_path_cleanup( &ctx );
> > +    return rv;
> > +  }
> > +
> > +  rv = rtems_filesystem_utime_check_times( times, new_times);  if( 
> > + rv == -1 ) {
> > +    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
>
> --
> 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/
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel


More information about the devel mailing list