Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008

On Thu, Aug 15, 2019, 10:59 AM Dionna Amalie Glaze via newlib <
[email protected]> wrote:

> You would also need the st_*time defines to be defined if
> !(defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)) instead of
> the current defined(__rtems__). Otherwise that seems fine by me.
>
> On Thu, Aug 15, 2019 at 3:03 AM Corinna Vinschen <[email protected]>
> wrote:
>
> > On Aug 14 12:49, Dionna Amalie Glaze via newlib wrote:
> > > Fixed the __MISC_VISIBLE part.
> > > I'm not sure I understand your question. I'm just restructuring how
> that
> > > code gets exposed. Where previously the timespec and blocks were
> defined
> > if
> > > rtems, and otherwise just the timespec if srv4 etc, I've changed the
> > > timespec declarations to all be grouped together. The block
> declarations
> > > are separate because only the #else after defined(__rtems__) is
> evaluated
> > > false and `defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)`
> > is
> > > evaluated false.
> > >
> > > ---
> > >  newlib/libc/include/sys/stat.h | 64 ++++++++++++++++++----------------
> > >  1 file changed, 34 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/newlib/libc/include/sys/stat.h
> > b/newlib/libc/include/sys/stat.h
> > > index eee98db64..d7d08e830 100644
> > > --- a/newlib/libc/include/sys/stat.h
> > > +++ b/newlib/libc/include/sys/stat.h
> > > @@ -24,7 +24,7 @@ extern "C" {
> > >  #define stat64 stat
> > >  #endif
> > >  #else
> > > -struct stat
> > > +struct stat
> > >  {
> > >    dev_t                st_dev;
> > >    ino_t                st_ino;
> > > @@ -34,15 +34,11 @@ struct      stat
> > >    gid_t                st_gid;
> > >    dev_t                st_rdev;
> > >    off_t                st_size;
> > > -#if defined(__rtems__)
> > > +#if __MISC_VISIBLE || __POSIX_VISIBLE >= 200809 || defined(__rtems__)
> >
> > Do we really need that?  I'm cringing at the idea to redefine a struct
> > based on macros set depending on user settings.  Can't we simplify this?
> > AFAICS, the timestamps definition of rtems is equivalent to the
> timestamps
> > definition of all other targets, except svr4 etc.  The only difference
> > is the additional st_spare4.
> >
> > I'd like to make the following suggestion, so all targets except svr4
> etc.
> > default to the POSIX compatible definition:
> >
> > diff --git a/newlib/libc/include/sys/stat.h
> > b/newlib/libc/include/sys/stat.h
> > index eee98db64a9a..e460c69c963f 100644
> > --- a/newlib/libc/include/sys/stat.h
> > +++ b/newlib/libc/include/sys/stat.h
> > @@ -34,27 +34,17 @@ struct      stat
> >    gid_t                st_gid;
> >    dev_t                st_rdev;
> >    off_t                st_size;
> > -#if defined(__rtems__)
> > -  struct timespec st_atim;
> > -  struct timespec st_mtim;
> > -  struct timespec st_ctim;
> > -  blksize_t     st_blksize;
> > -  blkcnt_t     st_blocks;
> > -#else
> > -  /* SysV/sco doesn't have the rest... But Solaris, eabi does.  */
> >  #if defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)
> >    time_t       st_atime;
> >    time_t       st_mtime;
> >    time_t       st_ctime;
> >  #else
> > -  time_t       st_atime;
> > -  long         st_spare1;
> > -  time_t       st_mtime;
> > -  long         st_spare2;
> > -  time_t       st_ctime;
> > -  long         st_spare3;
> > -  blksize_t    st_blksize;
> > +  struct timespec st_atim;
> > +  struct timespec st_mtim;
> > +  struct timespec st_ctim;
> > +  blksize_t     st_blksize;
> >    blkcnt_t     st_blocks;
> > +#if !defined(__rtems__)
> >    long st_spare4[2];
> >  #endif
> >  #endif
> >
> >
> > Thoughts?


I think this looks good unless there is a concern for small memory targets.
But this isn't a structure that is forced on every thread so I don't see
any concern. Make it as standard as possible. :)

--joel

> Corinna
> >
> > --
> > Corinna Vinschen
> > Cygwin Maintainer
> > Red Hat
> >
>
>
> --
> -Dionna Glaze, PhD (she/her)
>