RFC: futimens(2) and utimensat(2)

[Bruce Evans]

On Wed, 29 Feb 2012, Sergey Kandaurov wrote:

> On 29 February 2012 10:40, Xin Li <delphij at delphij.net> wrote:
>>
>> These are required by IEEE Std 1003.1-2008.  Patchset at:
>>
>> http://people.freebsd.org/~delphij/for_review/utimens.diff

I didn't look at this because it wasn't in the mail :-).

> This is the older version last time discussed with jilles.
> It misses man page update and compat32 parts (both were
> done since then except missing ERROR section in utimes(2).
> e.g. my compat32 version is just as yours :)).
> I started to commit my version (you can see r227447) but
> failed due to missing ERROR section, my lack of english to
> rewrite utimes(2) man page, and too complicated and wrong
> ERROR section in the existing utimes(2).
>
> http://plukky.net/~pluknet/patches/utimes.2008.3.diff
>
> It is pretty similar to your except I done getutimens() a bit different.
> I had to introduce such complication to pass all tests.
> Take note on private flags UTIMENS_NULL and UTIMENS_EXIT.
>
> Index: sys/kern/vfs_syscalls.c
> ===================================================================
> --- sys/kern/vfs_syscalls.c	(revision 220831)
> +++ sys/kern/vfs_syscalls.c	(working copy)
> ...
> static int
> +getutimens(usrtsp, tspseg, tsp, retflags)
> +	const struct timespec *usrtsp;
> +	enum uio_seg tspseg;
> +	struct timespec *tsp;
> +	int *retflags;

Should probably not use K&R function definitions in new code.

> +{
> +	int error;
> +	struct timespec tsnow;

Structs should be sorted before scalars (and pointers).

> +
> +	vfs_timestamp(&tsnow);

Not used in all paths.

> +	*retflags = 0;

Not used in all paths?

> +	if (usrtsp == NULL) {
> +		tsp[0] = tsnow;
> +		tsp[1] = tsnow;
> +		*retflags |= UTIMENS_NULL;
> +		return (0);
> +	}
> +	if (tspseg == UIO_SYSSPACE) {
> +		tsp[0] = usrtsp[0];
> +		tsp[1] = usrtsp[1];
> +	} else if ((error = copyin(usrtsp, tsp, sizeof(*tsp) * 2)) != 0)
> +			return (error);

Indentation.

> +

Extra blank line.  Many more of these below.

> +	if (tsp[0].tv_nsec == UTIME_OMIT && tsp[1].tv_nsec == UTIME_OMIT)
> +		*retflags |= UTIMENS_EXIT;
> +	if (tsp[0].tv_nsec == UTIME_NOW && tsp[1].tv_nsec == UTIME_NOW)
> +		*retflags |= UTIMENS_NULL;
> +
> +	if (tsp[0].tv_nsec == UTIME_OMIT)
> +		tsp[0].tv_sec = VNOVAL;

tsp[0].tv_nsec is not initialized (except it is UTIME_OMIT, which might
be the same as VNOVAL).  The patch seems to be missing the header part
that defines UTIME_OMIT).  Most setattr vnops are sloppy about checking
both tv_sec and tv_nsec, but VATTR_NULL() sets both to VNOVAL for
setattrs that don't request a time change.  More care is actually
required in the opposite direction -- getattr defaults va_birthtime.
tv_sec.tv_nsec to -1.0, so that when a getattr doesn't understand
birthime it comes back back unchanged as -1.0 which gives the error
value (time_t)-1.  All attributes for getattr should be defaulted like
this so that all file systems don't have to know about them, but only
va_birthtime, va_fsid and va_rdev are (all the others default to stack
garbage).

> +	else if (tsp[0].tv_nsec == UTIME_NOW)
> +		tsp[0] = tsnow;
> +	else if (tsp[0].tv_nsec < 0 || tsp[0].tv_nsec >= 1000000000L)
> +		return (EINVAL);
> +
> +	if (tsp[1].tv_nsec == UTIME_OMIT)
> +		tsp[1].tv_sec = VNOVAL;
> +	else if (tsp[1].tv_nsec == UTIME_NOW)
> +		tsp[1] = tsnow;
> +	else if (tsp[1].tv_nsec < 0 || tsp[1].tv_nsec >= 1000000000L)
> +		return (EINVAL);

Is it possible to extend this API to support birthtimes (and with more
security control, ctimes)?  Encoding more in tv_nsec should do it.
Certain magic values in tsp[1].tv_nsec  would indicate that there are
more than 2 entries in tsp[].  An extra copyin is needed to read the
extra entries (after reading tsp[1] to see if there are more).  Better
add this before the ABI solidifies.

This would have worked for utimes() too, with with magic in tsp[1].tv_usec,
but this seems unnecessary now.

Bruce