[PATCH v2 1/5] libcsupport: Added futimens() and utimensat()
Ryan Long
ryan.long at oarcorp.com
Tue May 11 18:19:12 UTC 2021
-----Original Message-----
From: Gedare Bloom <gedare at rtems.org>
Sent: Monday, May 10, 2021 11:50 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 Mon, May 10, 2021 at 3:16 PM Ryan Long <ryan.long at oarcorp.com> wrote:
>
> 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?
>
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);
[Ryan Long] Both of those changes will be in the next version.
> > +/**
> > + * @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.
[Ryan Long] I tried to change it back to have an argument for "access" and "modified" times, but with how some of the checks need to be done per the POSIX specification, it made it more complicated.
It seems best to keep the timespec arrays in as arguments. I just need to add some more documentation explaining that times[0] is the access time and times[1] is the modified time.
> > +
> > /*
> > * 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/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_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/functi
> > +on
> > +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