svn commit: r205792 - in head: lib/libc/sys sys/amd64/linux32
sys/compat/freebsd32 sys/compat/linux sys/compat/svr4 sys/i386/ibcs2
sys/i386/linux sys/kern sys/sys
Bruce Evans
brde at optusnet.com.au
Tue Mar 30 03:01:27 UTC 2010
On Sun, 28 Mar 2010, Ed Schouten wrote:
> Log:
> Rename st_*timespec fields to st_*tim for POSIX 2008 compliance.
>
> A nice thing about POSIX 2008 is that it finally standardizes a way to
> obtain file access/modification/change times in sub-second precision,
> namely using struct timespec, which we already have for a very long
> time. Unfortunately POSIX uses different names.
>
> This commit adds compatibility macros, so existing code should still
> build properly. Also change all source code in the kernel to work
> without any of the compatibility macros. This makes it all a less
> ambiguous.
>
> I am also renaming st_birthtime to st_birthtim, even though it was a
> local extension anyway. It seems Cygwin also has a st_birthtim.
st_birthtime should have been st_btime (for the seconds part). Now
st_btim for the timespec. Presumably POSIX chose to abbreviate "time"
instead of expanding st_xtime when providing the new names so as to
keep the names short, so a name like st_birthtim which is abbreviated
but not short is especially bogus.
> Modified: head/sys/kern/tty_pts.c
> ==============================================================================
> --- head/sys/kern/tty_pts.c Sun Mar 28 12:55:31 2010 (r205791)
> +++ head/sys/kern/tty_pts.c Sun Mar 28 13:13:22 2010 (r205792)
> @@ -556,9 +556,9 @@ ptsdev_stat(struct file *fp, struct stat
> #endif /* PTS_EXTERNAL */
> sb->st_ino = sb->st_rdev = tty_udev(tp);
>
> - sb->st_atimespec = dev->si_atime;
> - sb->st_ctimespec = dev->si_ctime;
> - sb->st_mtimespec = dev->si_mtime;
> + sb->st_atim = dev->si_atime;
> + sb->st_ctim = dev->si_ctime;
> + sb->st_mtim = dev->si_mtime;
> sb->st_uid = dev->si_uid;
> sb->st_gid = dev->si_gid;
> sb->st_mode = dev->si_mode | S_IFCHR;
>
Should probably rename other xx_ytime's. But I don't like renaming.
devfs is still missing support for st_btim, and this cannot be fixed here.
> Modified: head/sys/kern/uipc_mqueue.c
> ==============================================================================
> --- head/sys/kern/uipc_mqueue.c Sun Mar 28 12:55:31 2010 (r205791)
> +++ head/sys/kern/uipc_mqueue.c Sun Mar 28 13:13:22 2010 (r205792)
> @@ -2447,10 +2447,10 @@ mqf_stat(struct file *fp, struct stat *s
> struct mqfs_node *pn = fp->f_data;
>
> bzero(st, sizeof *st);
> - st->st_atimespec = pn->mn_atime;
> - st->st_mtimespec = pn->mn_mtime;
> - st->st_ctimespec = pn->mn_ctime;
> - st->st_birthtimespec = pn->mn_birth;
> + st->st_atim = pn->mn_atime;
> + st->st_mtim = pn->mn_mtime;
> + st->st_ctim = pn->mn_ctime;
> + st->st_birthtim = pn->mn_birth;
> st->st_uid = pn->mn_uid;
> st->st_gid = pn->mn_gid;
> st->st_mode = S_IFIFO | pn->mn_mode;
>
As for si_ytime's, but not missing support for st_btim and different (more)
bogus name for mn_btim.
Similarly for some other xx_ytime's.
> Modified: head/sys/sys/_timespec.h
> ==============================================================================
> --- head/sys/sys/_timespec.h Sun Mar 28 12:55:31 2010 (r205791)
> +++ head/sys/sys/_timespec.h Sun Mar 28 13:13:22 2010 (r205792)
> @@ -31,26 +31,18 @@
> * $FreeBSD$
> */
>
> -/*
> - * Prerequisite: <sys/_types.h>
> - *
> - * This file must be kept synchronized with <sys/timespec.h>.
> - * It defines a structure which must be a type pun for
> - * `struct timespec'; this structure is used in header files where
> - * the ABI uses a `struct timespec' but standards prohibit its
> - * definition. (Currently only <sys/stat.h>.)
> - *
> - * XXX should just declare struct __timespec as necessary. It's simple,
> - * so is easy to keep synchronized, and hopefully not needed in as many
> - * places as struct timespec, so we don't need this extra header.
> - * Perhaps we don't need timespec.h either.
> - */
The second paragraph still applies, even more than before. This file is
more bogus than before. If the first comment (about "Currently only")
was still correct, then this file is now completely bogus. In fact, the
first comment was still correct -- this file is only included by
<sys/stat.h>. Thus this file is now bogus -- struct timespec is now
standard pollution in <sys/stat.h>.
> -
> #ifndef _SYS__TIMESPEC_H_
> #define _SYS__TIMESPEC_H_
>
> -struct __timespec {
> - __time_t tv_sec; /* seconds */
> +#include <sys/_types.h>
> +
> +#ifndef _TIME_T_DECLARED
> +typedef __time_t time_t;
> +#define _TIME_T_DECLARED
> +#endif
> +
> +struct timespec {
> + time_t tv_sec; /* seconds */
> long tv_nsec; /* and nanoseconds */
> };
>
As also mentioned in the deleted but still relevant comment, there yet
another timespec file -- sys/timespec.h. It also declares struct timespec,
and in the current organization it is the file that is supposed to be
included to get the declaration of struct timespec when this declararation
is not pollution. However, this file also declares struct itimerspec,
and maybe that pollution is not yet permitted in <sys/stat.h> by POSIX.
POSIX's man page for sys/stat.h doesn't mention struct itimerspec.
In fact, struct itimerspec seems to be just recent breakage (by
pollution) of sys/timespec.h. There are currently only 3 other headers
that include sys/timespec.h: time.h, sys/select.h and sys/time.h. None
of these use struct itimerspec. struct itimerspec is only used in
sys/timers.h, which pollutes itself by including sys/time.h (which
itself is massively polluted by including <time.h>). OTOH, <time.h>
is non-polluted to a fault. In POSIX, struct itimerspec is supposed to
be declared in <time.h>, and so are the functions that use it. FreeBSD's
<sys/timers.h> and <timers.h> don't seem to be mentioned in POSIX, and
<time.h>'s lack of fault includes not including either of these, so the
POSIX interfaces involving struct itimerspec can't be accessed in the
normal way under FreeBSD. Bugs in this area used to be limited to the
decomposition of <time.h> and <sys/time.h> being almost perfectly backwards,
with <time.h> holding almost all time interfaces in POSIX and <sys/time.h>
holding (via nested pollution) almost all time interfaces in FreeBSD.
The first steps to untangle this would be:
- move the declaration of struct itimerspec from sys/timespec.h to
sys/timers.h, thus unpolluting the former
- remove sys/_timespec.h, and use sys/timespec.h instead of it in the
place where it is used (sys/stat.h).
Although I was responsible for many of them, I don't like most of the
little sys/_foo.h headers. They just avoid a few ifdefs and a few
duplicates of struct declarations, at a cost of 10 times as much code
consisting mainly of copyright comments and silly compile-time costs
for many more includes and parsing of copyright comments... struct
timespec is so simple that repeating its declaration is better. Ifdefs
still tend to be needed for things like the time_t typedef and avoiding
1 ifdef for the struct doesn't provide much avoidance of duplication.
> Modified: head/sys/sys/stat.h
> ==============================================================================
> --- head/sys/sys/stat.h Sun Mar 28 12:55:31 2010 (r205791)
> +++ head/sys/sys/stat.h Sun Mar 28 13:13:22 2010 (r205792)
> @@ -39,6 +39,7 @@
> #define _SYS_STAT_H_
>
> #include <sys/cdefs.h>
> +#include <sys/_timespec.h>
Can include sys/timespec.h after fixing it.
> #include <sys/_types.h>
>
> #ifndef _BLKSIZE_T_DECLARED
> @@ -86,11 +87,6 @@ typedef __off_t off_t;
> #define _OFF_T_DECLARED
> #endif
>
> -#ifndef _TIME_T_DECLARED
> -typedef __time_t time_t;
> -#define _TIME_T_DECLARED
> -#endif
> -
Also not needed when sys/timespec.h is used.
> #ifndef _UID_T_DECLARED
> typedef __uid_t uid_t;
> #define _UID_T_DECLARED
> @@ -98,16 +94,11 @@ typedef __uid_t uid_t;
>
> #if !defined(_KERNEL) && __BSD_VISIBLE
> /*
> - * XXX we need this for struct timespec. We get miscellaneous namespace
> - * pollution with it.
> + * XXX We get miscellaneous namespace pollution with this.
> */
> #include <sys/time.h>
> #endif
>
> -#if !__BSD_VISIBLE
> -#include <sys/_timespec.h>
> -#endif
> -
But this can't be fixed so simply. We now get namespace pollution from
<sys/[_]timespec.h>. struct timespec is only permitted here in recent
versions of POSIX. There needs to be messy _POSIX_C_SOURCE >= 2008mumble
ifdefs somewhere...
> #if __BSD_VISIBLE
> struct ostat {
> __uint16_t st_dev; /* inode's device */
> @@ -118,9 +109,9 @@ struct ostat {
> __uint16_t st_gid; /* group ID of the file's group */
> __uint16_t st_rdev; /* device type */
> __int32_t st_size; /* file size, in bytes */
> - struct timespec st_atimespec; /* time of last access */
> - struct timespec st_mtimespec; /* time of last data modification */
> - struct timespec st_ctimespec; /* time of last file status change */
> + struct timespec st_atim; /* time of last access */
> + struct timespec st_mtim; /* time of last data modification */
> + struct timespec st_ctim; /* time of last file status change */
> __int32_t st_blksize; /* optimal blocksize for I/O */
> __int32_t st_blocks; /* blocks allocated for file */
> fflags_t st_flags; /* user defined flags for file */
> @@ -136,28 +127,18 @@ struct stat {
> uid_t st_uid; /* user ID of the file's owner */
> gid_t st_gid; /* group ID of the file's group */
> __dev_t st_rdev; /* device type */
> -#if __BSD_VISIBLE
> - struct timespec st_atimespec; /* time of last access */
> - struct timespec st_mtimespec; /* time of last data modification */
> - struct timespec st_ctimespec; /* time of last file status change */
> -#else
> - time_t st_atime; /* time of last access */
> - long __st_atimensec; /* nsec of last access */
> - time_t st_mtime; /* time of last data modification */
> - long __st_mtimensec; /* nsec of last data modification */
> - time_t st_ctime; /* time of last file status change */
> - long __st_ctimensec; /* nsec of last file status change */
> -#endif
> + struct timespec st_atim; /* time of last access */
> + struct timespec st_mtim; /* time of last data modification */
> + struct timespec st_ctim; /* time of last file status change */
... and the mess of ifdefs and compatibility cruft needs to become even
larger here:
- pre-POSIX-2008: something like the old code
- post-POSIX-2008: something like the new code
Since we only want to support old POSIX applications and not old FreeBSD
applications, not quite such a large mess is needed -- struct timespec
must not be visible for old POSIX, but there is no need to expose the
nanoseconds parts for old POSIX/FreeBSD.
Hmm, this was already quite broken, and sys/_timespec.h was already
almost completely bogus, since the struct __timespec carefully declared
in it was almost completely unused. The old code should have used
__timespec instead of timespec in the above and then it wouldn't need
visibility ifdefs, provided compilers aren't super-sensitive to the
spelling of the struct tag. The new code can do something similar
to avoid ifdefs. But don't have a sys/_timespec.h just to declare
struct __timespec which is only used in this file.
Note that we got the declaration of struct timespec via the massive
namespace pollution in sys/time.h. My version fixed the pollution long
ago, except it included sys/timespec.h to directly get the much smaller
amount of pollution depended on by the above (sys.stat.h never didn't
depend on any other pollution):
% Index: stat.h
% ===================================================================
% RCS file: /home/ncvs/src/sys/sys/stat.h,v
% retrieving revision 1.40
% diff -u -2 -r1.40 stat.h
% --- stat.h 17 Jun 2004 17:16:52 -0000 1.40
% +++ stat.h 20 Jun 2004 02:09:28 -0000
% @@ -89,13 +89,7 @@
% #endif
%
% -#if !defined(_KERNEL) && __BSD_VISIBLE
% -/*
% - * XXX we need this for struct timespec. We get miscellaneous namespace
% - * pollution with it.
% - */
% -#include <sys/time.h>
% -#endif
% -
% -#if !__BSD_VISIBLE
% +#if __BSD_VISIBLE
% +#include <sys/timespec.h>
% +#else
% #include <sys/_timespec.h>
% #endif
Apparently I never tested the !__BSD_VISIBLE case. Since struct __timespec
is mostly unused, the above won't actually compile when only sys/_timespec.h
is included.
> @@ -166,12 +147,6 @@ struct stat {
> */
> unsigned int :(8 / 2) * (16 - (int)sizeof(struct timespec));
> unsigned int :(8 / 2) * (16 - (int)sizeof(struct timespec));
> -#else
> - time_t st_birthtime; /* time of file creation */
> - long st_birthtimensec; /* nsec of file creation */
> - unsigned int :(8 / 2) * (16 - (int)sizeof(struct __timespec));
> - unsigned int :(8 / 2) * (16 - (int)sizeof(struct __timespec));
> -#endif
> };
>
> #if __BSD_VISIBLE
For some reason, this actually used struct __timespec. This was the only
use of struct __timespec in any version of this file. The reason seems
to have been that sys/_timespec was never used as intended, except I used
it for the above 2 lines because I didn't notice its full lack of use
when I wrote these lines.
> @@ -200,13 +175,23 @@ struct nstat {
> };
> #endif
>
> +#ifndef _KERNEL
> +#define st_atime st_atim.tv_sec
> +#define st_mtime st_mtim.tv_sec
> +#define st_ctime st_ctim.tv_sec
Excessive horizontal whitespace in new code.
> #if __BSD_VISIBLE
> -#define st_atime st_atimespec.tv_sec
> -#define st_mtime st_mtimespec.tv_sec
> -#define st_ctime st_ctimespec.tv_sec
> -#define st_birthtime st_birthtimespec.tv_sec
Subexcessive horizontal whitespace including missing tabs in old code.
I wonder why we didn't use the struct for the !__BSD_VISIBLE case. It
might have been due to compilers being super-sensitive to the types,
or the macros being technically invalid. If so, the new macros would
be technically invalid for both old-FreeBSD and old-POSIX. (In new-FreeBSD
and new-POSIX, they are standard so applications are not allowed to #undef
them, etc., though this is mostly undocumented.) The !__BSD_VISIBLE case
was fragile since the compiler might pad the structs differently to the
scalars, but I think the layout was always the same in practice.
> Modified: head/sys/sys/timespec.h
> ==============================================================================
> --- head/sys/sys/timespec.h Sun Mar 28 12:55:31 2010 (r205791)
> +++ head/sys/sys/timespec.h Sun Mar 28 13:13:22 2010 (r205792)
> @@ -31,22 +31,11 @@
> * $FreeBSD$
> */
>
> -/*
> - * Prerequisites: <sys/cdefs.h>, <sys/_types.h>
> - */
> -
These should still be prerequisites. Their includes were intentionally
left out, partly to inhibit direct includes of this header.
> #ifndef _SYS_TIMESPEC_H_
> #define _SYS_TIMESPEC_H_
>
> -#ifndef _TIME_T_DECLARED
> -typedef __time_t time_t;
> -#define _TIME_T_DECLARED
> -#endif
> -
> -struct timespec {
> - time_t tv_sec; /* seconds */
> - long tv_nsec; /* and nanoseconds */
> -};
> +#include <sys/cdefs.h>
> +#include <sys/_timespec.h>
It's disgusting to include sys/_timespec.h (which shouldn't exist) several
layers deep just to uniquize the declaration of struct timespec.
>
> #if __BSD_VISIBLE
> #define TIMEVAL_TO_TIMESPEC(tv, ts) \
>
Here is yet more pollution in the primary timespec header. Not too bad
since it is ifdefed. But this belongs near timevals, not near timespecs,
since it is bogus compatibility cruft for the former. It can't even be
used unless the header which declares the former is included. It should
only be declared in that header, only in the kernel. That header is
sys/time.h which already declares timespecs and might still do so when
its pollution is removed.
Bruce
More information about the svn-src-head
mailing list