Re: [PATCH v2 1/5] libcsupport: Added futimens() and utimensat()
On Thu, May 6, 2021 at 1:51 PM 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 | 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. > +/** > + * @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. > +/** > + * @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? It may be simpler internally to explicitly refer to "access" and "modified" times instead of times[0] and times[1]? > + > /* > * File Descriptor Routine Prototypes > */ > diff --git a/cpukit/libcsupport/src/futimens.c > b/cpukit/libcsupport/src/futimens.c > new file mode 100644 > index 000..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
RE: [PATCH v2 1/5] libcsupport: Added futimens() and utimensat()
Reply is below. -Original Message- From: Gedare Bloom Sent: Monday, May 10, 2021 12:36 PM To: Ryan Long 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 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 up
Re: [PATCH v2 1/5] libcsupport: Added futimens() and utimensat()
On Mon, May 10, 2021 at 3:16 PM Ryan Long wrote: > > Reply is below. > > -Original Message- > From: Gedare Bloom > Sent: Monday, May 10, 2021 12:36 PM > To: Ryan Long > 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 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
RE: [PATCH v2 1/5] libcsupport: Added futimens() and utimensat()
-Original Message- From: Gedare Bloom Sent: Monday, May 10, 2021 11:50 PM To: Ryan Long Cc: devel@rtems.org Subject: Re: [PATCH v2 1/5] libcsupport: Added futimens() and utimensat() On Mon, May 10, 2021 at 3:16 PM Ryan Long wrote: > > Reply is below. > > -Original Message- > From: Gedare Bloom > Sent: Monday, May 10, 2021 12:36 PM > To: Ryan Long > 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 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 ); > > + > > +/** > > +
