On Mon, May 10, 2021 at 3:16 PM Ryan Long <ryan.l...@oarcorp.com> wrote: > > Reply is below. > > -----Original Message----- > From: Gedare Bloom <ged...@rtems.org> > Sent: Monday, May 10, 2021 12:36 PM > To: Ryan Long <ryan.l...@oarcorp.com> > Cc: devel@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.l...@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? >
I was thinking along the lines of the functions in include/rtems/score/timespec.h You might consider whether _Timespec_Is_non_negative() should be added. Speaking of which, you could probably use those helpers in a few places to simplify your code, e.g., _Timespec_Set( times[0], 0, UTIME_NOW); > > +/** > > + * @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? > yes. > > + > > /* > > * 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@rtems.org > > http://lists.rtems.org/mailman/listinfo/devel _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
