Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 22 Jul 2008 22:41:12 +1000 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Jaakko Heinonen <jh@saunalahti.fi>
Cc:        freebsd-fs@FreeBSD.org, ighighi@gmail.com
Subject:   Re: birthtime initialization
Message-ID:  <20080722215249.K17453@delplex.bde.org>
In-Reply-To: <20080722075718.GA1881@a91-153-120-204.elisa-laajakaista.fi>
References:  <200806020800.m528038T072838@freefall.freebsd.org> <20080722075718.GA1881@a91-153-120-204.elisa-laajakaista.fi>

next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 22 Jul 2008, Jaakko Heinonen wrote:

> On 2008-06-02, Bruce Evans wrote:
> [about patch for ext2fs in PR kern/122047]
>>  % +	vap->va_birthtime.tv_sec = 0;
>>  % +	vap->va_birthtime.tv_nsec = 0;
>>
>>  This is unrelated and should be handled centrally.  Almost all file
>>  systems get this wrong.  Most fail to set va_birthtime, so stat()
>>  returns kernel stack garbage for st_birthtime.  ffs1 does the same
>>  as the above.  msdosfs does the above correctly, by setting tv_sec to
>>  (time_t)-1 in unsupported cases.
>
> How about this patch?
>
> %%%
> Index: sys/kern/vfs_vnops.c
> ===================================================================
> --- sys/kern/vfs_vnops.c	(revision 180588)
> +++ sys/kern/vfs_vnops.c	(working copy)
> @@ -703,6 +703,9 @@ vn_stat(vp, sb, active_cred, file_cred,
> #endif
>
> 	vap = &vattr;
> +	/* Not all file systems initialize birthtime. */
> +	VATTR_NULL(vap);
> +
> 	error = VOP_GETATTR(vp, vap, active_cred, td);
> 	if (error)
> 		return (error);

I want to initialize va_birthtime to { -1, 0 } here only.  Don't
initialize the whole vattr here.  VOP_GETTATR() is supposed to initalize
everything, but doesn't for va_birthtime.  If there any other fields
that VOP_GETTATR() doesn't initialize, then these should be searched
for and fixed instead of setting them to the garbage value given by
vattr_null.  Similarly, if there are any fields that aren't supported
by most file systems, then they should be searched for and defaulted
like va_birthtime instead of requiring indivual file systems to invent
a default value for them.

> Index: sys/ufs/ufs/ufs_vnops.c
> ...
> Index: sys/fs/msdosfs/msdosfs_vnops.c
> ...
> Index: sys/nfsclient/nfs_subs.c

There are a probably more file systems that have missing or slightly
incorrect (all zero) settings of va_birthtime.

> The patch adds VATTR_NULL() call to vn_stat() to initialize the vattr
> structure before VOP_GETATTR() call. VATTR_NULL() initializes
> va_birthtime.tv_sec and va_birthtime.tv_nsec to -1 (VNOVAL). I also
> changed UFS1 and msdosfs to use consistent values. NFS needs explicit
> initialization because otherwise values would be set to 0 due to memory
> obtained with M_ZERO flag.

VNOVAL = -1 only accidentally gives the correct value for va_birthtime.tv_sec.
It gives a wrong value for va_birthtime.tv_nsec.  It is better to set
va_birthtime.tv_sec explicitly to -1.  This -1 is only accidentantally
equal to VNOVAL.  Fortunately, this accident doesn't prevent VOP_GETATTR()
from setting va_birthtime, since VNOVAL is only magic for VOP_SETATTR().

phk replied (but didn't quote enough, so I merged this manually):

>> Looks like something Kirk forgot to me.

>> We want to macroize the NOVAL for timespec instead of spreading
>> -1 casts all over.

This isn't a problem for the "GET" interface since VNOVAL doesn't apply
to it.

Also, the casts of -1 aren't really needed.  ufs_settattr() doesn't
have them for time_t's, and vattr_null() doesn't have them for anything.

The correctness of this depends on the type of time_t (and the other
va field times).  In userland we're supposed to cast -1 to time_t for
error detection in mktime() etc.  In userland, time_t can be any
arithmetic time so it is possible for (time_t)-1 != -1.  Even there,
I think there is only a problem if time_t is an unsigned intergral
type shorter than int.  Compilers may warn about other cases.
ufs_settatr() has the casts for va_bytes (bogus cast of va_bytes to
int, which breaks its value), va_uid, va_gid and va_mode.  For va_mode,
there is a problem -- the same one as in my example for time_t above --
va_mode is u_short so it cannot equal -1 (after the default promotions)
except on exotic systems.  For va_uid and va_gid, the casts were needed
15 years ago when uid_t and gid_t were 16 bits.  I can't see any problem
with omitting the cast for va_bytes -- va_bytes is u_quad_t, which is
certainly at least as large as int, so it can equal VNOVAL = -1 after
the default promotions though it cannot represent any negative value
(now C's conversion rules requires (uquad_t)-1 == -1, and it would be
a compiler bug to warn about expressions that depend on these rules).

In vattr_null(), the assignments go the other way and VNOVAL = -1
always gets converted to the intended value (which is not always -1).
C's conversion rules are depended on even more here to do something
reasonable with (foo_t)-1.

I wouldn't like VNOVAL being replaced by VNOTIMESPECVAL, VNOUIDVAL,
... etc.  Recently I noticed a commit that replaced (struct foo *)0
by NULL together with less contentions replacements of plain 0 by NULL.
Old code that tries to be careful uses (struct foo *)0 (or a macro
NULLFOO for this) too much.  Now that NULL is Standard we can just use
plain NULL.  Similarly for plain VNOVAL except in a few cases where
-1 doesn't get converted right.

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080722215249.K17453>